Skip to content

Commit

Permalink
Add logic to deal with reflect pkg changes on tip.
Browse files Browse the repository at this point in the history
This commit adds logic to gracefully handle the new internal reflect.Value
structure on tip as of golang commit 82f48826c6c7 as well as the internal
reflect.Value flag bit changes as of golang commit 90a7c3c86944.

It accomplishes this by doing some inspection at init time and choosing
the appropriate offsets and flag positions accordingly.  There was some
previous logic which dealt with a similar issue for golang commit
ecccf07e7f9d.  However, since the more recent commits essentially reverted
the change and also modify the flag bit positions, it made more sense to
rework the detection logic.  In particular, the new logic examines the
size of the reflect.Value struct to determine the difference and extracts
the kind from the flags to determine if the flags have been changed.

As a result, this commit allows spew to work properly with tip all the
back to Go 1.0.
  • Loading branch information
davecgh committed Oct 24, 2014
1 parent 3fdaf5c commit 1288542
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 51 deletions.
103 changes: 58 additions & 45 deletions spew/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,58 +25,71 @@ import (
"unsafe"
)

// offsetPtr, offsetScalar, and offsetFlag are the offsets for the internal
// reflect.Value fields.
var offsetPtr, offsetScalar, offsetFlag uintptr

// reflectValueOld mirrors the struct layout of the reflect package Value type
// before golang commit ecccf07e7f9d.
var reflectValueOld struct {
typ unsafe.Pointer
val unsafe.Pointer
flag uintptr
}
const (
// ptrSize is the size of a pointer on the current arch.
ptrSize = unsafe.Sizeof((*byte)(nil))
)

// reflectValueNew mirrors the struct layout of the reflect package Value type
// after golang commit ecccf07e7f9d.
var reflectValueNew struct {
typ unsafe.Pointer
ptr unsafe.Pointer
scalar uintptr
flag uintptr
}
var (
// offsetPtr, offsetScalar, and offsetFlag are the offsets for the
// internal reflect.Value fields. These values are valid before golang
// commit ecccf07e7f9d which changed the format. The are also valid
// after commit 82f48826c6c7 which changed the format again to mirror
// the original format. Code in the init function updates these offsets
// as necessary.
offsetPtr = uintptr(ptrSize)
offsetScalar = uintptr(0)
offsetFlag = uintptr(ptrSize * 2)

// flagKindWidth and flagKindShift indicate various bits that the
// reflect package uses internally to track kind information.
//
// flagRO indicates whether or not the value field of a reflect.Value is
// read-only.
//
// flagIndir indicates whether the value field of a reflect.Value is
// the actual data or a pointer to the data.
//
// These values are valid before golang commit 90a7c3c86944 which
// changed their positions. Code in the init function updates these
// flags as necessary.
flagKindWidth = uintptr(5)
flagKindShift = uintptr(flagKindWidth - 1)
flagRO = uintptr(1 << 0)
flagIndir = uintptr(1 << 1)
)

func init() {
// Older versions of reflect.Value stored small integers directly in the
// ptr field (which is named val in the older versions). Newer versions
// added a new field named scalar for this purpose which unfortuantely
// comes before the flag field. Further the new field is before the
// flag field, so the offset of the flag field is different as well.
// ptr field (which is named val in the older versions). Versions
// between commits ecccf07e7f9d and 82f48826c6c7 added a new field named
// scalar for this purpose which unfortunately came before the flag
// field, so the offset of the flag field is different for those
// versions.
//
// This code constructs a new reflect.Value from a known small integer
// and checks if the val field within it matches. When it matches, the
// old style reflect.Value is being used. Otherwise it's the new style.
v := 0xf00
vv := reflect.ValueOf(v)
upv := unsafe.Pointer(uintptr(unsafe.Pointer(&vv)) +
unsafe.Offsetof(reflectValueOld.val))

// Assume the old style by default.
offsetPtr = unsafe.Offsetof(reflectValueOld.val)
offsetScalar = 0
offsetFlag = unsafe.Offsetof(reflectValueOld.flag)

// Use the new style offsets if the ptr field doesn't match the value
// since it must be in the new scalar field.
if int(*(*uintptr)(upv)) != v {
offsetPtr = unsafe.Offsetof(reflectValueNew.ptr)
offsetScalar = unsafe.Offsetof(reflectValueNew.scalar)
offsetFlag = unsafe.Offsetof(reflectValueNew.flag)
// and checks if the size of the reflect.Value struct indicates it has
// the scalar field. When it does, the offsets are updated accordingly.
vv := reflect.ValueOf(0xf00)
if unsafe.Sizeof(vv) == (ptrSize * 4) {
offsetScalar = ptrSize * 2
offsetFlag = ptrSize * 3
}
}

// flagIndir indicates whether the value field of a reflect.Value is the actual
// data or a pointer to the data.
const flagIndir = 1 << 1
// Commit 90a7c3c86944 changed the flag positions such that the low
// order bits are the kind. This code extracts the kind from the flags
// field and ensures it's the correct type. When it's not, the flag
// order has been changed to the newer format, so the flags are updated
// accordingly.
upf := unsafe.Pointer(uintptr(unsafe.Pointer(&vv)) + offsetFlag)
upfv := *(*uintptr)(upf)
flagKindMask := uintptr((1<<flagKindWidth - 1) << flagKindShift)
if (upfv&flagKindMask)>>flagKindShift != uintptr(reflect.Int) {
flagKindShift = 0
flagRO = 1 << 5
flagIndir = 1 << 6
}
}

// unsafeReflectValue converts the passed reflect.Value into a one that bypasses
// the typical safety restrictions preventing access to unaddressable and

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 1, 2014

// unsafeReflectValue converts the passed reflect.Value into a one that bypasses
// the typical safety restrictions preventing access to unaddressable and
// unexported data.

A question about the unsafeReflectValue func. When you say "unaddressable and unexported data", is that one thing, or are there two separate concepts of "unaddressable data" and "unexported data"? I'm only aware of the latter, so if there is something else unsafeReflectValue does except allow access to unexported fields, can you give an example please?

If all unsafeReflectValue does is allow access to unexported fields, I want to ask another question. Have you considered and what are your thoughts on Roger Peppe's (@rogpeppe) approach of modifying the reflect.Value value, instead of "generating a new unprotected (unsafe) reflect.Value"?

I don't have the original source, but here's a copy that I played/tested here, to show you what I'm referring to.

Anyway, it's probably equivalent and either approach works, so it's not a big deal. But I wanted to ask and perhaps learn something new. Thank you.

This comment has been minimized.

Copy link
@davecgh

davecgh Nov 1, 2014

Author Owner

They are different. You can have something unaddressable and exported (i.e an exported field from a copy of a struct).

package main

import(
    "fmt"
    "reflect"
)

type foo struct {
    A int
}

func (f *foo) String() string {
    return fmt.Sprintf("0x%x", f.A)
}

func main(){
    // Notice how f.A is exported, but it's not addressable because it
    // was accessed via a copy of the struct.  Also notice how the stringer
    // is not invoked because the type expects a pointer receiver.  Since vf
    // is not addressable, calling vf.Addr() will panic.
    f := foo{1234}
    vf := reflect.ValueOf(f)
    vfa := vf.Field(0)
    fmt.Println(vf.CanAddr(), vfa.CanAddr(), vf.Interface())

    // However, here f.A is addressable because it was accessed via a
    // pointer to the struct.  Notice how vf2e below is basically the same
    // as vf above, but it is addressable.  As a result, the address can
    // be taken to invoke the stringer via the pointer receiver.
    vf2 := reflect.ValueOf(&f)
    vf2e := vf2.Elem()
    vf2a := vf2e.Field(0)
    fmt.Println(vf2e.CanAddr(), vf2a.CanAddr(), vf2e.Addr().Interface())
}

It's been quite a while since I wrote all of this, but iirc, it needs to be a copy because there needs to be an instance created in order to invoke stringers/errors on types which are nil (i.e. (*foo)(nil)). Aside from that case, I think it would be possible to modify the reflect.Value in place to unset the read-only flag and set the addressable flag, because it's important that the reflect value is true for both CanAddr() and CanInterface(). However, I don't see much benefit to changing it now since the current method is extremely well-tested.

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 1, 2014

Thanks for the detailed clarification and explanation!

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 10, 2014

Hmm, I think I'm seeing a regression in my code that only happens on 1.3.3 when I switch to this updated code... But this is really tricky to investigate, sorry. It might be me doing something wrong elsewhere.

This comment has been minimized.

Copy link
@davecgh

davecgh Nov 10, 2014

Author Owner

I'd be interested to hear about it if it turns out to be something wrong in my code, but I did test it under what I believe are all of the possible scenarios:

before commit ecccf07e7f9d
after commit ecccf07e7f9d, but before commit 82f48826c6c7
after commit 82f48826c6c7, but before commit 90a7c3c86944
after commit 90a7c3c86944

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 10, 2014

What I'm seeing, on a high level, is this:

Using the unsafeReflectValue from the previous commit (without 1.4 support) produces expected results. But switching to the latest unsafeReflectValue results in slightly different output in one case. But using that alternative approach of modifying the value in place produces same expected results.

It might be a mistake in how I'm using the new unsafeReflectValue, or... another issue with the interaction of unsafe/complex features, so I'm trying to drill down to a smaller reproduce case...

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 10, 2014

Ok, thankfully I was able to figure out a minimal reproduce case.

At this point, it absolutely must be either my mistake of copying/pasting code incorrectly, or there's a bug/deviation in behavior in the new code:

package main

import (
    "fmt"
    "reflect"

    bypass_new "github.com/shurcooL/go-goon/bypass"
    bypass_alt "github.com/shurcooL/go-goon/bypass_alt"
    bypass_prev "github.com/shurcooL/go-goon/bypass_prev"
)

func main() {
    unexportedFuncStruct := struct {
        unexportedFunc func() string
    }{func() string { return "This is the source of an unexported struct field." }}

    var v reflect.Value = reflect.ValueOf(unexportedFuncStruct)

    if v.Kind() != reflect.Struct {
        panic("v.Kind() != reflect.Struct")
    }

    if v.NumField() != 1 {
        panic("v.NumField() != 1")
    }

    v = v.Field(0)

    if v.Kind() != reflect.Func {
        panic("v.Kind() != reflect.Func")
    }

    if v.CanInterface() != false {
        panic("v.CanInterface() != false")
    }

    fmt.Println("pointer value with previous (1.3 only) code:          ", bypass_prev.UnsafeReflectValue(v).Pointer())
    fmt.Println("pointer value with alternative (modify in-place) code:", bypass_alt.UnsafeReflectValue(v).Pointer())
    fmt.Println("pointer value with new code:                          ", bypass_new.UnsafeReflectValue(v).Pointer())
}

My output with go version go1.3.3 darwin/amd64:

pointer value with previous (1.3 only) code:           10064
pointer value with alternative (modify in-place) code: 10064
pointer value with new code:                           0

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 10, 2014

I've pushed the bypass and bypass_prev packages on a go14_support branch of go-goon.

https://github.com/shurcooL/go-goon/commits/go14_support

I'm gonna try to double check I didn't make any copy/paste errors.

Let me know if you can or cannot reproduce.

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 10, 2014

Just verified that my new bypass code completely matches yours, and that seems to be the case.

Note that the "previous" version is from a much older commit, before 65ca732.

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 13, 2014

@davecgh, are you able to reproduce the above? Any thoughts?

This comment has been minimized.

Copy link
@davecgh

davecgh Nov 14, 2014

Author Owner

I haven't had a chance to try it yet. I should be able to take a closer look this weekend.

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 14, 2014

All right, thanks for letting me know! Looking forward to that.

This comment has been minimized.

Copy link
@davecgh

davecgh Nov 15, 2014

Author Owner

Ok. I reproduced this and tracked it down a bit further. This isn't really a problem with the new code as it existed with the previous code which added support for the scalar field as well. The issue appears to be that a function is already a reference type and thus stored in the ptr field, but it isn't marked with flagIndir unlike others. Consequently, the code incorrectly assumes that anything which does not have flagIndir set is kept in the scalar field. So, this is only on issue after golang commit ecccf07e7f9d and before golang commit 82f48826c6c7.

I can come up with a fix that examines the type to react accordingly for the reference types which don't set flagIndir, but I think I'm going to see about making the bypass method work as it might be less risky in the face of future changes. I suspect there are some issues with that approach though such as the inability to properly invoke pointer receiver interface methods (e.g. String and Error) for cases like foo := (*type)(nil) since you can't take the address of nil, whereas the current method actually turns that into a real instance which points to nil and invokes the interface method on that.

EDIT: I need to dig some more to be sure, but I think this doesn't affect spew because of the restricted cases in which it's called. For example, I believe spew always dereferences pointers and unpacks interfaces/reference types, before hitting code that calls unsafeReflectValue.

This comment has been minimized.

Copy link
@davecgh

davecgh Nov 15, 2014

Author Owner

Alright, so I confirmed simply modifying the reflect.Value in place doesn't work well when you need addressable values as you can't simply set the flagAddr bit because that leaves the ptr field as 0 which leads to returning nil instead of the real address.

However, I have a fix for the existing code and will commit it later. I need to see if test cases need to be added or not. I believe they won't be for the most part, since, as I already said, spew doesn't invoke unsafeReflectValue with the conditions that trigger what you're seeing.

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 15, 2014

Thanks a lot for looking into this! I need to figure this out to close shurcooL-legacy/Conception-go#6.

Alright, so I confirmed simply modifying the reflect.Value in place doesn't work well when you need addressable values as you can't simply set the flagAddr bit because that leaves the ptr field as 0 which leads to returning nil instead of the real address.

Can you share a test case that demonstrates that?

I see that your use of unsafeReflectValue in spew is slightly different from mine in goon, which is why I'm seeing a problem that your tests do not catch (namely, this go-goon test fails).

I think it's possible for me to fix the issue by using the code differently. It seems I can get the Pointer() even for unexported struct fields. Bypass is only needed for getting the Interface(). But my GetSourceAsString func does a reflect.ValueOf(f).Pointer() anyway, so I can have an alternative version that accepts the address and pass Pointer() from goon directly and skips the need to do bypass to get Interface() (see this section).

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 15, 2014

Okay, I did this shurcooL-deprecated/go-goon@46f8e2d, which I think lets me use your well tested current bypass code (which works with 1.3 and 1.4), while fixing my issue.

I will go ahead with that for now, but I will keep the bypass_alt on a separate branch, so once I hear from you about a counter-example of what doesn't work, I'd be curious to try it.

But at least I think I can now move on and consider my original issue closed. Thanks for your help!

This comment has been minimized.

Copy link
@davecgh

davecgh Nov 15, 2014

Author Owner

I'm glad you were able to work around it as well! A change to the code which I'm going to make is as follows. It corrects the issue you're seeing and is technically more accurate even though spew itself doesn't really need it. I have to leave for a bit, but I'll make an example for you that shows why the in-place modification approach has issues later tonight.

diff --git a/spew/common.go b/spew/common.go
index 2cec098..0ce0df1 100644
--- a/spew/common.go
+++ b/spew/common.go
@@ -109,7 +109,19 @@ func unsafeReflectValue(v reflect.Value) (rv reflect.Value) {
                vt = reflect.PtrTo(v.Type())
                indirects++
        } else if offsetScalar != 0 {
-               upv = unsafe.Pointer(uintptr(unsafe.Pointer(&v)) + offsetScalar)
+               // The value is in the scalar field when it's not one of the
+               // reference types.
+               switch vt.Kind() {
+               case reflect.Uintptr:
+               case reflect.Chan:
+               case reflect.Func:
+               case reflect.Map:
+               case reflect.Ptr:
+               case reflect.UnsafePointer:
+               default:
+                       upv = unsafe.Pointer(uintptr(unsafe.Pointer(&v)) +
+                               offsetScalar)
+               }
        }

This comment has been minimized.

Copy link
@davecgh

davecgh Nov 15, 2014

Author Owner

Here is the example of why the in-place modification doesn't work for all cases:

package main

import (
    "fmt"
    "reflect"

    bypass_new "github.com/shurcooL/go-goon/bypass"
    bypass_alt "github.com/shurcooL/go-goon/bypass_alt"
)

type stringer struct {
    a int
}

func (s *stringer) String() string {
    return fmt.Sprintf("%d", s.a)
}

func main() {
    var s stringer
    var v = reflect.ValueOf(s)

    fmt.Println("can address with alternative (modify in-place)?", bypass_alt.UnsafeReflectValue(v).CanAddr())
    fmt.Println("can address with spew code?", bypass_new.UnsafeReflectValue(v).CanAddr())


    // Notice since you can't get the address of it, you can't create a
    // pointer to it in order to invoke the Stringer (String method) on it.
    // Adding additional code to the bypass method to set the `flagAddr`
    // bit does _not_ work because the `ptr` is still nil and it thus
    // causes .Addr() to return a reflect.Value with nil instead of a
    // reflect.Value with the actual address, unlike the spew code.
}

// Output:
// can address with alternative (modify in-place)? false
// can address with spew code? true

This comment has been minimized.

Copy link
@dmitshur

dmitshur Nov 16, 2014

A change to the code which I'm going to make is as follows. It corrects the issue you're seeing and is technically more accurate even though spew itself doesn't really need it.

That's awesome! I understand that spew doesn't make use of it, but it's still nice to have it be more accurate... Especially for situations when people like me come along and make an exported version of the code, hehe. (To be fair, I think this code is excellent and well tested, so I always point other people who need to deal with this functionality to come here. So I think having it be more accurate is beneficial to more people than just me.)

Before that change, my slightly modified test program was giving this output:

pointer value with no code:                            10352
pointer value with previous (1.3 only) code:           10352
pointer value with alternative (modify in-place) code: 10352
pointer value with new code:                           0

But with the change you proposed in the diff, it becomes:

pointer value with no code:                            10352
pointer value with previous (1.3 only) code:           10352
pointer value with alternative (modify in-place) code: 10352
pointer value with new code:                           10352

So I will definitely say LGTM for making that change. Thanks!

Also, I appreciate the insight on what the difference was, as it raises my confidence about this.

Expand Down
6 changes: 0 additions & 6 deletions spew/internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,6 @@ func TestInvalidReflectValue(t *testing.T) {
}
}

// flagRO, flagKindShift and flagKindWidth indicate various bit flags that the
// reflect package uses internally to track kind and state information.
const flagRO = 1 << 0
const flagKindShift = 4
const flagKindWidth = 5

// changeKind uses unsafe to intentionally change the kind of a reflect.Value to
// the maximum kind value which does not exist. This is needed to test the
// fallback code which punts to the standard fmt library for new types that
Expand Down

2 comments on commit 1288542

@dmitshur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work!

This works in go1.4beta1. I will apply the same change to my (exported) code for this functionality if you don't have objections.

@davecgh
Copy link
Owner Author

@davecgh davecgh commented on 1288542 Nov 1, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have no objections as I'm sure you already have the license anyways.

Please sign in to comment.