Skip to content
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

cmd/compile: replace //go:notinheap with runtime/internal/sys.NotInHeap #46731

Open
mdempsky opened this issue Jun 13, 2021 · 22 comments
Open

cmd/compile: replace //go:notinheap with runtime/internal/sys.NotInHeap #46731

mdempsky opened this issue Jun 13, 2021 · 22 comments

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 13, 2021

//go:notinheap is 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:notinheap fails to handle tricky situations: https://play.golang.org/p/hYGrwJx37TN

//go:notinheap is 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:notinheap should be modified to work similarly.

I propose the following:

  1. Add a new NotInHeap type to runtime/internal/sys. Its initial definition would be:

    type NotInHeap struct { _ nih }
    
    //go:notinheap
    type nih struct{}
    
  2. Disallow all other use of //go:notinheap, and maybe eventually get rid of it altogether by turning sys.NotInHeap into a compiler intrinsic type like unsafe.Pointer.

  3. Change existing runtime types that use //go:notinheap to add a sys.NotInHeap-typed field instead. The handful of defined types that are marked //go:notinheap but aren't already a struct type (i.e., debugLogBuf, checkmarksMap, gcBits) would need to be adjusted to use a struct wrapper.

  4. Add a new runtime/cgo.Incomplete type with the definition: type Incomplete struct { _ sys.NotInHeap }. Change cmd/cgo to emit type _Ctype_struct_foo cgo.Incomplete instead of using struct{} with a //go:notinheap directive.

  5. 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.elemT as an intrinsic type known to the compiler with special alignment semantics.

@gopherbot gopherbot added this to the Proposal milestone Jun 13, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 14, 2021
@rsc rsc moved this from Incoming to Active in Proposals Aug 4, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Aug 4, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

@rsc rsc commented Aug 11, 2021

This is undocumented, so it is probably OK to remove.
If we replace it with internal-only things then no one will be able to see it.
I don't think we need an exported cgo.Incomplete - we can do whatever we need to let cgo see things others can't.
(Maybe an internal/cgo?)

@nightlyone
Copy link
Contributor

@nightlyone nightlyone commented Aug 11, 2021

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.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Aug 11, 2021

I don't think we need an exported cgo.Incomplete - we can do whatever we need to let cgo see things others can't.
(Maybe an internal/cgo?)

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?

One question that I didn't see discussed: It might be useful to support notinheap data structures not only for the runtime.

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.

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 11, 2021

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 don't think it does. I think we would end up in a weird state in which packages that import "C" can mysteriously also import "internal/cgo" without a diagnosed error. That doesn't seem appreciably better to me than exporting a runtime/cgo.Incomplete marker type.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 11, 2021

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 notinheap (or unsafe.Notiheap) that is only available when that flag is passed. We already compile the runtime with special compiler flags, so we could make it available there as well. The downside is that other people can pass the compiler flag as well.

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.

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 11, 2021

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.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 11, 2021

Good point.

@aclements
Copy link
Member

@aclements aclements commented Aug 17, 2021

There's still a distinction of what semantics we're exporting. The semantics of go:notinheap have changed a few times, but since it's internal it hasn't been a problem to just shape it as our needs have changed. Likewise, while @bcmills is right that there's nothing stopping users from using an incomplete type from C to achieve the same effect, the semantics of that are that you're using an incomplete C type. Those happens to overlap in the current implementation, but the intent is different.

So I'm not convinced by that particular argument that we should export this type.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Aug 17, 2021

FWIW, the reason I suggested runtime/cgo.Incomplete initially is so we'd have a name specifically for the semantics of incomplete C types. Immediately, this would be in terms of NotInHeap, because this is how we implement them today; but I think longer term we probably should distinguish incomplete C types by not allowing them in any context that depends on their size (e.g., unsafe.Sizeof, struct field types, array element types).

@aclements
Copy link
Member

@aclements aclements commented Aug 17, 2021

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. :) )

@rsc
Copy link
Contributor

@rsc rsc commented Aug 18, 2021

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.

@aclements
Copy link
Member

@aclements aclements commented Aug 18, 2021

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 cgo.Incomplete, it would be nice if we could find a way to not export that, but at least Ian and I are fine with just making that an exported type in runtime/cgo.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 25, 2021

@mdempsky do you want to try a prototype and see whether it works in practice?

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345093 mentions this issue: cmd/cgo: add and use runtime/cgo.Incomplete instead of //go:notinheap

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345089 mentions this issue: runtime: add and use runtime/internal/sys.NotInHeap instead of //go:notinheap

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345094 mentions this issue: cmd/compile: restrict //go:notinheap to runtime/internal/sys

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Aug 25, 2021

@mdempsky do you want to try a prototype and see whether it works in practice?

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 *C.struct_S across packages, so they have to refer to a common standard library type otherwise the _ notinheap field makes the struct types different (because _ isn't exported). Using "runtime/cgo.Incomplete" was the easiest way to do this, but I'm open to using a different name that we suppress from use by end users. (But as discussed above, I'm not sure there's any benefit in hiding it from users either; they can get basically the same time by just using cgo normally.)

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.

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 25, 2021

they have to refer to a common standard library type otherwise the _ notinheap field makes the struct types different (because _ isn't exported)

See previously #21967.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Sep 1, 2021

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.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 8, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals Sep 8, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals Sep 15, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Sep 15, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: cmd/compile: replace //go:notinheap with runtime/internal/sys.NotInHeap cmd/compile: replace //go:notinheap with runtime/internal/sys.NotInHeap Sep 15, 2021
@rsc rsc removed this from the Proposal milestone Sep 15, 2021
@rsc rsc added this to the Backlog milestone Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants