RFR(s): 8144219: [posix] Remove redundant code around os::print_siginfo()

[David Holmes]

Hi Thomas,

Sorry for the delay.

On 30/11/2015 8:03 PM, Thomas Stüfe wrote:
> Hi all,
>
> please review this small change:

Maybe not so small :)

> Webrev:
> http://cr.openjdk.java.net/~stuefe/webrevs/8144219-posix-os-print_siginfo/webrev.00/webrev/
> Bug Report: https://bugs.openjdk.java.net/browse/JDK-8144219
>
> This is mainly a cleanup change for posix platforms. It gets rid of
> multiple copies of os::print_siginfo() in favour of the central
> os::Posix::print_siginfo() in os_posix.cpp.

Something seems not right. Currently vmError calls os::print_siginfo, 
and the per-posix-platform os::print_siginfo then call 
os::Posix::print_siginfo_brief. And now you have 
os::Posix::print_siginfo in os_posix.cpp, but that should be 
os::print_siginfo - no Posix namespace ??

> It also extends os::Posix::print_siginfo() to write more details about the
> received signal which stems from our experiences with various Unices over
> the time. This will improve hs-err file printout.

The old code was deliberately intended as a simple one-liner for the 
error report. But okay ....

987   os->print_cr("siginfo @ " PTR_FORMAT ":", p2i(si));

What is the value in printing the pointer to the siginfo_t ??

I personally prefer to see si_signo, si_code and si_errno to be clear 
that what is printed are the fields of the siginfo_t.

1030 #ifdef _AIX
1031   } else if (si->si_code == SI_EMPTY) {
1032     os->print_cr("  Signal may have been sent by another thread.");
1033 #endif
1034 #ifdef __linux
1035   } else if (si->si_code == SI_TKILL) {
1036     os->print_cr("  Signal has been sent by another thread.");
1037 #endif
1038 #ifdef __sun
1039   } else if (si->si_code == SI_LWP) {
1040     os->print_cr("  Signal has been sent by another thread.");
1041 #endif
1042   } else if (sig == SIGSEGV || sig == SIGBUS) {
1043     os->print_cr("  Address of faulting memory reference: " 
PTR_FORMAT, p2i(si->si_addr));
1044   } else if (sig == SIGILL || sig == SIGFPE) {
1045     os->print_cr("  Address of faulting instruction: " PTR_FORMAT, 
p2i(si->si_addr));
1046   } else if (sig == SIGTRAP) {
1047     os->print_cr("  Address of trap instruction: " PTR_FORMAT, 
p2i(si->si_addr));
1048 #ifndef __APPLE__
1049   } else if (sig == SIGPOLL) {
1050     os->print_cr("  Band event: " PTR64_FORMAT, (uint64_t)si->si_band);
1051 #endif

These should be conditional on the specific signal or value being 
defined, not platform specific. So the __APPLE__  goes back to SIGPOLL 
(even if it is always defined you'll never receive it on platforms that 
don't use it). The others should check SI_EMPTY etc.

Note that there are a number of other possibilities just on Solaris:

https://docs.oracle.com/cd/E23823_01/html/816-5173/siginfo-3head.html

That said we don't need to give this much detail in my opinion. If you 
get a si_code of SI_LWP for example then you can go an look at the 
manpage to find out exactly what it means - we don't want to be 
maintaining that information here. So I'm not sure we really need/want 
this expanded level of detail.


> Note that there was on all platforms code which examines the crash address
> for a possible CDS fault and prints a specialized hint. In my opinion that
> makes no sense in a general os::print_siginfo() function and I moved it to
> a separate error reporting step into vmError.cpp. This may be a point of
> contention because the way to extract the fault address from a
> siginfo/EXCEPTION_RECORD structure is platform dependend, but imho this is
> still the least ugly solution.

Sorry I don't like seeing the _WIN32 condition in the vmError.cpp code. 
By keeping this as part of the print_siginfo the platform specific 
aspect is handled transparently. As something that is only used as part 
of error reporting, and which is directly related to a signal being 
received the current placement doesn't seem unreasonable. If you really 
want to factor this out then I would add 
os::check_for_failing_cds_access() or something like that.

Thanks,
David
-----

> Kind regards, Thomas
>