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: -d=checkptr should disable inlining reflect.Value.{Pointer,UnsafeAddr} even when package reflect is compiled without it #35073
Comments
Yeah those programs are both valid. Weird, I thought checkptr was handling this: go/src/cmd/compile/internal/gc/walk.go Lines 3961 to 3966 in 19d2a1c
|
I think the reproducers are representative of the issue, but nonetheless I'd like to add the stacktrace of the original panic from my wI2L/jettison project. godev test -race -covermode=atomic -gcflags=-d=checkptr
--- FAIL: TestCompositeTypes (0.00s)
panic: (runtime.ptrArith) (0x81c900,0xc00000eb80) [recovered]
panic: (runtime.ptrArith) (0x81c900,0xc00000eb80)
goroutine 12 [running]:
testing.tRunner.func1(0xc0000c2b00)
/home/wpoussie/github/golang/go/src/testing/testing.go:874 +0x69f
panic(0x81c900, 0xc00000eb80)
/home/wpoussie/github/golang/go/src/runtime/panic.go:679 +0x1b2
github.com/wI2L/jettison.(*Encoder).encode(0xc0000e8ab0, 0x8c6220, 0x804020, 0x804020, 0xc0000e86c0, 0x8c0c60, 0xc0000e8ae0, 0x0, 0x0, 0x0, ...)
/home/wpoussie/github/wI2L/jettison/encoder.go:231 +0x1cb
github.com/wI2L/jettison.(*Encoder).Encode(0xc0000e8ab0, 0x804020, 0xc0000e86c0, 0x8c0c60, 0xc0000e8ae0, 0x0, 0x0, 0x0, 0x0, 0x0)
/home/wpoussie/github/wI2L/jettison/encoder.go:201 +0x1ba
github.com/wI2L/jettison.TestCompositeTypes(0xc0000c2b00)
/home/wpoussie/github/wI2L/jettison/encoder_test.go:187 +0x82a
testing.tRunner(0xc0000c2b00, 0x866220)
/home/wpoussie/github/golang/go/src/testing/testing.go:909 +0x19a
created by testing.(*T).Run
/home/wpoussie/github/golang/go/src/testing/testing.go:960 +0x652
exit status 2
FAIL github.com/wI2L/jettison 0.009s I will check tonight when I get home if the issue reproduce on darwin/amd64, because I didn't see any issue yesterday evening after I fixed the remaining unsafe pointer misuses per your suggestions in wI2L/jettison@293c0b0. |
It's interesting that:
panics, but:
doesn't. |
@cuonglm Good catch. So I've reran the test of my package wI2L/jettison again with |
@cuonglm Oh right. Thanks for noticing that! The UnsafeAddr and Pointer methods are marked as go:nocheckptr, which means they won't be inlined. This is necessary because otherwise walk.go can't recognize the function calls in their inlined forms. But go:nocheckptr only disables inlining when that package is compiled with -d=checkptr. That's why all= makes a difference here. I'll have to think about how best to handle this. Probably inl.go should be changed to disable inlining those calls based on whether the calling compilation unit is using checkptr, rather than whether package reflect was compiled with it. |
@mdempsky This seems work:
|
Ah no, it doesn't, as it will disable inline if compiled with |
I think the short-term solution here is just to declare that you have to enable The longer-term fix requires making inlining smarter about pragmas. This might be beneficial for the runtime too, since currently we disable inlining for a lot of runtime functions just because it's the easiest way to ensure write barriers are enabled/disabled correctly. |
@mdempsky Feel free to close this issue then, or keeping it for future references. |
@wI2L Thanks, I'd like to keep it open for now. There's fundamentally no reason |
@mdempsky Sorry to hjack here, but It sounds like |
@cuonglm What makes you say that? |
@mdempsky This one should be invalid per rule 3rd, but it passes:
I tried other invalid cases in rule 3rd, the only one that
|
@cuonglm The detection is best effort. It's expected to have false negatives, but it should never have false positives. In your example, If you change |
Thanks for details explanation 👍 |
Change https://golang.org/cl/222878 mentions this issue: |
@mdempsky, could you update the title of this issue to reflect your current understanding of the root cause? |
@bcmills Done. |
goos/goarch: linux/amd64
version: go version devel +7f98c0e Tue Oct 22 08:54:50 2019 +0000 linux/amd64
This program should be valid according to unsafe rules number 5.
When runned with
-d=checkptr
, it panics onruntime.PtrArith
.The rule number 5, unless I misunderstood something, specify that the uintptr-typed return of functions
UnsafeAddr
andPointer
MUST be converted immediately tounsafe.Pointer
after the call, but nothing about that unsafe pointer being reinterpreted immediately to another pointer type.This variant, where the pointer is reinterpreted and dereferenced at the same time panics as well.
/cc @mdempsky @cuonglm
Edit: @mdempsky for reference, I found this because of this line in the project we mentionned in #35027.
The text was updated successfully, but these errors were encountered: