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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(s): 8191101: Show register content in hs-err file on assert (second version)
From:       Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date:       2018-03-23 15:06:40
Message-ID: CAA-vtUz5TT=FK5tXABO9NmuuChwb2DkO3bKyXZDO82=WhFmrjw () mail ! gmail ! com
[Download RAW message or body]

Great thanks! Will push next week.

Best, Thomas

On Fri 23. Mar 2018 at 15:21, Volker Simonis <volker.simonis@gmail.com>
wrote:

> Hi Thomas,
>
> thanks for following my suggestions :)
>
> The only thing I found is that you now don't need the semicolon in the
> definition of TOUCH_ASSERT_POISON any more.
>
> But there's no need for a new webrev. The change looks good - thumbs up
> from me!
>
> Regards,
> Volker
>
>
> On Thu, Mar 22, 2018 at 9:20 PM, Thomas Stüfe <thomas.stuefe@gmail.com>
> wrote:
> > Hi Volker,
> >
> > thanks! Thorough review as usual :) - see remarks inline.
> >
> > On Thu, Mar 22, 2018 at 6:36 PM, Volker Simonis <
> volker.simonis@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@gmail.com>
> >> wrote:
> >> > Thank you Christoph!
> >> >
> >> > Here the new webrev:
> >> >
> >> >
> >> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8191101-show-registers-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@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/ShowRegistersOnAssertTest.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@openjdk.java.net] On Behalf Of Thomas Stüfe
> >> >> > Sent: Dienstag, 20. März 2018 15:12
> >> >> > To: Hotspot dev runtime <hotspot-runtime-dev@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
> >> >
> >> >
> >
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic