-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: no write barrier for stores to reflect.{Slice,String}Header #19168
Comments
Actually, I think the fix is slightly more involved than that. This program is valid:
but writebarrierptr will complain about 42 being in the range (0, minPhysPageSize). Perhaps add a writebarrieruintptr variant for assigning to runtime.{Slice,String}Header.Data that just omits that check? Edit: This program is NOT valid. Package unsafe's documentation warns "A program should not declare or allocate variables of these struct types." |
I'm not sure we guarantee that SliceHeader actually works any more. Its documentation wording is quite strong:
I'm not sure you can ever reliably detect writes to it.
|
The unsafe package docs provide specific guarantees for |
That said, I think we can modify the unsafe package docs to reject the case you mention: I think we can forbid modifying the |
@ianlancetaylor I'm okay with forbidding assigning to |
The doc of unsafe package also says (at the end)
So I don't think it needs write barrier, which is to hold Data as reference. |
@cherrymui The distinction there is memory is being allocated for a variable of type In the valid example, memory is allocated for a variable of type |
@mdempsky Oh, I see the difference. But I still think that |
I think the problem here is the hybrid write barrier - the old value of |
We discussed this internally:
|
1.8.1 material? Has the write barrier always been missing, or is this new to 1.8? |
I could be wrong, but I think that is what is new to 1.8 is that missing the write barrier could conceivably actually matter in practice. Before the hybrid write barrier, which is new in 1.8, it's pretty hard to imagine a real program that would fail. With the hybrid write barrier, it's much easier to imagine that omitting the write barrier could leave a dangling reference on the stack. |
CL https://golang.org/cl/37663 mentions this issue. |
For go 1.9, can we define in package unsafe:
and encourage people to use that instead of the one in reflect. Then in Go 2 we can drop the reflect ones. Or even Go 1.10? |
@randall77 I just filed #19367. |
Re-opening for cherry-pick to 1.8.1 |
CL https://golang.org/cl/38738 mentions this issue. |
The uintptr-typed Data field in reflect.SliceHeader and reflect.StringHeader needs special treatment because it is really a pointer. Add the special treatment in walk for bug #19168 to escape analysis. Includes extra debugging that was helpful. Fixes #19743. Change-Id: I6dab5002f0d436c3b2a7cdc0156e4fc48a43d6fe Reviewed-on: https://go-review.googlesource.com/38738 Run-TryBot: David Chase <drchase@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
CL https://golang.org/cl/39615 mentions this issue. |
CL https://golang.org/cl/39616 mentions this issue. |
…e,String}Header.Data Fixes #19168. (*state).insertWBstore needed to be tweaked for backporting so that store reflect.{Slice,String}Header.Data stores still fallthrough and end the SSA block. This wasn't necessary at master because of CL 36834. Change-Id: I3f4fcc0b189c53819ac29ef8de86fdad76a17488 Reviewed-on: https://go-review.googlesource.com/39615 Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Austin Clements <austin@google.com>
…ader fields to esc The uintptr-typed Data field in reflect.SliceHeader and reflect.StringHeader needs special treatment because it is really a pointer. Add the special treatment in walk for bug #19168 to escape analysis. Includes extra debugging that was helpful. Fixes #19743. Change-Id: I6dab5002f0d436c3b2a7cdc0156e4fc48a43d6fe Reviewed-on: https://go-review.googlesource.com/39616 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Austin Clements <austin@google.com>
Cherry-picked to release. |
Package unsafe's godocs explicitly state this is safe:
But compiling this code does not produce any write barriers:
We should probably change
needwritebarrier(l)
to return true whenl
is anODOTPTR
for accessingreflect.{Slice,String}Header.Data
.This should probably be backported to 1.8 too?
/cc @aclements @rsc
The text was updated successfully, but these errors were encountered: