RFR: 8364816: GetLastError() in os_windows.cpp should not store value to errno [v3]
Joel Sikström
jsikstro at openjdk.org
Wed Aug 27 12:11:45 UTC 2025
On Wed, 27 Aug 2025 10:51:57 GMT, Anton Artemov <duke at openjdk.org> wrote:
>> Hi, please consider the following changes:
>>
>> In `os_windows.cpp` in a few places results returned by `GetLastError()` are stored to `errno`. However, `errno` has no relation to `GetLastError()`, and their ranges are not the same.
>>
>> Results of `GetLastError()` should be stored into variables of type `DWORD`.
>>
>> The removed section in `src/hotspot/share/cds/aotClassLocation.cpp` was relying on values returned by `GetLastError()` and stored to `errno` in `os::stat()`. Though the logic was correct, these values should not be stored to `errno`. The functionality is preserved by storing a **valid** value `ENOENT` to `errno` in `os::stat()`.
>>
>> Tested in tiers 1 - 3.
>
> Anton Artemov has updated the pull request incrementally with two additional commits since the last revision:
>
> - 8364816: Fixed whitespace error.
> - 8364816: Added missed assignment to errno.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26901#discussion_r2303734030
More information about the hotspot-runtime-dev
mailing list