RFR(M): 8244949: [PPC64] Reengineer assembler stop function
Doerr, Martin
martin.doerr at sap.com
Mon May 25 16:45:11 UTC 2020
Hi Lutz and Xin,
thanks a lot for the reviews. Pushed to jdk/jdk.
@Xin: Feel free to proceed with JDK-8230552 and thanks again for waiting. (Rebasing with manual resolution should be trivial and I don't need to see another webrev for that.)
Best regards,
Martin
> -----Original Message-----
> From: Schmidt, Lutz <lutz.schmidt at sap.com>
> Sent: Montag, 25. Mai 2020 18:30
> To: Doerr, Martin <martin.doerr at sap.com>; Liu, Xin <xxinliu at amazon.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>
> Subject: Re: RFR(M): 8244949: [PPC64] Reengineer assembler stop function
>
> 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