RFR: 8328630: Add logging when needed symbols in dll are missing. [v3]

[Robbin Ehn]

On Tue, 2 Apr 2024 01:50:28 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> src/hotspot/os/windows/os_windows.cpp line 1295:
>> 
>>> 1293:   void* ret = ::GetProcAddress((HMODULE)lib, name);
>>> 1294:   DWORD errval;
>>> 1295:   if ((errval = GetLastError()) != 0) {
>> 
>> `GetProcAddress` doesn't clear last-error on success, so you should only be calling `GetLastError` if the return value was null.
>
> That said, why are you duplicating the logic from `os::lasterror` instead of just passing it the buf to fill in?
> 
> if (ret == nullptr) {
>   char buf[512];
>   os::lasterror(buf, sizeof(buf));
>   log_debug(os)(...);
> }

> `GetProcAddress` doesn't clear last-error on success, so you should only be calling `GetLastError` if the return value was null.

It seem like you are correct. I didn't find anything on GetProcAddress manpage, but on GetLastError page it says: "If the function is not documented to set the last-error code, the value returned by this function is simply the most recent last-error code"

I'll update.

> That said, why are you duplicating the logic from `os::lasterror` instead of just passing it the buf to fill in?
> 
> ```
> if (ret == nullptr) {
>   char buf[512];
>   os::lasterror(buf, sizeof(buf));
>   log_debug(os)(...);
> }
> ```

It's not clear to me that GetProcAddress will not return NULL without setting GetLastError.
Thus calling os::lasterror could accidentally return error string for errno instead.
Maybe this worry have no basis, what you think?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18404#discussion_r1547245559