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

[Liu, Xin]

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
    >