Skip to content

syscall: SyscallN always escapes the variadic argument [1.23 backport] #70202

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.

Sign up for GitHub

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

Jump to bottom
Closed
gopherbot opened this issue Nov 5, 2024 · 7 comments
Closed

syscall: SyscallN always escapes the variadic argument [1.23 backport] #70202

gopherbot opened this issue Nov 5, 2024 · 7 comments
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Go1.23.4

Comments

@gopherbot
Copy link
Contributor

@qmuntal requested issue #70197 to be considered for backport to the next 1.23 minor release.

@gopherbot please open the backport tracking issues. syscall.SyscallN alternatives are deprecated, and there is the risk of users switching back to them if syscall.SyscallN is less performant.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Nov 5, 2024
@gopherbot gopherbot mentioned this issue Nov 5, 2024
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 5, 2024
@gopherbot gopherbot added this to the Go1.23.3 milestone Nov 5, 2024
@ianlancetaylor
Copy link
Contributor

Also I'll note that it would be nice to update x/sys/windows to use the newer more efficient SyscallN (this was actually done and then reverted), and that people still use older (but still supported) versions of Go to compile new versions of x/sys/windows. Backporting the fix (which is small) will avoid tripping people up.

@dr2chase
Copy link
Contributor

dr2chase commented Nov 6, 2024

We decided not to backport because it isn't a regression, and the problem will be solved with the 1.24 release.

@dr2chase dr2chase closed this as completed Nov 6, 2024
@ianlancetaylor
Copy link
Contributor

But it will become a regression as soon as we change x/sys/windows.

@dr2chase
Copy link
Contributor

dr2chase commented Nov 6, 2024

Revisit next week, then.

@dr2chase dr2chase reopened this Nov 6, 2024
@gopherbot gopherbot modified the milestones: Go1.23.3, Go1.23.4 Nov 6, 2024
@dmitshur dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Nov 20, 2024
@cherrymui
Copy link
Member

cherrymui commented Nov 20, 2024 •
edited by gabyhelp
Loading

When backport, we should incorporate the fix for the test, CL 630136.

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/630196 mentions this issue: [release-branch.go1.23] syscall: mark SyscallN as noescape

gopherbot pushed a commit that referenced this issue Nov 20, 2024
@qmuntal @gopherbot
[release-branch.go1.23] syscall: mark SyscallN as noescape
847cb6f 
syscall.SyscallN is implemented by runtime.syscall_syscalln, which makes
sure that the variadic argument doesn't escape.

There is no need to worry about the lifetime of the elements of the
variadic argument, as the compiler will keep them live until the
function returns.

For #70197
Fixes #70202

Change-Id: I12991f0be12062eea68f2b103fa0a794c1b527eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/625297
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 7fff741)
Reviewed-on: https://go-review.googlesource.com/c/go/+/630196
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
@dmitshur
Copy link
Contributor

Closed by merging CL 630196 (commit 847cb6f) to release-branch.go1.23.

mpminardi pushed a commit to tailscale/go that referenced this issue Jan 28, 2025
@qmuntal @mpminardi
[release-branch.go1.23] syscall: mark SyscallN as noescape
acda438 
syscall.SyscallN is implemented by runtime.syscall_syscalln, which makes
sure that the variadic argument doesn't escape.

There is no need to worry about the lifetime of the elements of the
variadic argument, as the compiler will keep them live until the
function returns.

For golang#70197
Fixes golang#70202

Change-Id: I12991f0be12062eea68f2b103fa0a794c1b527eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/625297
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 7fff741)
Reviewed-on: https://go-review.googlesource.com/c/go/+/630196
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Milestone
Go1.23.4
Development

No branches or pull requests
5 participants
@dmitshur @dr2chase @ianlancetaylor @gopherbot @cherrymui