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

[Volker Simonis]

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;

- 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');

- 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.

debug.cpp

- The "ifdef WIN32" is unnecessary if we don't support Windows anyway
 #ifdef _WIN32


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

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.

Thanks for finally bringing this into OpenJDK!
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
>
>