cmd/compile: enable -d=checkptr as part of -race and/or -msan? #34964
Comments
mdempsky
added
the
NeedsDecision
|
Change https://golang.org/cl/201783 mentions this issue: |
It can still be manually disabled again using -d=checkptr=0. It's also still disabled by default for GOOS=windows, because the Windows standard library code has a lot of unsafe pointer conversions that need updating. Updates #34964. Change-Id: Ie0b8b4fdf9761565e0dcb00d69997ad896ac233d Reviewed-on: https://go-review.googlesource.com/c/go/+/201783 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
Pointer checking is now enabled automatically when -race or -msan are specified (except on Windows, because of remaining Windows-specific work for #34972 (comment)). I've marked this issue as release-blocker for Go 1.14 so we make sure to make an explicit decision on the matter before release. In the mean time, I think it's valuable at least to get extra mileage for the pointer checking instrumentation. |
|
Change https://golang.org/cl/203837 mentions this issue: |
Even on platforms that allow unaligned reads, the Go runtime assumes that a pointer to a given type has the alignment required by that type. Fixes golang/go#35173 Updates golang/go#34972 Updates golang/go#34964 Change-Id: I90361e096e59162e42ebde2914985af92f777ece Reviewed-on: https://go-review.googlesource.com/c/crypto/+/203837 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org>
|
Another thing to consider before release too: during discussion on #22218, @rsc suggested "make it a throw, not a panic", but the current implementation does use panic. It should be easy to change to throw instead. We could also make it controllable via GODEBUG at runtime. I don't have any strong opinions here. (I think it only panics because in the moment of trying to debug some corner case behavior, it was quicker to use panic because it automatically handled formatting integers for me.) |
|
I don't write unsafe code myself, only via third party dependencies (so take my opinion with a grain of salt), but my main want would be to have a message printed like how race mode prints a message (and during Right now, a panic coming from checkptr can be recovered and you'll never see it unless you're careful (as in yuin/gopher-lua#254 which I got from running gotip in race mode in my CI). A throw could work, but would exit the process. Configurable is nice too. |
|
Change https://golang.org/cl/214118 mentions this issue: |
Hopefully we'll have the remaining safety violations in the standard library ironed out by 1.15. We also fix a minor (but important) typo while we're here. Updates #34964, #34972. Change-Id: Ic72fd4d9411b749f8c0cea87e95ab68347009893 Reviewed-on: https://go-review.googlesource.com/c/go/+/214118 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
Since this seems to be working pretty smoothly, we've decided to go ahead and keep -race/-msan setting -d=checkptr by default for the release, except on Windows where it's still disabled because of some remaining issues in std. I've just updated the release notes to mention this. Since this issue was about making this decision, I'm going to go ahead and close it. We'll continue to track remaining std fixes in #34972. |
|
Was there any discussion as to whether checkptr should be using |
|
(Reopening to make sure we resolve the panic vs throw question.) |
|
I'm not inclined to make it configurable, since that seems like an unnecessary knob and it fails my knob litmus test of "I can give clear guidance on how to set this knob." A throw seems pretty reasonable to me. If panics are being silently eaten by a system, that seems like a bug in the system to me, but I'm also fine with not making this a panic. |
|
Change https://golang.org/cl/214217 mentions this issue: |
Updates #34964. Change-Id: I5afb2c1e77a9a47358a1d0d108c4a787d7172b94 Reviewed-on: https://go-review.googlesource.com/c/go/+/214217 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Austin Clements <austin@google.com>
|
Closing again since we've decided to use |
|
Change https://golang.org/cl/227003 mentions this issue: |
CL 201783 enable -d=checkptr when -race or -msan is specified everywhere but windows. But, now that all unsafe pointer conversions in the standard library are fixed, enable -d=checkptr even on windows. Updates #34964 Updates #34972 Change-Id: Id912fa83b0d5b46c6f1c134c742fd94d2d185835 Reviewed-on: https://go-review.googlesource.com/c/go/+/227003 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
As discussed on golang-dev, the new -d=checkptr instrumentation is compatible with -race and -msan and likely cheaper than either of them (about the cost of two runtime.findObject calls per conversion involving unsafe.Pointer), so maybe it should just be enabled as one of those flags.
It would be easy to tweak cmd/compile to enable -d=checkptr by default when -race or -msan are specified, and then to still allow
-race -d=checkptr=0to turn it back off (i.e., race instrumentation without pointer checking). I can do that now so we get some extra usage testing of -d=checkptr (thanks to existing builders that use -race, etc), and then closer to release we can re-evaluate the best user experience?/cc @aclements @bradfitz @rsc
The text was updated successfully, but these errors were encountered: