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

[David Holmes]

On Thu, 21 Mar 2024 07:13:40 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> src/hotspot/os/posix/os_posix.cpp line 718:
>> 
>>> 716:     const char* tmp = ::dlerror();
>>> 717:     if (tmp != nullptr) {
>>> 718:       log_debug(os)("Symbol %s not found in dll: %s", name, tmp);
>> 
>> It is unclear whether failing to locate a symbol simply because it isn't there is considered an error? dlsym and dlerror docss are silent on that. So I'm not clear if the logging happens for all missing symbols, or only where the dlsym actually encountered an error? If this is only meant for actual errors then the message seems wrong. If the intent is to report all missing symbols, then making the log statement conditional seems wrong.
>
> Thanks for having a look. Yes this log line will happen for every symbol we don't find (or other 'errors'), therefore I put it as debug.
> For example there is around 40 symbols we try to find libjsvml producing:
> `[0.211s][debug][os] Symbol __jsvml_atan2_ha_l9 not found in dll: /home/rehn/source/jdk/vanilla/build/hsdis/jdk/lib/libjsvml.so: undefined symbol: __jsvml_atan2_ha_l9
> `
> But for my case with hsdis I would have gotten this output:
> 
> [0.219s][info ][os] shared library load of /home/rehn/source/jdk/vanilla/build/hsdis/jdk/lib/hsdis-amd64.so was successful
> [0.219s][debug][os] Symbol decode_instructions_virtual not found in dll: /home/rehn/source/jdk/vanilla/build/hsdis/jdk/lib/hsdis-amd64.so: undefined symbol: decode_instructions_virtual
> 
> 
> I think it reasonable that at debug we list symbols we tried to find.
> What wording do you think is better?
> 
> On windows if dll_lookup fails, and caller thinks this is an error it can call os::lasterror.
> As these error are also reported via GetLastError.
> On posix they are instead reported via dlerror, so os::lasterror don't report them.
> Should we add a os::dll_lasterror() ?
> 
> So we can do ~:
> 
> _decode_instructions_virtual = CAST_TO_FN_PTR(Disassembler::decode_func_virtual,
>                                             os::dll_lookup(_library, decode_instructions_virtual_name));
> + if (_decode_instructions_virtual == nullptr) {
> +   char errmsg[512];
> +   os::dll_lasterror(errmsg, 512);
> +   log_error(os)(....., errmsg);
> + }

So if `ret == nullptr` then we are guaranteed that `::dl_error` does not return `nullptr`, in which case we don't really need the null check, but I suppose it is conservatively more robust to have it.

I don't think cross-platform os::dll_last_error is really feasible given the Posix version will keep the value across unrelated syscalls while on windows it would be lost on the next syscall. But you could add the same kind of logging to Windows

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

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