cmd/compile: replace //go:notinheap with runtime/internal/sys.NotInHeap
#46731
Comments
|
This proposal has been added to the active column of the proposals project |
|
This is undocumented, so it is probably OK to remove. |
|
One question that I didn't see discussed: It might be useful to support notinheap data structures not only for the runtime. Management of mmaped data or data in sysV shared memory comes to mind. The user current has no way in deeply pointer heavy code to detect that she got that right. It has been solved beautifully via notinheap for runtime purposes and it might be time to support it for a bit wider scope. |
This is ok by me. Does it fit cleanly into cmd/go's model to have internal packages that user packages can't directly import, but that the cgo-rewritten code can access?
I think this is an orthogonal concern. I'm more concerned here that type directives are very difficult to implement correctly, as evidenced by how many corner cases are still mishandled by cmd/compile. It's a secondary concern that we accidentally exposed the feature to end users. If we decide to intentionally export a NotInHeap type for users though, I think that's fine. But that should probably be a separate proposal. |
I don't think it does. I think we would end up in a weird state in which packages that |
|
I think it would be straightforward for cmd/go to pass a special option to the compiler when compiling Go files generated by cgo. Then we could use a special builtin Alternatively, if we put the type into runtime/cgo we should consider @nightlyone 's suggestion and pick a name ad documentation that permits anybody to use the new type if they so desire. I think this is a fairly specialized use case and I wouldn't ordinary argue in favor of supporting that use case, but if we need it anyhow for cgo then perhaps it will do little harm to let other people use it as well. |
|
If cgo supports it at all, then it seems to me that there is a trivial way for everyone else to use it when cgo is enabled too: package clever
/*
typedef struct Incomplete incomplete;
*/
import "C"
type NotInHeap {
_ C.incomplete
}So I'm not sure that it's worth going to a lot of effort to prevent external users from making their own not-in-Go-heap types. |
|
Good point. |
|
There's still a distinction of what semantics we're exporting. The semantics of So I'm not convinced by that particular argument that we should export this type. |
|
FWIW, the reason I suggested |
|
We discussed this in the runtime/compiler meeting today (sorry you weren't there @mdempsky !) and everyone's fine with the general idea of replacing the type pragma with an embedded type. I'd like to see what the current non-struct notinheap types wind up looking like, but that's a minor concern (@mknyszek thinks they'd probably be better off as struct types anyway). We realized we were all pretty unclear on why exactly mmap-using user code would want something like notinheap. We use it in the runtime for types that must not have write barriers, but (performance aside) there's no such restriction in user code. (FWIW, some history of go:notinheap: There are parts of the runtime, like the scheduler, that need to manipulate pointer-based data structures but cannot have write barriers because write barriers depend on resources that don't exist in these core parts of the runtime. We had used unsafe casting shenanigans to carefully avoid write barriers in this code, but I felt that was getting too annoying and unmaintainable, so I added go:notinheap as an expedient solution for a small number of core types in the runtime. It was kind of messy and incomplete, but it was still an improvement over the status quo, and since it was only for internal use it didn't have to be 100% complete. But from there it sort of grew, especially when Keith realized it had almost the exact semantics we needed to solve a problem in cgo. And now it's been through a bunch of awkward contortions and I'm happy someone is rethinking it. :) ) |
|
Will record this as "discussion ongoing", but it also sounds like a prototype is needed before making a decision. If that will take a while we can also move this to Proposal-Hold until the prototype is ready. |
|
At least for my minor concern, I don't need to see a whole prototype, I just wanted confirmation that it's not a problem to convert the non-struct types to struct types (which would probably only take a few minutes). I don't think we should expose a user-visible type for this at this time. Maybe we will in the future, but I think that's outside the scope of this proposal. For |
|
@mdempsky do you want to try a prototype and see whether it works in practice? |
|
Change https://golang.org/cl/345093 mentions this issue: |
|
Change https://golang.org/cl/345089 mentions this issue: |
|
Change https://golang.org/cl/345094 mentions this issue: |
Done. For the cgo CL, I used runtime/cgo.Incomplete as initially proposed above. This is necessary because https://github.com/golang/go/blob/master/misc/cgo/test/testdata/issue41761.go tests that we can convert between For the compiler CL, I just did a quick hack to lock down visibility. It fails a couple compiler unit tests, but I'm sure we can fix those somehow. |
See previously #21967. |
|
I was following this and was interested in where this might be used outside the runtime, so did a sourcegraph query to do a survey. A few cases stuck out:
The other cases seem to be misuses, but I personally don't know enough to say. |
|
Based on the discussion above, this proposal seems like a likely accept. |
|
No change in consensus, so accepted. 🎉 |
Same as CL 421880, but for test directory. Updates #46731 Change-Id: If8d18df013a6833adcbd40acc1a721bbc23ca6b2 Reviewed-on: https://go-review.googlesource.com/c/go/+/421881 Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Heschi Kreinick <[email protected]> Run-TryBot: Cuong Manh Le <[email protected]> Reviewed-by: Keith Randall <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
|
Change https://go.dev/cl/427394 mentions this issue: |
Since when go/types,types2 do not know about build constraints, and runtime/cgo.Incomplete is only available on platforms that support cgo. These tests are also failing on aix with failure from linker, so disable them on aix to make builder green. The fix for aix is tracked in #54814 Updates #46731 Updates #54814 Change-Id: I5d6f6e29a8196efc6c457ea64525350fc6b20309 Reviewed-on: https://go-review.googlesource.com/c/go/+/427394 Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Bryan Mills <[email protected]> Auto-Submit: Cuong Manh Le <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Cuong Manh Le <[email protected]>
|
Change https://go.dev/cl/427142 mentions this issue: |
So they can be added to ignored list, since the tests now require cgo.Incomplete, which is not recognized by go/types and types2. Updates #46731 Change-Id: I9f24e3c8605424d1f5f42ae4409437198f4c1326 Reviewed-on: https://go-review.googlesource.com/c/go/+/427142 Run-TryBot: Cuong Manh Le <[email protected]> Reviewed-by: Bryan Mills <[email protected]> Auto-Submit: Cuong Manh Le <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Heschi Kreinick <[email protected]>
So next CL can get rid of go:notinheap pragma. Updates #46731 Change-Id: Ib2e2f2d381767e11cec10f76261b516188ddaa6a Reviewed-on: https://go-review.googlesource.com/c/go/+/422814 Run-TryBot: Cuong Manh Le <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: David Chase <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
Updates #46731 Change-Id: I247fa9c7ca97feb9053665da7ff56e7f5b571f74 Reviewed-on: https://go-review.googlesource.com/c/go/+/422815 Reviewed-by: Keith Randall <[email protected]> Run-TryBot: Cuong Manh Le <[email protected]> Reviewed-by: David Chase <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Joedian Reid <[email protected]>
|
Change https://go.dev/cl/432338 mentions this issue: |
This ports https://go.dev/cl/421879 to libgo. This is a quick port to update gofrontend to work with the version of cgo in gc mainline. A more complete port will follow, changing the gc version of cmd/cgo to choose an approach based on feature testing the gccgo in use. Updates golang/go#46731 Change-Id: I61d1f97781ac1aeb5f8a51ddeb1db378d8c97f95 Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/432338 Reviewed-by: Than McIntosh <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
This ports https://go.dev/cl/421879 to libgo. This is a quick port to update gofrontend to work with the version of cgo in gc mainline. A more complete port will follow, changing the gc version of cmd/cgo to choose an approach based on feature testing the gccgo in use. Updates golang/go#46731 Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/432338
This ports https://go.dev/cl/421879 to libgo. This is a quick port to update gofrontend to work with the version of cgo in gc mainline. A more complete port will follow, changing the gc version of cmd/cgo to choose an approach based on feature testing the gccgo in use. Updates golang/go#46731 Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/432338
|
Change https://go.dev/cl/446260 mentions this issue: |
…lete Test whether gccgo/GoLLVM supports cgo.Incomplete. If it doesn't, use a local definition rather than importing it. Roll back 426496, which skipped a gccgo test, as it now works. For #46731 Fixes #54761 Change-Id: I8bb2ad84c317094495405e178bf5c9694f82af56 Reviewed-on: https://go-review.googlesource.com/c/go/+/446260 Reviewed-by: Bryan Mills <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> Auto-Submit: Ian Lance Taylor <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Run-TryBot: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
…lete Test whether gccgo/GoLLVM supports cgo.Incomplete. If it doesn't, use a local definition rather than importing it. Roll back 426496, which skipped a gccgo test, as it now works. For golang#46731 Fixes golang#54761 Change-Id: I8bb2ad84c317094495405e178bf5c9694f82af56 Reviewed-on: https://go-review.googlesource.com/c/go/+/446260 Reviewed-by: Bryan Mills <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> Auto-Submit: Ian Lance Taylor <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Run-TryBot: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
|
Change https://go.dev/cl/453556 mentions this issue: |
Updates #46731 Change-Id: Ie64e87d759c48642582342d221b24f77bb81d47a Reviewed-on: https://go-review.googlesource.com/c/go/+/453556 Auto-Submit: Cuong Manh Le <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Cuong Manh Le <[email protected]> Reviewed-by: Russ Cox <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
|
Change https://go.dev/cl/455279 mentions this issue: |
[Re-land of CL 424854, which was reverted as CL 425214.] When handling a type declaration like: ``` type B A ``` unified IR has been writing out that B's underlying type is A, rather than the underlying type of A. This is a bit awkward to implement and adds complexity to importers, who need to handle resolving the underlying type themselves. But it was necessary to handle when A was declared like: ``` //go:notinheap type A int ``` Because we expected A's not-in-heap'ness to be conferred to B, which required knowing that A was on the path from B to its actual underlying type int. However, since #46731 was accepted, we no longer need to support this case. Instead we can write out B's actual underlying type. One stumbling point though is the existing code for exporting interfaces doesn't work for the underlying type of `comparable`, which is now needed to implement `type C comparable`. As a bit of a hack, we we instead export its underlying type as `interface{ comparable }`. Fixes #54512. Change-Id: I9aa087e0a277527003195ebc7f4fbba6922e788c Reviewed-on: https://go-review.googlesource.com/c/go/+/455279 Run-TryBot: Matthew Dempsky <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Robert Griesemer <[email protected]>
Simplify the handling of pragmas in type declarations within noder/writer.go. Remove redundant checks by calling checkPragmas once at the beginning of case *syntax.TypeDecl and eliminate unnecessary else block. Also, ensure unique ID assignment for function-scoped defined types is only performed when n.Alias is false. Fixes a redundancy issue where pragma checks were performed inside both branches of an if-else statement unnecessarily. Update golang#46731
|
Change https://go.dev/cl/541739 mentions this issue: |
//go:notinheapis the only current type pragma, and it imposes a lot of complexity and special cases on the compiler and tools. E.g., it's not captured within the go/types or types2 type models, so users wanting to analyze code that uses it have to do a lot of effort to reconstruct this information. The reflect API wasn't updated to reject ChanOf or MapOf for notinheap types. Even the existing compiler typechecker that was extended to natively support//go:notinheapfails to handle tricky situations: https://play.golang.org/p/hYGrwJx37TN//go:notinheapis effectively a language change, but it bypassed review by the usual language spec reviewers because it started out as a runtime-only hack. It was then extended to be used by cmd/cgo and be accessible to user packages (as a necessity of cmd/cgo's design), but again skipped language review.We already have what are effectively type pragmas in the form of "noCompare" (where embedding a non-comparable, zero-size type makes the enclosing struct non-comparable too) and "noCopy" (where cmd/vet can detect value copies of types that directly contain an element of type "noCopy"). However, they work much more robustly and interoperate with existing tooling better because they rely on struct field embedding, rather than introducing actual type pragmas. I think
//go:notinheapshould be modified to work similarly.I propose the following:
Add a new
NotInHeaptype to runtime/internal/sys. Its initial definition would be:Disallow all other use of
//go:notinheap, and maybe eventually get rid of it altogether by turningsys.NotInHeapinto a compiler intrinsic type likeunsafe.Pointer.Change existing runtime types that use
//go:notinheapto add asys.NotInHeap-typed field instead. The handful of defined types that are marked//go:notinheapbut aren't already a struct type (i.e., debugLogBuf, checkmarksMap, gcBits) would need to be adjusted to use a struct wrapper.Add a new
runtime/cgo.Incompletetype with the definition:type Incomplete struct { _ sys.NotInHeap }. Change cmd/cgo to emittype _Ctype_struct_foo cgo.Incompleteinstead of usingstruct{}with a//go:notinheapdirective.Disallow the reintroduction of type pragmas, even for runtime use, without proposal review. (I have no objection to runtime-internal intrinsic types though.)
Incidentally, this is also the approach I took in implementing https://go-review.googlesource.com/c/go/+/308971 as a proof of concept of #19057 (adding intrinsic types to allow adjusting field alignment) for use within the runtime. I think it worked cleanly, and it avoided requiring the introduction of new type pragmas. Instead, it added
runtime/internal/align.elemTas an intrinsic type known to the compiler with special alignment semantics.The text was updated successfully, but these errors were encountered: