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

[Doerr, Martin]

Hi Xin,

no problem. As already mentioned, your review is appreciated. I'll wait for a Reviewer's review, of course.

I think JDK-8230552 will be nice after JDK-8022574 and JDK-8244949.
Thanks for doing it and for your support.

Best regards,
Martin


> -----Original Message-----
> From: Liu, Xin <xxinliu at amazon.com>
> Sent: Freitag, 22. Mai 2020 11:25
> 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,
> 
> Thank you for the explanation.
> 
> You are right. Sorry I didn't notice the detail_msg = NULL for
> stop_shouldnotreachhere at first place.
> Your implementation is good to me.  I am not a reviewer, still need
> reviewers.
> 
> Thanks,
> --lx
> 
> 
> On 5/22/20, 1:49 AM, "Doerr, Martin" <martin.doerr at sap.com> wrote:
> 
>     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 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
>     >
>