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

runtime: pinner.Pin doesn't panic when it says it will in Go 1.21 #63768

Open
bcmills opened this issue Oct 27, 2023 · 8 comments
Open

runtime: pinner.Pin doesn't panic when it says it will in Go 1.21 #63768

bcmills opened this issue Oct 27, 2023 · 8 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bcmills
Copy link
Member

bcmills commented Oct 27, 2023

The documentation for runtime.Pinner.Pin currently says:

The argument must be a pointer of any type or an unsafe.Pointer. It must be the result of calling new, taking the address of a composite literal, or taking the address of a local variable. If one of these conditions is not met, Pin will panic.

However, it does not always do so today.

A channel, map, or slice value can be constructed by calling make, which does not involve a call to new, nor a composite literal, nor the address of a local variable. Empirically, Pin does not panic when called on such an object (https://go.dev/play/p/q_iyNHJduRf).

	p := new(runtime.Pinner)
	c := make(chan struct{}, 1)
	p.Pin(reflect.ValueOf(c).UnsafePointer())

It isn't clear to me whether this is a mistake in the documentation (a valid case that was omitted from the doc comment), or a bug in the implementation (Pin failing to panic for an unsupported address).

At a higher level, it seems to me that it is basically always a mistake for a doc comment to state that something will panic for an unsupported input, since that makes it an incompatible change to ever add support for such an input. (It would be better for Pin not to promise anything about what happens for such an input, but to panic anyway as a debugging aid to the user.)

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 27, 2023
@bcmills
Copy link
Member Author

bcmills commented Oct 27, 2023

This was prompted by the desire to pin channel values in https://go.dev/cl/528796.

Note that channels are, to my knowledge, the only Go type that cannot be allocated by either calling new or taking the address of a composite literal or local variable (compare #28366).

@dr2chase
Copy link
Contributor

It reads like an oversight in the documentation, though it is also unclear to me what C code would do with a pinned channel or map.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 27, 2023
@dr2chase
Copy link
Contributor

CC @mknyszek especially.

@qiulaidongfeng
Copy link
Contributor

See #62356 and CL 527156,After CL 527156, the documentation becomes:
The argument must be a pointer of any type or an unsafe.Pointer. It's safe to call Pin on non-Go pointers, in which case Pin will do nothing.

@bcmills
Copy link
Member Author

bcmills commented Oct 30, 2023

Huh! Yeah, it looks like this is probably fixed at head as a result of #62356. 🙃

@bcmills
Copy link
Member Author

bcmills commented Oct 30, 2023

In that case, I guess this is a bug affecting only Go 1.21. (Leaving open for the runtime folks to decide whether it's worth backporting some kind of fix for compatibility.)

@bcmills bcmills changed the title runtime: pinner.Pin doesn't panic when it says it will runtime: pinner.Pin doesn't panic when it says it will in Go 1.21 Oct 30, 2023
@randall77
Copy link
Contributor

We only usually backport fixes for regressions, so I'm inclined to leave this one be.
Pin will panic less in 1.22 than in 1.21, but that's not so bad. It isn't like Pin in 1.21 panics more than 1.20 (because Pin doesn't exist in 1.20).

@qiulaidongfeng
Copy link
Contributor

Perhaps this is a go1.21 documention issue that should be written as

The argument must be a pointer of any type or an unsafe.Pointer.

@mknyszek mknyszek self-assigned this Nov 8, 2023
@mknyszek mknyszek added this to the Backlog milestone Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: Todo
Development

No branches or pull requests

6 participants