It seems the issue has to do with the compiler inlining `foo`. If I add the
compiler directive `//go:noinline` above `foo` and run `go test` the test
fails, just like when I run `go test -covermode=atomic`. This makes sense
because if the function is inlined and the slice is allocated on the stack
the string will still point to valid data. On the other hand, if the
function is not inlined, as I expect is the case when generating code
coverage, then if the slice is allocated on the stack when `foo` returns
the string could point to invalid data. In any case, it seems the compiler
is not able to "see" this particular pointer swap and may, in fact, be
allocating the slice on the stack.
On Sunday, March 26, 2017 at 9:57:15 PM UTC-4, Jerome Froelich wrote:
>
> Interesting, I actually ran into some odd behavior with this function that
> I'm not sure how to explain. Consider the following test:
>
> import (
> "reflect"
> "testing"
> "unsafe"
> )
>
> func sliceToStringUnsafe(v []byte) string {
> var s string
> sh := (*reflect.StringHeader)(unsafe.Pointer(&s))
> sh.Data = uintptr(unsafe.Pointer(&v[0]))
> sh.Len = len(v)
> return s
> }
>
> func foo() string {
> v := []byte{"foobarbaz"}
> s := sliceToStringUnsafe(v)
> return s
> }
>
> func TestFoo(t *testing.T) string {
> foo := foo()
> require.Equal(t, "foobarbaz", foo)
> }
>
> If I run this test with `go test` it passes. However, if I run the test
> with `go test -covermode atomic` it fails and foo contains random data.
> Perhaps the `covermode atomic` affects how the compiler performs escape
> analysis?
>
>
>
> On Saturday, March 25, 2017 at 12:43:38 PM UTC-4, Keith Randall wrote:
>>
>> That is a worry, but escape analysis can handle this case. It
>> understands flow of pointers through unsafe.Pointer and uintptr. As a
>> consequence, in this example it forces the array to be allocated on the
>> heap.
>>
>> The runtime has on occasion a need to hide values from escape analysis.
>> It has to do some weird math on the uintptr to hide the escape. It uses:
>>
>> // noescape hides a pointer from escape analysis. noescape is
>> // the identity function but escape analysis doesn't think the
>> // output depends on the input. noescape is inlined and currently
>> // compiles down to zero instructions.
>> // USE CAREFULLY!
>> //go:nosplit
>> func noescape(p unsafe.Pointer) unsafe.Pointer {
>> x := uintptr(p)
>> return unsafe.Pointer(x ^ 0)
>> }
>>
>>
>> On Friday, March 24, 2017 at 11:58:31 AM UTC-7, Jerome Froelich wrote:
>>>
>>> It just occurred to me that there may actually be a problem with the
>>> conversion functions in the example. Specifically, since the Go
>>> compiler can allocate byte slices on the heap if they do not escape, the
>>> following function may be invalid:
>>>
>>> func foo() string {
>>> v := []uint64{1,2,3}
>>> s := sliceToStringUnsafe(v)
>>> return s
>>> }
>>>
>>> Admittedly, this is a contrived example but I think it can illustrate
>>> the issue. As noted on this previous thread
>>> <https://groups.google.com/forum/#!topic/golang-nuts/KdbtOqNK6JQ> the
>>> compiler will allocate slices on the heap if it can prove they do not
>>> escape. Consequently, in the function above, the data for v can be
>>> allocated on the stack. In such a case, since sliceToStringUnsafe only
>>> adjusts pointers, the string it returns will point to this stack-allocated
>>> data. However, once foo returns, this data is no longer valid and can be
>>> overwritten by subsequent functions that are pushed on the stack.
>>>
>>> On Thursday, March 23, 2017 at 9:05:48 PM UTC-4, Caleb Spare wrote:
>>>>
>>>> Filed: https://github.com/golang/go/issues/19687
>>>>
>>>> On Thu, Mar 23, 2017 at 4:41 PM, 'Keith Randall' via golang-nuts
>>>> <golan...@googlegroups.com> wrote:
>>>> >
>>>> >
>>>> > On Thursday, March 23, 2017 at 4:24:40 PM UTC-7, Caleb Spare wrote:
>>>> >>
>>>> >> That's very good to know. Thanks, Ian.
>>>> >>
>>>> >> Unfortunately if I use this KeepAlive-based fix, p escapes and so
>>>> the
>>>> >> function now allocates. I guess I'll stick with the original version
>>>> >> from my first email.
>>>> >>
>>>> >> Does this indicate a shortcoming of either compiler support for
>>>> >> KeepAlive or escape analysis in general?
>>>> >>
>>>> >
>>>> > KeepAlive shouldn't be making things escape. If that is happening
>>>> you
>>>> > should file a bug for it.
>>>> > The definition is:
>>>> >
>>>> > //go:noinline
>>>> > func KeepAlive(interface{}) {}
>>>> >
>>>> > which should be pretty easy to analyze :)
>>>> >
>>>> >> Caleb
>>>> >>
>>>> >> On Thu, Mar 23, 2017 at 10:26 AM, Ian Lance Taylor <ia...@golang.org>
>>>>
>>>> >> wrote:
>>>> >> > On Thu, Mar 23, 2017 at 9:16 AM, Caleb Spare <ces...@gmail.com>
>>>> wrote:
>>>> >> >>
>>>> >> >> Brief follow-up: does the seeming validity of the code rely at
>>>> all on
>>>> >> >> the fact that the indicated line is written as a single line?
>>>> What if,
>>>> >> >> instead, a *StringHeader var were extracted?
>>>> >> >>
>>>> >> >> func stringToSliceUnsafe(s string) []uint64 {
>>>> >> >> var v []uint64
>>>> >> >> h := (*reflect.StringHeader)(unsafe.Pointer(&s)) // <--
>>>> >> >> sh := (*reflect.SliceHeader)(unsafe.Pointer(&v))
>>>> >> >> sh.Data = h.Data
>>>> >> >> sh.Len = h.Len >> 3
>>>> >> >> sh.Cap = h.Len >> 3
>>>> >> >> return v
>>>> >> >> }
>>>> >> >>
>>>> >> >> (Play link: https://play.golang.org/p/BmGtYTsGNY)
>>>> >> >>
>>>> >> >> Does h keep s alive? A strict reading of rule 6 doesn't seem to
>>>> say
>>>> >> >> that keeping a *StringHeader or *SliceHeader around keeps the
>>>> >> >> underlying string/slice alive (but it's sort of implied by the
>>>> rule 6
>>>> >> >> example code, which doesn't refer to s after converting it to a
>>>> >> >> *StringHeader).
>>>> >> >
>>>> >> > That is an interesting point. I don't think there is anything
>>>> keeping
>>>> >> > s alive here. I think this isn't quite the same as the example in
>>>> the
>>>> >> > docs, because that example is assuming that you are doing to use s
>>>> >> > after setting the fields--why else would you be doing that? In
>>>> this
>>>> >> > case it does seem theoretically possible that s could be freed
>>>> between
>>>> >> > the assignment to h and the use of h.Data. With the current and
>>>> >> > foreseeable toolchains it's a purely theoretical problem, since
>>>> there
>>>> >> > is no point there where the goroutine could be preempted and the
>>>> fact
>>>> >> > that s is no longer referenced be detected. But as a theoretical
>>>> >> > problem it does seem real. One fix would be something like
>>>> >> > p := &s
>>>> >> > h := (*reflect.StringHeader)(unsafe.Pointer(p))
>>>> >> > sh := (*reflect.SliceHeader)(unsafe.Pointer(&v))
>>>> >> > sh.Data = h.Data
>>>> >> > sh.Len = ...
>>>> >> > sh.Cap = ...
>>>> >> > runtime.KeepAlive(p)
>>>> >> >
>>>> >> > Ian
>>>> >
>>>> > --
>>>> > You received this message because you are subscribed to the Google
>>>> Groups
>>>> > "golang-nuts" group.
>>>> > To unsubscribe from this group and stop receiving emails from it,
>>>> send an
>>>> > email to golang-nuts...@googlegroups.com.
>>>> > For more options, visit https://groups.google.com/d/optout.
>>>>
>>>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
