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

[David Holmes]

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 …
>      >      >
>      >      >
>      >      >
>      >      > Best regards, Matthias
>      >      >
>      >      >
>      >      > From: Thomas Stüfe <thomas.stuefe at gmail.com
>     <mailto:thomas.stuefe at gmail.com>
>      >     <mailto:thomas.stuefe at gmail.com
>     <mailto:thomas.stuefe at gmail.com>>>
>      >      > Sent: Mittwoch, 5. Juni 2019 09:31
>      >      > To: Baesken, Matthias <matthias.baesken at sap.com
>     <mailto:matthias.baesken at sap.com>
>      >     <mailto:matthias.baesken at sap.com
>     <mailto:matthias.baesken at sap.com>>>
>      >      > Cc: hotspot-dev at openjdk.java.net
>     <mailto:hotspot-dev at openjdk.java.net>
>      >     <mailto:hotspot-dev at openjdk.java.net
>     <mailto:hotspot-dev at openjdk.java.net>>
>      >      > Subject: Re: RFR [XS]: 8224958: add os::dll_load calls to
>     event log
>      >      >
>      >      > Hi Matthias,
>      >      >
>      >      > looks good. Minor nit:
>      >      >
>      >      > +  if (error_report == NULL) {
>      >      > +    error_report = "dlerror returned no error description";
>      >      > +  }
>      >      > +  if (error_report && ebuf && ebuflen > 0) {
>      >      >
>      >      > Since you ensure error_report is always set - with a default
>      >     text, if needed - you do not need to check it for NULL
>     afterwards.
>      >      >
>      >      > If you only fix this, I do not need a new webrev.
>      >      >
>      >      > ---
>      >      >
>      >      > Side note:
>      >      >
>      >      >
>      >
>     http://cr.openjdk.java.net/~mbaesken/webrevs/8224958.1/src/hotspot/os/windows/os_windows.cpp.udiff.html
>      >      >
>      >      > You moved lasterror() up to get a "fresh" error value before
>      >     calling into event logging right? I needed a while to understand
>      >     that. Okay.
>      >      >
>      >      > This shows that lasterror() is actually not well designed.
>      >     lasterror() does two things: 1) get the error code with
>     GetLastError
>      >     and 2) translate that numerical code to a text.
>      >      >
>      >      > 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.
>      >      > Cheers, Thomas
>      >      >
>      >      >
>      >
>