RFR(M): 8244949: [PPC64] Reengineer assembler stop function

[Doerr, Martin]

Hi Xin,

thanks a lot for reviewing it in detail.

Right, R0 is random when using should_not_reach_here().
(should_not_reach_here was already designed without support for a message on PPC64. I didn't want to change that. It is only one instruction and it preserves all registers, now.)
However, the signal handler doesn't use the random value:

detail_msg is set to NULL here:
+           case MacroAssembler::stop_shouldnotreachhere:  msg = "shouldnotreachhere"; detail_msg = NULL; break;

And replaced by a message here:
+         if (detail_msg == NULL) {
+           detail_msg = "no details provided";
+         }

print_cr and report_and_die should always get valid msg and detail_msg.

Best regards,
Martin


> -----Original Message-----
> From: Liu, Xin <xxinliu at amazon.com>
> Sent: Donnerstag, 21. Mai 2020 08:27
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-
> dev at openjdk.java.net
> Cc: Andrew Haley <aph at redhat.com>; Derek White
> <derekw at marvell.com>; Ningsheng Jian <ningsheng.jian at arm.com>;
> Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Schmidt, Lutz
> <lutz.schmidt at sap.com>
> Subject: Re: RFR(M): 8244949: [PPC64] Reengineer assembler stop function
> 
> Hi, Martin,
> 
> I have a question:
> MacroAssembler::stop() skips assigning R0 if type is
> stop_shouldnotreachhere.
> 
> But os_linux_ppc.cpp loads R0 from ucontext.
> *detail_msg = (const char*)(uc->uc_mcontext.regs->gpr[0]);
> 
> Is that possible that the R0 is a random value when the type is
> stop_shouldnotreachhere?
> Especially,  hotspot will output detail_msg as a 0-terminated string when
> TraceTraps is set.
> 
> Nit:
> Better have an extra newline before.
> +        if (detail_msg == NULL) {
> 
> 
> Thanks,
> --lx
> 
> From: "Doerr, Martin" <martin.doerr at sap.com>
> Date: Wednesday, May 20, 2020 at 10:12 AM
> To: hotspot-runtime-dev <hotspot-runtime-dev at openjdk.java.net>
> Cc: "Liu, Xin" <xxinliu at amazon.com>, Andrew Haley <aph at redhat.com>,
> Derek White <derekw at marvell.com>, Ningsheng Jian
> <ningsheng.jian at arm.com>, "Lindenmaier, Goetz"
> <goetz.lindenmaier at sap.com>, "Schmidt, Lutz" <lutz.schmidt at sap.com>
> Subject: [EXTERNAL] RFR(M): 8244949: [PPC64] Reengineer assembler stop
> function
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> Hi,
> 
> I'd like to contribute an improved and more space saving version of the
> MacroAssembler::stop function for PPC64.
> Main reason is that usage of it is planned for better error messages in
> HaltNodes (C2 compiler): https://bugs.openjdk.java.net/browse/JDK-
> 8230552
> 
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8244949
> Webrev:
> http://cr.openjdk.java.net/~mdoerr/8244949_ppc64_asm_stop/webrev.00/
> 
> Change includes cleanup:
> - Removal of stop ids which have not proven to be beneficial and are not
> available on most other platforms, either.
> - Removal of SIGTRAP based not-entrant patching (includes old PPC-only flag
> TrapBasedNotEntrantChecks which is not expected to be used in
> production). SIGILL version is already available and works fine, too. (It is
> supposed to get removed completely with:
> https://openjdk.java.net/jeps/8221828)
> 
> Tested by inserting stop/unimplemented/untested/should_not_reach_here
> in interpreter/C2/stub code.
> Prints: stop type + stop message + registers + instructions (unfortunately not
> disassembled at the moment) + nice stack trace
> 
> Tested in our nightly tests to verify SIGILL based not-entrant patching. No
> issues found.
> 
> Please review.
> 
> Best regards,
> Martin
>