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: no write barrier for stores to reflect.{Slice,String}Header #19168

Closed
mdempsky opened this issue Feb 18, 2017 · 20 comments

Comments

Projects
None yet
7 participants
@mdempsky
Copy link
Member

commented Feb 18, 2017

Package unsafe's godocs explicitly state this is safe:

hdr.Data = uintptr(unsafe.Pointer(p))              // case 6 (this case)

But compiling this code does not produce any write barriers:

package p

import (
	"reflect"
	"unsafe"
)

func F(hdr *reflect.SliceHeader, p *byte) {
	hdr.Data = uintptr(unsafe.Pointer(p))
}

func G(hdr *reflect.StringHeader, p *byte) {
	hdr.Data = uintptr(unsafe.Pointer(p))
}

We should probably change needwritebarrier(l) to return true when l is an ODOTPTR for accessing reflect.{Slice,String}Header.Data.

This should probably be backported to 1.8 too?

/cc @aclements @rsc

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2017

Actually, I think the fix is slightly more involved than that. This program is valid:

package main

import "reflect"

func main() {
    var hdr reflect.SliceHeader
    p := &hdr
    p.Data = 42
}

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

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2017

I'm not sure we guarantee that SliceHeader actually works any more. Its documentation wording is quite strong:

It cannot be used safely or portably and its representation may change in a later release.

I'm not sure you can ever reliably detect writes to it.

var hdr reflect.SliceHeader
p = &hdr.Data
some_crazy_function_that_ends_up_writing_to_its_arg(p)
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2017

The unsafe package docs provide specific guarantees for reflect.SliceHeader and reflect.StringHeader that must continue to work. I think @mdempsky is right that this will require special handling in the compiler.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2017

That said, I think we can modify the unsafe package docs to reject the case you mention: I think we can forbid modifying the Data field in any way other than x.Data = uintptr(p) where p is some pointer type (meaning that if p is not unsafe.Pointer it must be converted to unsafe.Pointer to support the conversion to uintptr).

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2017

@ianlancetaylor I'm okay with forbidding assigning to x.Data through a pointer (that's how I currently interpret the docs already), but I don't think we should restrict the RHS. E.g., I think x.Data = uintptr(p) + 1 should be valid.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2017

The doc of unsafe package also says (at the end)

A program should not declare or allocate variables of these struct types.

// INVALID: a directly-declared header will not hold Data as a reference.
var hdr reflect.StringHeader
hdr.Data = uintptr(unsafe.Pointer(p))
hdr.Len = n
s := *(*string)(unsafe.Pointer(&hdr)) // p possibly already lost

So I don't think it needs write barrier, which is to hold Data as reference.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2017

@cherrymui The distinction there is memory is being allocated for a variable of type reflect.StringHeader, so Data's memory slot doesn't get marked as containing a pointer.

In the valid example, memory is allocated for a variable of type string, and only modified indirectly through a *reflect.StringHeader pointer.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2017

@mdempsky Oh, I see the difference. But I still think that hdr.Data = uintptr(unsafe.Pointer(p)) doesn't mean to hold a reference to p.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2017

I think the problem here is the hybrid write barrier - the old value of hdr.Data might need saving, even if you agree that p is not referred to.
@aclements

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2017

We discussed this internally:

  1. Assignments of the precise form p.Data = u where p has type *reflect.SliceHeader or *reflect.StringHeader require a write barrier. We do not need to support q := &p.Data; *q = u.
  2. The package unsafe docs already warn "A program should not declare or allocate variables of these struct types." so the p.Data = 42 example I posted above is invalid anyway.

@mdempsky mdempsky self-assigned this Feb 21, 2017

@cespare

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

1.8.1 material? Has the write barrier always been missing, or is this new to 1.8?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

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.

@ianlancetaylor ianlancetaylor added this to the Go1.8.1 milestone Feb 21, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Mar 2, 2017

CL https://golang.org/cl/37663 mentions this issue.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

For go 1.9, can we define in package unsafe:

type StringHeader struct {
    Data Pointer
    Len int
}

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?

@gopherbot gopherbot closed this in 0ee9c46 Mar 2, 2017

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

@randall77 I just filed #19367.

@aclements

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

Re-opening for cherry-pick to 1.8.1

@gopherbot

This comment has been minimized.

Copy link

commented Mar 28, 2017

CL https://golang.org/cl/38738 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 29, 2017

cmd/compile: added special case for reflect header 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/38738
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot

This comment has been minimized.

Copy link

commented Apr 5, 2017

CL https://golang.org/cl/39615 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 5, 2017

CL https://golang.org/cl/39616 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 5, 2017

[release-branch.go1.8] cmd/compile: add missing WBs for reflect.{Slic…
…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>

gopherbot pushed a commit that referenced this issue Apr 5, 2017

[release-branch.go1.8] cmd/compile: added special case for reflect he…
…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>
@aclements

This comment has been minimized.

Copy link
Member

commented Apr 5, 2017

Cherry-picked to release.

@aclements aclements closed this Apr 5, 2017

lparth added a commit to lparth/go that referenced this issue Apr 13, 2017

cmd/compile: added special case for reflect header 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 golang#19168 to escape analysis.

Includes extra debugging that was helpful.

Fixes golang#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>

@golang golang locked and limited conversation to collaborators Apr 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.