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

Langer, Christoph christoph.langer at sap.com
Wed Mar 21 08:51:22 UTC 2018


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


More information about the hotspot-runtime-dev mailing list