RFR(s): 8191101: Show register content in hs-err file on assert (second version)

Thu Mar 22 20:20:43 UTC 2018

Hi Volker,

thanks! Thorough review as usual :) - see remarks inline.

On Thu, Mar 22, 2018 at 6:36 PM, Volker Simonis <volker.simonis at gmail.com>
wrote:

> Hi Thomas,
>
> in general the change looks good! I only have a few comments/questions:
>
> debug.hpp
>
> - Why does "g_assert_poison" has to be a volatile pointer?
> extern void* volatile g_assert_poison;
>
>
It does not, and I changed it.

> - I agree with David's first review that it would be better to make
> "g_assert_poison" an "intptr_t*" and  get rid of the following cast:
> #define TOUCH_ASSERT_POISON (*(char*)g_assert_poison = 'X');
>
>
The problem with intptr_t is that it requires stdint.h to be included,
which does not get included by debug.hpp.

The standard way (posix) to get intptr_t would be to include stdint.h,
which I do not want to do in this header. I just feel disinclined to
including system headers in a generic hotspot header, but maybe I am
overcautious.

The standard (hotspot) way to include system headers is to include
globalDefinitions.hpp,
which does not work either because globalDefinitions.hpp includes
debug.hpp, so we have a circular reference (which I think is questionable).

The easiest way to deal with this is just not to use intptr_t. I understand
that you guys want to avoid the cast, so I changed the type to char*
instead of void*.

> - I'd also prefer to remove the trailing ";" from the macro definition
> of  "TOUCH_ASSERT_POISON" to make it's usage more similar to that of
> "BREAKPOINT" in the same file.
>

Okay, fixed.

>
> debug.cpp
>
> - The "ifdef WIN32" is unnecessary if we don't support Windows anyway
>  #ifdef _WIN32
>
>
Fair enough. Fixed.

>
> And finally some stylistic comments:
>
> vmError_posix.cpp
> - there's an extra, unncessary whitespace between the two opening braces:
> if ( (sig == SIGSEGV || sig == SIGBUS) && in
>

Fixed.

>
> vmError_posix.cpp, os_linux_aarch64.cpp, os_linux_arm.cpp,
> os_linux_ppc.cpp, os_linux_s390.cpp, os_linux_sparc.cpp,
> os_linux_x86.cpp, thread.cpp
> - I think the the general convention is to start preprocessor "#ifdef"
> directives at the beginning of the line and not indent them with the
> surrounding code.
>
>
Fixed.

> Thanks for finally bringing this into OpenJDK!
>

You are welcome :)

New webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8191101-show-registers-on-assert/webrev.04/webrev/
incremental webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8191101-show-registers-on-assert/webrev.04.delta/webrev/

Thanks, Thomas

> Volker
>
>
> On Wed, Mar 21, 2018 at 5:44 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
> > Thank you Christoph!
> >
> > Here the new webrev:
> >
> > http://cr.openjdk.java.net/~stuefe/webrevs/8191101-show-regi
> sters-on-assert/webrev.03/webrev/
> >
> > - copyright years fixed
> > - spaces removed as you noted
> > - Removed one blank line at the end of the jtreg test file.
> >
> > Best Regards, Thomas
> >
> >
> >
> > On Wed, Mar 21, 2018 at 9:51 AM, Langer, Christoph
> > <christoph.langer at sap.com> wrote:
> >>
> >> Hi Thomas,
> >>
> >> overall this looks good to me. I went through the changes and can't see
> >> nothing obviously wrong. But I didn't play with it nor do I have much
> >> experience myself in the area of error reporting.
> >>
> >> I found a few formatting nits:
> >> In several files you forgot to update the copyright year. But I guess
> >> you'd check this anyway before pushing.
> >> Then, in your new test
> >> 'test/hotspot/jtreg/runtime/ErrorHandling/ShowRegistersOnAss
> ertTest.java'
> >> you have two blank lines in the end. I guess jcheck would find it,
> though...
> >> The if condition in all the os* files 'if ( (sig == SIGSEGV || sig ==
> >> SIGBUS) && info != NULL && info->si_addr == g_assert_poison)' contains a
> >> blank after the first '(' which I wouldn't have used there - but that's
> >> really personal taste
> >>
> >> Best regards
> >> Christoph
> >>
> >> > -----Original Message-----
> >> > From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> >> > bounces at openjdk.java.net] On Behalf Of Thomas Stüfe
> >> > Sent: Dienstag, 20. März 2018 15:12
> >> > To: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
> >> > Subject: RFR(s): 8191101: Show register content in hs-err file on
> assert
> >> > (second version)
> >> >
> >> > Hi all,
> >> >
> >> > May I please have reviews for this new version of this RFE.
> >> >
> >> > Bug id:  https://bugs.openjdk.java.net/browse/JDK-8191101
> >> > webrev:
> >> > http://cr.openjdk.java.net/~stuefe/webrevs/8191101-show-registers-on-
> >> > assert/webrev.02/webrev/
> >> >
> >> > Please find old discussion thread here:
> >> > http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-
> >> > October/025018.html
> >> >
> >> > And the first review thread from last november:
> >> > http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-
> >> > November/025371.html
> >> >
> >> > Since then, the issue was sleeping in a pile of other issues, but I'd
> >> > like
> >> > to finish this now.
> >> >
> >> > For those who looked at the first version of this fix: this version is
> >> > substantially smaller than the last one. I removed all the
> >> > os::safe_memcpy() stuff and unified all the ucontext_t coding locally
> in
> >> > debug.cpp. That, at the costs of some very small #ifdefs, greatly
> >> > reduces
> >> > the chaff this patch causes (all the copy_ucontext.. functions we had
> in
> >> > the os_cpu files before).
> >> >
> >> > Thanks, Thomas
> >
> >
>