On Thu, 4 Aug 2022 17:42:45 GMT, Julian Waters <jwat...@openjdk.org> wrote:
>> Please review a small patch for dumping the failure reason when the MSVCRT >> libraries or the Java Virtual Machine fails to load on Windows, which can >> provide invaluable insight when debugging related launcher issues. This also >> fixes a small bug in the Windows implementation of JLI_ErrorMessageSys where >> C runtime errors are never written to stderr. > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Missing spacing between errors Hi Julian, > > 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, > why, there was no 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. Oh right. I see. Not your change, but I am not a big fan of this pattern (mixing CRT and win32 error handling). This is broken since it assumes that win32 error code and errno are mutually exclusive, or somehow related, which they are not. A lingering GetLastError() from some earlier win32 API call could be misreported as error detail for a follow-up CRT error. The correct way to do this would be to have two functions, one for failing win32 APIs, one for failing CRT functions. And let the caller decide what to call. > 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. If you can make the fix for the CRT extra info small, I'd go for it. > > 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 I think it would be reasonable to expect that the additional info printed by that function gets visually separated somehow from the error text the caller hands in. Note, btw, if you change output like this, make sure to check all test sources for tests that may grep the output. Not sure how likely this is here. Just something to keep in mind. This is nothing an IDE will tell you, unfortunately. Cheers, Thomas ------------- PR: https://git.openjdk.org/jdk/pull/9749
