RFR [XS]: 8224958: add os::dll_load calls to event log

[David Holmes]

On 6/06/2019 5:15 pm, Baesken, Matthias wrote:
> Hello, I would keep  the last revision as it is .
> David may I add you as reviewer ?

Yes.

Thanks,
David

> Best regards, Matthias
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Donnerstag, 6. Juni 2019 09:02
>> To: Thomas Stüfe <thomas.stuefe at gmail.com>
>> Cc: Baesken, Matthias <matthias.baesken at sap.com>; hotspot-
>> dev at openjdk.java.net
>> Subject: Re: RFR [XS]: 8224958: add os::dll_load calls to event log
>>
>> On 6/06/2019 4:57 pm, Thomas Stüfe wrote:
>>> On Thu, Jun 6, 2019 at 1:18 AM David Holmes <david.holmes at oracle.com
>>> <mailto:david.holmes at oracle.com>> wrote:
>>>
>>>      On 5/06/2019 10:58 pm, Thomas Stüfe wrote:
>>>       > On Wed, Jun 5, 2019 at 2:50 PM David Holmes
>>>      <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>>>       > <mailto:david.holmes at oracle.com
>>>      <mailto:david.holmes at oracle.com>>> wrote:
>>>       >
>>>       >     On 5/06/2019 7:06 pm, Baesken, Matthias wrote:
>>>       >      > Hi Thomas , I removed  the error_report _ NULL check
>>>       >      >
>>>       >      > http://cr.openjdk.java.net/~mbaesken/webrevs/8224958.2/
>>>       >
>>>       >     You missed one in src/hotspot/os/bsd/os_bsd.cpp
>>>       >
>>>       >     +   if (error_report == NULL) {
>>>       >     +     error_report = "dlerror returned no error description";
>>>       >     +   }
>>>       >     +   if (error_report && ebuf && ebuflen > 0) {
>>>       >
>>>       >      >
>>>       >      >    *   You moved lasterror() up to get a "fresh" error value
>>>       >     before calling into event logging right? I needed a while to
>>>       >     understand that. Okay.
>>>       >      >
>>>       >      > Yes this was my intention .
>>>       >
>>>       >     Not sure I'd say "fresh" is the right description. An error has
>>>       >     occurred
>>>       >     and you want the error message for log statement. Its the
>>>      same error
>>>       >     message you'd get later as no other system call is being
>>>      made. There's
>>>       >     no difference to the how the code currently works, we just
>>>      want the
>>>       >     error text earlier for the log.
>>>       >
>>>       >
>>>       > not quite :)
>>>       >
>>>       > Matthias prints out the numerical errcode in his call to
>>>      Events::log,
>>>       > retrieved by GetLastError() some lines above. That has nothing to do
>>>       > with lasterror and errbuf. lasterror() is used to fill errbuf. It
>>>      too
>>>       > calls GetLastError(), which makes it position dependend. Matthias
>>>      added
>>>       > a new call into the EventLog system, so he is careful and moves
>>>      the call
>>>       > to lasterror() up in case the EventLog system calls system APIs and
>>>       > overwrites the error code.
>>>
>>>      <sigh> Sometimes my brain continues to see what it thinks it should be
>>>      seeing.
>>>
>>>
>>> Lack of coffee :)
>>
>> Alas I have tried a range of beverages but none seem to help ;-)
>>
>>>      I thought we were logging the text version of the error - why
>>>      aren't we?
>>>
>>>
>>> This comes down to taste. I personally prefer a numerical error code
>>> (would prefer this on the POSIX platforms too but dlopen() is this weird
>>> outlier that does not set errno).
>>>
>>> I'm fine with both error code or error text. Matthias should decide.
>>
>> Okay I'm fine with that.
>>
>>>      The error message has to be captured before the log call as logging may
>>>      change what is returned by GetLastError.
>>>
>>> Which is what Matthias did?
>>
>> Yes.
>>
>> Cheers,
>> David
>>
>>> Cheers, Thomas
>>>
>>>      David
>>>
>>>       >
>>>       > ..Thomas
>>>       >
>>>       >
>>>       >     David
>>>       >     -----
>>>       >
>>>       >      >
>>>       >      >
>>>       >      >    *   It would be better to have a function which only
>>>      does the
>>>       >     text translation, using an error code we give it as parameter.
>>>       >      >    *   Then we could have given it the errcode we already
>>>      have.
>>>       >     Would be good for a future improvement.
>>>       >      >
>>>       >      > There are not so many  os::lasterror   usages currently .
>>>      So I
>>>       >     am not sure if it is really needed ,   let’s  do it in a separate
>>>       >     CR  any way …
>>>       >      >
>