go icon indicating copy to clipboard operation
go copied to clipboard

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

Open dagood opened this issue 2 years ago • 4 comments

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
  • Like I mentioned in https://github.com/golang/go/issues/57913, I'm not sure if it makes sense to use mkwinsyscall to generate a broad list of APIs like these, but I figure it's worth making room for the possibility by proposing this change.

dagood avatar Jan 18 '23 20:01 dagood

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

ianlancetaylor avatar Jan 18 '23 21:01 ianlancetaylor

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

https://github.com/golang/go/blob/9894ded19417fbc40420dc813d3c6606348ad31b/src/runtime/syscall_windows.go#L528-L533

qmuntal avatar Jan 19 '23 07:01 qmuntal

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

alexbrainman avatar Jan 22 '23 01:01 alexbrainman

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

sladyn98 avatar Feb 03 '23 17:02 sladyn98

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

alexbrainman avatar Feb 04 '23 01:02 alexbrainman

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

dagood avatar Feb 06 '23 16:02 dagood

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

gopherbot avatar Aug 12 '23 01:08 gopherbot

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

gopherbot avatar Aug 12 '23 01:08 gopherbot