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

x/sys/windows: mkwinsyscall: support syscalls with more than 15 params #57914

Closed
dagood opened this issue Jan 18, 2023 · 8 comments
Closed

x/sys/windows: mkwinsyscall: support syscalls with more than 15 params #57914

dagood opened this issue Jan 18, 2023 · 8 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@dagood
Copy link
Contributor

dagood commented Jan 18, 2023

Currently mkwinsyscall includes a 15 arg limit with a panic: https://github.com/golang/sys/blob/6e4d1c53cf3b2c1c6d104466749f76cc7a5c9ed8/windows/mkwinsyscall/mkwinsyscall.go#L554-L558

The limit was committed Oct 3, 2019, and since then, Go 1.18 deprecated the individual Syscall, Syscall6, etc. methods in favor of SyscallN.

Changing mkwinsyscall to use SyscallN simplifies the code as well as widening support, unless I'm not aware of some caveat. 😄

Here's an example API with 17 params: https://learn.microsoft.com/en-us/windows/win32/api/fontsub/nf-fontsub-createfontpackage

Background

I'm trying to use mkwinsyscall for x/sys/windows: use win32metadata? #43838 by having a generator create //sys comments. When I generated //sys lines for all the API methods for Windows.Win32.winmd, I found a handful of methods with a few more params than the current limit:

CreateFontPackage
ICDrawBegin
DRMGetUsagePolicy
ScriptPlaceOpenType
D2D1GetGradientMeshInteriorPointsFromCoonsPatch
AccessCheckByTypeAndAuditAlarmW
AccessCheckByTypeResultListAndAuditAlarmW
AccessCheckByTypeResultListAndAuditAlarmByHandleW
AccessCheckByTypeAndAuditAlarmA
AccessCheckByTypeResultListAndAuditAlarmA
AccessCheckByTypeResultListAndAuditAlarmByHandleA
@gopherbot gopherbot added this to the Proposal milestone Jan 18, 2023
@ianlancetaylor
Copy link
Contributor

Thanks. I don't think this has to be a proposal. It's just a bug fix.

@ianlancetaylor ianlancetaylor changed the title proposal: x/sys/windows/mkwinsyscall: Support syscalls with more than 15 params x/sys/windows: mkwinsyscall: support syscalls with more than 15 params Jan 18, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 18, 2023
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal compiler/runtime Issues related to the Go compiler and/or runtime. labels Jan 18, 2023
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unreleased Jan 18, 2023
@qmuntal
Copy link
Contributor

qmuntal commented Jan 19, 2023

For the record: as of today (go1.20), syscall.SyscallN is internally limited to 42 arguments, even though it accepts a variadic input.

// maxArgs should be divisible by 2, as Windows stack
// must be kept 16-byte aligned on syscall entry.
//
// Although it only permits maximum 42 parameters, it
// is arguably large enough.
const maxArgs = 42

@alexbrainman
Copy link
Member

Thank you @dagood for creating this issue.

I agree with Ian. It is just a bug in mkwinsyscall program. We should fix it.

Perhaps we could even change mkwinsyscall to stop using Syscall3 and friends and use SyscallN instead. If we do that, we should regenerate all existing code in the standard library and goloang.org/x/sys/windows/... packages at the same time.

Alex

@sladyn98
Copy link

sladyn98 commented Feb 3, 2023

I could give this a shot @alexbrainman, would this fix just involve changing the number of params ?

@alexbrainman
Copy link
Member

I could give this a shot @alexbrainman, ...

Sure. Go ahead. Unless @dagood or others already started working on this change.

... would this fix just involve changing the number of params ?

Current version uses Syscall/Syscall6/Syscall9/Syscall12/Syscall15 syscalls.

You can add Syscall18 support, that will take care of https://learn.microsoft.com/en-us/windows/win32/api/fontsub/nf-fontsub-createfontpackage with 17 parameters that is listed above.

Or you could replace all Syscall/Syscall6/Syscall9/Syscall12/Syscall15 syscalls with new SyscallN. That will cover every call that Go syscall package supports. If you do that, you would have to regenerate all code that uses mkwinsyscall program in golang.org/x/sys/windows/... and in the standard library, so people who use mkwinsyscall next time are not surprised by large diffs that new version of mkwinsyscall will generate.

Alex

@dagood
Copy link
Contributor Author

dagood commented Feb 6, 2023

I'm not currently working on this. Thanks @sladyn98 for giving it a try. 🙂

mauri870 added a commit to mauri870/sys that referenced this issue Aug 12, 2023
The mkwinsyscall command has a hard limit of 15 on the
number of syscall arguments. Windows has several system
calls with more than 15 arguments, for example CreateFontPackage
has 18 arguments.

Another limiting factor is that mkwinsyscall uses the various
SyscallX functions, that generates extra complexity that can
be avoided by relying on SyscallN.

Updates golang/go#57914
@gopherbot
Copy link

Change https://go.dev/cl/518995 mentions this issue: windows: use SyscallN in mkwinsyscall

mauri870 added a commit to mauri870/go that referenced this issue Aug 12, 2023
This is a follow up for CL 518995 that regenerates the
windows zsyscalls to use SyscallN instead of the various
SyscallX functions.

Fixes golang#57914
@gopherbot
Copy link

Change https://go.dev/cl/518857 mentions this issue: syscall: update windows zsyscalls to use SyscallN

mauri870 added a commit to mauri870/sys that referenced this issue Aug 12, 2023
The mkwinsyscall command has a hard limit of 15 on the
number of syscall arguments. Windows has several system
calls with more than 15 arguments, for example CreateFontPackage
has 18 arguments.

If the number of arguments is higher than 15 we use SyscallN.

Updates golang/go#57914
mauri870 added a commit to mauri870/sys that referenced this issue Aug 12, 2023
The mkwinsyscall command has a hard limit of 15 on the
number of syscall arguments. Windows has several system
calls with more than 15 arguments, for example CreateFontPackage
has 18 arguments.

If the number of arguments is higher than 15 we use SyscallN.

Updates golang/go#57914
mauri870 added a commit to mauri870/sys that referenced this issue Aug 12, 2023
The mkwinsyscall command has a hard limit of 15 on the
number of syscall arguments. Windows has several system
calls with more than 15 arguments, for example CreateFontPackage
has 18 arguments.

If the number of arguments is higher than 15 we use SyscallN.

Updates golang/go#57914
mauri870 added a commit to mauri870/sys that referenced this issue Aug 13, 2023
The mkwinsyscall command has a hard limit of 15 on the
number of syscall arguments. Windows has several system
calls with more than 15 arguments, for example CreateFontPackage
has 18 arguments.

If the number of arguments is higher than 15 we use SyscallN.

Updates golang/go#57914
mauri870 added a commit to mauri870/sys that referenced this issue Aug 13, 2023
The mkwinsyscall command has a hard limit of 15 on the
number of syscall arguments. Windows has several system
calls with more than 15 arguments, for example CreateFontPackage
has 18 arguments.

If the number of arguments is higher than 15 we use SyscallN.

Updates golang/go#57914
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
6 participants