Go: all: use reflect.StringHeader and reflect.SliceHeader for pointer conversions

[smasher164]

As @mdempsky states in https://github.com/golang/go/issues/19367#issuecomment-597807805,

It's not guaranteed that you can convert a pointer to a slice to a pointer to any other struct type except reflect.SliceHeader.

Therefore, the following conversions in the golang subrepos (except runtime and reflect) should be modified to use reflect.StringHeader and reflect.SliceHeader when converting between an unsafe.Pointer and a string/slice.

• syscall/syscall_unix.go
• x/sys/unix/syscall_unix.go
• x/sys/unix/syscall_darwin.1_13.go
• cmd/internal/goobj2/objfile.go
• cmd/oldlink/internal/objfile/objfile.go

[mdempsky]

Thanks. Confirmed, those are all misuses of unsafe.Pointer.

[twmb]

Don't most of these fall under unsafe rule (1), and the uintptr ones fall under managing syscall stuff directly?

[mdempsky]

Yes, the last three examples are safe today (on gc and gccgo, at least) under rule 1, but might not be safe in the future. As reflect.SliceHeader's docs warn: "its representation may change in a later release." Using reflect.SliceHeader is the future proof solution.

The first two don't involve direct uses of syscall.Syscall, so rule 4 is inapplicable.

But that does make me note that the first example is within package syscall, which isn't allowed to depend on package reflect. :/

[twmb]

I thought I remembered a proposal about adding equivalent unsafe.StringHeader and unsafe.SliceHeader types, but I can't find it. But, that idea may be worthwhile to address the syscall issue, as well as to allow users of just StringHeader/SliceHeader to avoid importing the reflect package.

[smasher164]

Can't syscall access runtime.slice instead of reflect.SliceHeader? It already linknames in runtime functions.

Edit: @twmb You are referring to #19367 which forked off into this issue to fix pre-existing conversions.

[mdempsky]

@smasher164 linkname only works for functions and variables; not types.

[smasher164]

@smasher164 linkname only works for functions and variables; not types.

Right, so we would need a runtime function similar to func newslice() unsafe.Pointer that syscall would call to turn it into *[]byte. Although, that might be too heavyweight of an addition to support one package.

[bcmills]

Seems like syscall could use the usual “convert to *[reallyBig]byte and then slice down to size” pattern, but for portability we'll presumably need build constraints for the definition of reallyBig (see https://github.com/golang/go/issues/13656#issuecomment-165858089).

[bcmills]

(More supporting evidence for #19367? CC @ianlancetaylor @griesemer)

[gopherbot]

Change https://golang.org/cl/230557 mentions this issue: x/sys: use reflect.SliceHeader for ptr conversion

[tklauser]

Given the comments regarding the syscall package, I think we also want to avoid importing reflect in x/sys/unix for which https://golang.org/cl/230557 was sent. Copying my comment from there:

[...] I'd really like to avoid importing the quite heavy-weight reflect package just for the reflect.SliceHeader type. The issue also mentions special constraints regarding the syscall package. Could we maybe use something like proposed by Bryan in https://github.com/golang/go/issues/37805#issuecomment-598241456 for this package as well, given it's closely related to package syscall?

[mdempsky]

Yeah, I forgot that package syscall can't import package reflect, and that x/sys probably doesn't want to either. Using the (*[big]T)[:x:x] idiom seems reasonable in those cases.

The two instances in cmd should be reasonable for package reflect though.

[keyan]

After reading the linked discussion in https://github.com/golang/go/issues/13656, it still isn't clear to me if the technique explained there and described in https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices is platform independent?

Ian mentioned that big == 50 is safe? https://github.com/golang/go/issues/13656#issuecomment-291957684

But for the case in syscall_unix's Mmap we are mapping up to length int bytes so on 64-bit systems isn't it possible 1<<50 is not large enough?

[smasher164]

The two instances in cmd should be reasonable for package reflect though.

Since the old linker is moved, I've edited the description to point the second instance in cmd to cmd/oldlink/internal/objfile.

[mdempsky]

After reading the linked discussion in #13656, it still isn't clear to me if the technique explained there and described in https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices is platform independent?

The correct value of big is unfortunately platform-dependent. See the value of arch.MAXWIDTH in cmd/compile/internal/$ARCH/galign.go. (You then have to divide down by sizeof(T).)

But for the case in syscall_unix's Mmap we are mapping up to length int bytes so on 64-bit systems isn't it possible 1<<50 is not large enough?

The amd64 ISA supports 64-bit pointers, but today's implementations (i.e., CPUs) only support 48-bits of addressable page tables.

Edit: It seems like this is no longer true: "The 5-level paging scheme supports a Linear Address space up to 57 bits and a physical address space up to 52 bits". https://en.wikipedia.org/wiki/Ice_Lake_(microprocessor)

[keyan]

Okay thanks for the clarification. I'll take another pass tonight.

For the cases in cmd/internal and cmd/oldlink using MAXWIDTH seems straightforward as the package global thearch can be used. Not sure how/if architecture can be identified for the open CL I have in sys, or how/if build directives can be used to accomplish this (as Bryan noted above).

[mdempsky]

I don't think you can use MAXWIDTH as it's a variable, not a constant. It's also the maximum width of the target architecture, whereas for the array type expressions you need the maximum width of the host architecture.

[bcmills]

Another option might be to define a separate struct, and rely on a regression test to ensure that its layout remains compatible with reflect.SliceHeader. That way only the test itself needs to depend on reflect, but we don't need a bunch of platform-specific build-constraint shenanigans to make it portable.

For example, perhaps something like: https://play.golang.org/p/13IbIZOGFFl
(unpack locally using txtar -x).

[mdempsky]

@bcmills Thanks, I hate it.

But seriously, that seems to me like it should work, and it's probably the least invasive solution while we wait for #19367 and/or generics.

[gopherbot]

Change https://golang.org/cl/231177 mentions this issue: unix: use a type equivalent to reflect.SliceHeader to manipulate slices

[gopherbot]

Change https://golang.org/cl/231223 mentions this issue: internal/unsafeheader: consolidate stringHeader and sliceHeader declarations into an internal package

[keyan]

@bcmills Thanks for all the suggestions. Unfortunately this felt a little over my head for my second issue, but reading your CL has been very educational. Thanks!