RFR: 8364816: GetLastError() in os_windows.cpp should not store value to errno [v3]

[Joel Sikström]

On Wed, 27 Aug 2025 13:06:03 GMT, Anton Artemov <duke at openjdk.org> wrote:

>> src/hotspot/os/windows/os_windows.cpp line 4821:
>> 
>>> 4819:     errno = ENOENT;
>>> 4820:     DWORD errcode = ::GetLastError();
>>> 4821:     log_debug(os)("os::stat() failed to GetFileAttributesExW: GetLastError->%lu.", errcode);
>> 
>> I'm not sure this is the right solution. Now that we don't store the value of `GetLastError()` in errno and instead set errno to `ENOENT`, we don't detect if we get some other error than no entry. I think we should only set errno to `ENOENT` if `GetLastError()` returns either `ERROR_FILE_NOT_FOUND` or `ERROR_PATH_NOT_FOUND`.
>> 
>> If `GetLastError()` returns something else, we should not set errno to `ENOENT`, but some other value. It's hard since we can't do a 1:1 mapping between `GetLastError()` and errno error values. We could use `EOTHER`, as defined in https://learn.microsoft.com/en-us/cpp/c-runtime-library/errno-constants?view=msvc-170. 
>> 
>> I really think that `os::stat` should move away from using errno in this case. Also, it isn't really communicated clearly that that `os::stat()` is setting errno. I think this is best addressed in a follow-up, but it makes it harder to reason clearly about this patch.
>
>> I'm not sure this is the right solution. Now that we don't store the value of `GetLastError()` in errno and instead set errno to `ENOENT`, we don't detect if we get some other error than no entry. I think we should only set errno to `ENOENT` if `GetLastError()` returns either `ERROR_FILE_NOT_FOUND` or `ERROR_PATH_NOT_FOUND`.
> 
> I added this `if` case, but generally I do not think it is needed at all. 
> 
> We do not need to detect multiple flavours of failure. Simply because we are interested only in the end result, which is "does the file exist or not". If it does not, it does not matter why.  Introducing generic value` EOTHER `which means anything and nothing at the same time will serve no purpose. 
> 
> 
>> I really think that `os::stat` should move away from using errno in this case. Also, it isn't really communicated clearly that that `os::stat()` is setting errno. I think this is best addressed in a follow-up, but it makes it harder to reason clearly about this patch.
> 
> Maybe, but it is not in the scope of this PR.

Okay, I agree that the way the codebase is using `os::stat()` right now, we only care about whether we failed to stat or not, with one exception in the AOT code that also checks the errno value.

I think this is a tricky situation, because as `os::stat()` is designed right now it returns -1 on failure, and sets the errno value appropriately. We can't reasonable translate all `GetLastError()` values to errno, so we need some compromise. I don't think it is right that we either don't set the errno value in all cases, or always set it to the same value here.

I'll let others fill in with what they think, but ultimately we should remove using errno completely from `os::stat()`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26901#discussion_r2304076575