RFR : 8211326 : add OS user related information to hs_err file

[Thomas Stüfe]

On Thu, Nov 15, 2018 at 1:43 PM David Holmes <david.holmes at oracle.com> wrote:
>
> On 15/11/2018 10:33 pm, Thomas Stüfe wrote:
> > I'm fine with this, Matthias.
> >
> > BTW, I looked and the STEP header line is only printed when secondary
> > exceptions happen inside the step, not always as David assumed (?). So
>
> Yes my bad - sorry.

no problem. I did not see it right away either.

>
> > your first form
> >
> > STEP("blabla")
> >    if (ExtensiveErrorReports && _verbose) {
> >       blub
> >    }
> >
> > would have been fine too I think. But I do not want to send you
> > through another review iteration, so I am fine with this version.
>
> Deleting the else doesn't need another round of review.

Same from me. Remove the else path and ship it.

..Thomas

>
> Thanks,
> David
>
>
> > Thanks for incorporating my remarks.
> >
> > Cheers,Thomas
> >
> >
> > On Thu, Nov 15, 2018 at 12:51 PM Baesken, Matthias
> > <matthias.baesken at sap.com> wrote:
> >>
> >> Hi Thomas,
> >>
> >> http://cr.openjdk.java.net/~mbaesken/webrevs/8211326.3/
> >>
> >>
> >> - got  rid of buf+buflen arguments of print_user_info
> >> - removed "Removing"
> >> - adjusted the output a bit   in  os::Posix::print_user_info()
> >> - vmError.cpp  added a short  output line  with info in case   ( ExtensiveErrorReports && _verbose  )  is false
> >>
> >>
> >> Best regards, Matthias
> >>
> >>
> >>> -----Original Message-----
> >>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
> >>> Sent: Mittwoch, 14. November 2018 18:43
> >>> To: Baesken, Matthias <matthias.baesken at sap.com>
> >>> Cc: David Holmes <david.holmes at oracle.com>; Langer, Christoph
> >>> <christoph.langer at sap.com>; Volker Simonis <volker.simonis at gmail.com>;
> >>> HotSpot Open Source Developers <hotspot-dev at openjdk.java.net>
> >>> Subject: Re: RFR : 8211326 : add OS user related information to hs_err file
> >>>
> >>> Hi Matthias,
> >>>
> >>> none of my notes from my earlier mail are addressed? I guess my mail
> >>> got lost in all that discussion. Here you go:
> >>>
> >>> +  static void print_user_info(outputStream* st, char* buf, size_t buflen);
> >>>
> >>> You can get rid of buf+buflen arguments, not needed anymore
> >>> ---
> >>> http://cr.openjdk.java.net/~mbaesken/webrevs/8211326.2/src/hotspot/os/
> >>> posix/os_posix.cpp.udiff.html
> >>>
> >>> In os::Posix::print_user_info(), for brevity, I would print it all one
> >>> line like this:  "uid: 4711, euid: 0, euid: 4711, egid: 4711, umask:
> >>> rwxr--r--"
> >>>
> >>> ---
> >>>
> >>> The "removing" text does not make sense and can be removed.
> >>>
> >>> Best Regards, Thomas
> >>>
> >>>
> >>> On Wed, Nov 14, 2018 at 6:16 PM Baesken, Matthias
> >>> <matthias.baesken at sap.com> wrote:
> >>>>
> >>>> Hi Thomas , so are you fine with the latest revision
> >>>>
> >>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8211326.1/
> >>>>
> >>>> of the patch ,  can I add you as reviewer ?
> >>>>
> >>>> David - is the info added by Thomas sufficient for you?
> >>>>
> >>>> Any other reviewers ?
> >>>>
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
> >>>>> Sent: Samstag, 10. November 2018 14:04
> >>>>> To: David Holmes <david.holmes at oracle.com>
> >>>>> Cc: Baesken, Matthias <matthias.baesken at sap.com>; Langer, Christoph
> >>>>> <christoph.langer at sap.com>; Volker Simonis
> >>> <volker.simonis at gmail.com>;
> >>>>> HotSpot Open Source Developers <hotspot-dev at openjdk.java.net>
> >>>>> Subject: Re: RFR : 8211326 : add OS user related information to hs_err file
> >>>>>
> >>>>> Hi David,
> >>>>>
> >>>>> On Sat, Nov 10, 2018 at 12:59 PM David Holmes
> >>> <david.holmes at oracle.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> Hi Matthias,
> >>>>>>
> >>>>>> On 9/11/2018 3:13 AM, Baesken, Matthias wrote:
> >>>>>>> Hello  , after the flag   "-XX:+-ExtensiveErrorReports"   make it into
> >>> jdk/jdk
> >>>>> , I created another webrev :
> >>>>>>>
> >>>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8211326.1/
> >>>>>>>
> >>>>>>>
> >>>>>>> The user info output is now guarded  by ExtensiveErrorReports .
> >>>>>>
> >>>>>> +   STEP("printing user info")
> >>>>>> +      if (ExtensiveErrorReports) {
> >>>>>> +        if (_verbose) {
> >>>>>> +          os::Posix::print_user_info(st, buf, sizeof(buf));
> >>>>>> +        }
> >>>>>> +      }
> >>>>>>
> >>>>>> I don't understand why we explicitly need _verbose if we've asked for
> >>>>>> ExtensiveErrorreports?
> >>>>>
> >>>>> That flag has a different purpose: _verbose distinguishes the two
> >>>>> calls to VMError::report(): the first one to print a small report to
> >>>>> stderr, the second print a large report to the hs-err file. We only
> >>>>> want to print to the hs-err file, so _verbose is still needed.
> >>>>>
> >>>>>>
> >>>>>> Also, ideally the STEP would be inside the guard as otherwise we will
> >>>>>> just print the step description followed by nothing. If the macro
> >>>>>> expansion makes that impossible then we should have an else clause
> >>> that
> >>>>>> prints something like:
> >>>>>>
> >>>>>>    - disabled (use -XX:+ExtensiveErrorReports to see these details)
> >>>>>>
> >>>>>
> >>>>> We may even go a step further (but in a separate patch) and make
> >>>>> "ExtensiveErrorReports" a property of the STEP, to be given as
> >>>>> parameter of the STEP macro.
> >>>>>
> >>>>> Thanks, Thomas
> >>>>>
> >>>>