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: GetLastError always returns nil #41220

Open
bupjae opened this issue Sep 4, 2020 · 7 comments
Open

x/sys/windows: GetLastError always returns nil #41220

bupjae opened this issue Sep 4, 2020 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. OS-Windows
Milestone

Comments

@bupjae
Copy link

bupjae commented Sep 4, 2020

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

$ go version
go version go1.14.7 windows/amd64

Does this issue reproduce with the latest release?

Not tried, but I'm pretty sure it will.

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

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Bupjae\AppData\Local\go-build
set GOENV=C:\Users\Bupjae\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\Bupjae\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\Bupjae\AppData\Local\Temp\go-build843644218=/tmp/go-build -gno-record-gcc-switches

What did you do?

package main

import (
	"fmt"
	"golang.org/x/sys/windows"
)

func main() {
	var handle windows.Handle
	wrongName := windows.StringToUTF16Ptr("ADFADFASDFSDAFASFD")
	err := windows.GetModuleHandleEx(0, wrongName, &handle)
	if handle == 0 {
		fmt.Println(err)
		fmt.Println(windows.GetLastError())
	}
}

What did you expect to see?

Prints out error message describing 'module not found' twice.

What did you see instead?

The specified module could not be found.
<nil>

According to https://golang.org/src/runtime/sys_windows_amd64.s , before calling actual DLL routine (line 58), last error information is erased (line 22-23). This makes syscall.GetLastError (and windows.GetLastError) completely meaningless,

@gopherbot gopherbot added this to the Unreleased milestone Sep 4, 2020
@ianlancetaylor
Copy link
Contributor

The idea is that the error will be returned by the call, so you should never to call GetLastError yourself. And your code shows that: the err value is the error you want.

So, if there is anything to do here, it's simply to document that there is no point to calling GetLastError. I don't know why we even define it.

@ianlancetaylor ianlancetaylor changed the title x/sys/windows: calling Syscall clears last error information before calling actual DLL call, even GetLastError. x/sys/windows: GetLastError always returns nil Sep 5, 2020
@ianlancetaylor ianlancetaylor added Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Sep 5, 2020
@ianlancetaylor
Copy link
Contributor

CC @alexbrainman

@alexbrainman
Copy link
Member

What Ian said - do not call GetLastError directly from your code - Go runtime calls GetLastError for you behind the scenes.

It is pointless to call GetLastError from your Go code, because GetLastError changes every time you make new Windows API call. But Go runtime calls different Windows API at all times (for example to get memory or create threads). The fact that you don't see any Windows API calls in your code does not mean your program does not call Windows APIs.

Go runtime also changes threads all the time. GetLastError is thread specific. So, if you call GetLastError from your code, you might be getting GetLastError value from another unrelated thread.

According to https://golang.org/src/runtime/sys_windows_amd64.s , before calling actual DLL routine (line 58), last error information is erased (line 22-23).

On lines 70-73 this function gathers GetLastError value right after Windows API is finished. Just use that value.

Alex

@bupjae
Copy link
Author

bupjae commented Nov 16, 2021

Sorry for bumping up more than 1-year-old thread.

I understand that calling GetLastError is not the right way to get error.

Then, I think it should be documented properly, such as "GetLastError will return always nil. Use return value of each API call instead", as suggested in previous comment. Maybe marking GetLastError as deprecated is also possible.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@bcmills
Copy link
Contributor

bcmills commented Feb 22, 2024

As an alternative, I wonder if we could change the Go runtime to provide a stronger invariant that makes GetLastError actually work. For example, this invariant might be feasible: “If the calling goroutine is locked to an OS thread, a call to GetLastError immediately following a system call (with no other function calls or variable declarations in between) returns the error set by that system call.”

And perhaps GetLastError should panic (or return an error unconditionally) if the calling goroutine is not locked to an OS thread?

That invariant might require more care in preserving and restoring error values in the runtime's signal handlers, although if signal-handler changes are needed for this invariant, they are probably also needed in order to avoid corrupting GetLastError return values in programs that use cgo.

(CC @golang/windows)

@bcmills bcmills added OS-Windows NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed Documentation help wanted labels Feb 22, 2024
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Feb 22, 2024
@alexbrainman
Copy link
Member

As an alternative, I wonder if we could change the Go runtime to provide a stronger invariant that makes GetLastError actually work. For example, this invariant might be feasible: “If the calling goroutine is locked to an OS thread, a call to GetLastError immediately following a system call (with no other function calls or variable declarations in between) returns the error set by that system call.”

What would be the benefit of making runtime code and the documentation more complicated in comparison to just using the returned GetLastError value from original API call? See

On lines 70-73 this function gathers GetLastError value right after Windows API is finished. Just use that value.

from #41220 (comment) .

And perhaps GetLastError should panic (or return an error unconditionally)

I am OK with that, if you feel strongly about it. But I don't think many people call GetLastError anyway.

We can also just document not to use it. And explain the reason.

Alex

@bcmills
Copy link
Contributor

bcmills commented Feb 26, 2024

What would be the benefit of making runtime code and the documentation more complicated in comparison to just using the returned GetLastError value from original API call?

The benefit would be that callers can use GetLastError for purposes described in the Microsoft documentation (such as the case in #64170) and get the documented behavior of that system call, like they can do for most other system call functions provided by the syscall and x/sys/windows packages.

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. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. OS-Windows
Projects
Status: Triage Backlog
Development

No branches or pull requests

5 participants