Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: enable -d=checkptr as part of -race and/or -msan? #34964

Open
mdempsky opened this issue Oct 17, 2019 · 5 comments

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 17, 2019

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=0 to 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

@mdempsky mdempsky added this to the Go1.14 milestone Oct 17, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 17, 2019

Change https://golang.org/cl/201783 mentions this issue: cmd/compile: enable -d=checkptr when -race or -msan is specified

gopherbot pushed a commit that referenced this issue Oct 22, 2019
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>
@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 23, 2019

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.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 28, 2019

Change https://golang.org/cl/203837 mentions this issue: sha3: align (*state).storage

@changkun changkun referenced this issue Nov 1, 2019
0 of 16 tasks complete
gopherbot pushed a commit to golang/crypto that referenced this issue Nov 5, 2019
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>
@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Nov 6, 2019

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.)

@zikaeroh

This comment has been minimized.

Copy link

@zikaeroh zikaeroh commented Nov 13, 2019

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 go test, fails).

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.