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
all: use reflect.StringHeader and reflect.SliceHeader for pointer conversions #37805
Comments
Thanks. Confirmed, those are all misuses of unsafe.Pointer. |
Don't most of these fall under unsafe rule (1), and the uintptr ones fall under managing syscall stuff directly? |
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. :/ |
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. |
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. |
@smasher164 linkname only works for functions and variables; not types. |
Right, so we would need a runtime function similar to |
Seems like |
(More supporting evidence for #19367? CC @ianlancetaylor @griesemer) |
Change https://golang.org/cl/230557 mentions this issue: |
Given the comments regarding the
|
Yeah, I forgot that package syscall can't import package reflect, and that x/sys probably doesn't want to either. Using the The two instances in cmd should be reasonable for package reflect though. |
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? Ian mentioned that But for the case in syscall_unix's |
Since the old linker is moved, I've edited the description to point the second instance in cmd to |
The correct value of
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) |
Okay thanks for the clarification. I'll take another pass tonight. For the cases in |
I don't think you can use |
Another option might be to define a separate struct, and rely on a regression test to ensure that its layout remains compatible with For example, perhaps something like: https://play.golang.org/p/13IbIZOGFFl |
Change https://golang.org/cl/231177 mentions this issue: |
The regression test included in this change verifies that the type is, in fact, equivalent, while allowing the actual header definitions to avoid importing the (relatively heavy) "reflect" package itself. This change is loosely based on Keyan Pishdadian's draft in CL 230557. For golang/go#37805 Change-Id: I998c69cdeb852154cd66ab5fdaa542a6f19666a2 Reviewed-on: https://go-review.googlesource.com/c/sys/+/231177 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Change https://golang.org/cl/231223 mentions this issue: |
@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! |
…rations into an internal package The new package "internal/unsafeheader" depends only on "unsafe", and provides declarations equivalent to reflect.StringHeader and reflect.SliceHeader but with Data fields of the proper unsafe.Pointer type (instead of uintptr). Unlike the types it replaces, the "internal/unsafeheader" package has a regression test to ensure that its header types remain equivalent to the declarations provided by the "reflect" package. Since "internal/unsafeheader" has almost no dependencies, it can be used in other low-level packages such as "syscall" and "reflect". This change is based on the corresponding x/sys change in CL 231177. Fixes golang#37805 Updates golang#19367 Change-Id: I7a6d93ef8dd6e235bcab94e7c47270aad047af31 Reviewed-on: https://go-review.googlesource.com/c/go/+/231223 Reviewed-by: Ian Lance Taylor <iant@golang.org>
As @mdempsky states in #19367 (comment),
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.
The text was updated successfully, but these errors were encountered: