RFR(M): 8244949: [PPC64] Reengineer assembler stop function
Schmidt, Lutz
lutz.schmidt at sap.com
Mon May 25 16:29:36 UTC 2020
Hi Martin,
after having had a thorough look, your change looks good to me.
Thank you for doing the cleanup and streamlining the implementation.
Best regards,
Lutz
On 22.05.20, 12:07, "Doerr, Martin" <martin.doerr at sap.com> wrote:
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
> >
>
More information about the hotspot-runtime-dev
mailing list