RFR: 8291917: Windows - Dump error diagnostics when runtime libraries fail to load [v3]

[Julian Waters]

On Sat, 6 Aug 2022 10:54:56 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> Hi Julian,
> 
> I don't get it.
> 
> AFAICS the existing version displays the failure reason for win32 API just fine, appending it to the message. Where is the bug? I see that the message was omitted for CRT errors, so that is okay to fix, but that could have been fixed with a simple freeit=1 in the CRT error path (although my taste would be to just append errtxt if errtxt!=NULL, which is less circumvent).
> 
> I also do not understand the changes to the call sites, where you add "%s". What is the point? If you want to separate error message and detail error message with " - ", you can just do an additional conditional strcat in the error message function.
> 
> Cheers, Thomas

Hi Thomas, sorry for the delay,

Yes, the bug was the CRT errors being omitted, the issue with setting freeit for CRT errors too is that it, from what I can see, is only supposed to be used by the branch for Win32 errors, and is used for calling LocalFree (required for FormatMessage) later on. Setting this for CRT errors would mean LocalFree is passed something that LocalAlloc did not allocate, which would likely result in bad things happening. I didn't feel like changing too much of the existing logic just to fix a small issue and decided to leave the calculation and concatenation of the message to other JLI functions, but that may have been a misjudgement on my part.

JLI_ReportErrorMessageSys is documented to deliberately leave the formatting of spacing up to the caller - Which is why no changes were made to it to accomodate that quirk. Admittedly a strcat could've been used in the caller rather than an additional format specifer though, like you mentioned

best regards,
Julian

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

PR: https://git.openjdk.org/jdk/pull/9749