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: Add support for DLLs with extensions other than .dll #58337

Open
ranganath42 opened this issue Feb 5, 2023 · 9 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@ranganath42
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.20 windows/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
    :
GOARCH=amd64
GOOS=windows
    :

What did you do?

Some of the Windows system dynamic link library (DLL) files have extensions other than .dll.
For example,

  • ActiveX Controls files with .ocx extension
  • Control Panel applets with .cpl extension
  • Device drivers with .drv extension

mkwinsyscall does not support generating the syscall wrappers for these DLLs with different extensions.
The extension is always hard-coded to .dll in the generated code.

//sys ClosePrinter(h syscall.Handle) (err error) = winspool.ClosePrinter

What did you expect to see?

var (
        modwinspool = windows.NewLazySystemDLL("winspool.drv")
        procClosePrinter = modwinspool.NewProc("ClosePrinter")
)

What did you see instead?

var (
        modwinspool = windows.NewLazySystemDLL("winspool.dll")
        procClosePrinter = modwinspool.NewProc("ClosePrinter")
)

Proposed solution

If the user specified the extension of the DLL in the //sys comment, use that extension instead of adding the .dll extension.
Going back to the previous example, the user can specify the extension as .drv in the //sys comment.

//sys ClosePrinter(h syscall.Handle) (err error) = winspool.drv.ClosePrinter

Then, the generated code will be,

var (
        modwinspool = windows.NewLazySystemDLL("winspool.drv")
        procClosePrinter = modwinspool.NewProc("ClosePrinter")
)

The code changes are fairly straightforward. I have working code that I can submit as a pull request if the proposal makes sense.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 5, 2023
@gopherbot gopherbot added this to the Unreleased milestone Feb 5, 2023
@ranganath42
Copy link
Author

The downside of the proposed solution is that if the DLL name itself has a dot (for example, Windows.Networking.dll), then the user will need to specify the full DLL name including the .dll. extension. Here's the corresponding issue.

@bcmills
Copy link
Contributor

bcmills commented Feb 6, 2023

(CC @golang/windows)

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 6, 2023
@ranganath42
Copy link
Author

Anything else that I can provide here, team?

@qmuntal
Copy link
Contributor

qmuntal commented Feb 16, 2023

The proposed solution enters in conflict with what has been implemented in #57913 to support DLL names with . and -. It is difficult, if not impossible, to differentiate between a . that is part of the name or that is separating the name from the extension in the general case.

For example, if I had to use the method ClosePrinter from an (imaginary but legit) DLL whose file name is printer.drv.dll, I would use the following comment:

//sys ClosePrinter(h syscall.Handle) (err error) = printer.drv.ClosePrinter

Which would generate this:

  modprinter_drv = windows.NewLazySystemDLL("printer.drv.dll")
  procClosePrinter = modprinter_drv.NewProc("ClosePrinter")

With your solution, it could incorrectly generate the following:

  modprinter = windows.NewLazySystemDLL("printer.drv")
  procClosePrinter = modprinter.NewProc("ClosePrinter")

In summary, I would avoid overloading the DLL name in the //sys comment. An alternative approach would be to accept an additional optional parameter, similar to what we already do for custom error codes with [failretval==X]. It could look like this (using your example):

//sys ClosePrinter(h syscall.Handle) (err error) = winspool.ClosePrinter [ext=drv]

@dagood @alexbrainman

@dagood
Copy link
Contributor

dagood commented Feb 16, 2023

Yeah, removing ambiguity makes the most sense to me, and I feel like I should have explored that more rather than escaping . and -. 😄 I had an idea in #57913 that I like the look of (parallels with Go syntax!) but it seems to me would mean changing how the file is parsed/stored a fair amount:

//sysimport windowsnetworking "windows.networking.dll"
//sys	SetSocketMediaStreamingMode(...) (...) = windowsnetworking.SetSocketMediaStreamingMode
//sysimport winspool "winspool.drv"
//sys	ClosePrinter(h syscall.Handle) (err error) = winspool.ClosePrinter

Adding [ext=drv] seems to me like a reasonable minimal way to remove the ambiguity and fix this.

@ranganath42
Copy link
Author

Agreed, a new optional parameter like [ext=drv] sounds like a good way to handle exceptions like these. I'll create a pull request.
Thanks for looking into this.

@alexbrainman
Copy link
Member

Adding [ext=drv] seems to me like a reasonable minimal way to remove the ambiguity and fix this.

Agreed. It is unfortunate to complicate things, but I don't see any other way to solve this problem. Hopefully there are not many dll files with not dll extension, so this scenario will be pretty rare.

Alex

@ranganath42
Copy link
Author

This pull request addresses the above issue.

@alexbrainman
Copy link
Member

This pull request addresses the above issue.

Replied at

https://go-review.googlesource.com/c/sys/+/506697

Alex

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. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
Development

No branches or pull requests

9 participants