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

[Volker Simonis]

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 at 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 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-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 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/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 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
>> >
>> >
>
>