RFR(XS): Provide information when hitting a HaltNode for architectures other than x86

[Ningsheng Jian]

Hi Xin,

Martin's review comments reminds me that we should worry about code size 
increase on AArch64 with your patch, given that the HaltNode will be 
generated in many cases now.

Currently in AArch64 MacroAssembler::stop(), it will generate many 
register saving instructions by pusha() before calling to debug64(). But 
I think debug64() only uses the regs[] and pc arguments when 
ShowMessageBoxOnError is on. Maybe we should at least only do the saving 
and pc arg passing when ShowMessageBoxOnError is on in 
MacroAssembler::stop(), as what x86 does in macroAssembler_x86.cpp?


Thanks,
Ningsheng

On 5/5/20 7:26 AM, Liu, Xin wrote:
> Hi, Martin,
> 
> Thank you to review it and build it on s390 and PPC!
> 
> If I delete size(4) in ppc.ad,  hotspot can work out the correct size of instruction chunk, can't it?
> I found most of instructions in ppc.ad have size(xx), but there're a couple of exceptions:  cacheWB & cacheWBPreSync.
> 
> I think it should be in product hotspot.  Here are my arguments.
> 1.  Crash reports of release build are more common.
> Some customers even don't bother trying again with a debug build.
> 
> Let me take the crash report on aarch64 as an example. I paste the comparison before and after.
> https://bugs.openjdk.java.net/browse/JDK-8230552?focusedCommentId=14334977&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14334977
> 
> Without stop(_halt_reason), what we know is "there's a bug in C2-generated code". If the method is big,  which is very likely because of inlining, it's easy to get lost.
> 
> I feel it's more helpful with the patch. We can locate which HaltNode it is by searching _halt_reason in codebase.
> Hopefully, we can find the culprit method from "Compilation events".
> 
> 2. HaltNode is rarely generated and will be removed if it's dead.
> IMHO, the semantic of that Node is "halt". If It remains after optimizer or lowering to mach-nodes,  something wrong and unrecoverable happened in the compilers. After we fix the compiler bug,  it should be gone.  That's is too say, it shouldn't cause any problem about code size in ideal cases.
> 
> In reality, I observe that a HaltNode always follows the uncommon_trap call.  Christian also observed that in JDK-8022574.
> Isn't uncommon trap a one-way ticket for all architectures? I feel the control flow never returns after uncommon_trap, why do we generate a HaltNode after that? Nevertheless, it's a separated issue.
> 
> Let me make another revision to fix PPC and I found that sparc.ad is gonna gone.
> 
> Thanks,
> --lx
> 
> 
> On 5/4/20, 1:47 PM, "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 lx,
> 
>      the size attribute is wrong on PPC64 (stop is larger than 4 Bytes). S390 builds fine.
>      I've only run the build. No tests.
> 
>      Should this feature be debug-only?
>      Do we want the lengthy code emitted in product build?
> 
>      Best regards,
>      Martin
> 
> 
>      > -----Original Message-----
>      > From: hotspot-compiler-dev <hotspot-compiler-dev-
>      > bounces at openjdk.java.net> On Behalf Of Liu, Xin
>      > Sent: Donnerstag, 30. April 2020 06:03
>      > To: hotspot-compiler-dev at openjdk.java.net
>      > Subject: RFR(XS): Provide information when hitting a HaltNode for
>      > architectures other than x86
>      >
>      > Hi,
>      >
>      > Could you review this small patch?  It unifies codegen of HaltNode for other
>      > architectures.
>      > JBS: https://bugs.openjdk.java.net/browse/JDK-8230552
>      > Webrev: https://cr.openjdk.java.net/~xliu/8230552/00/webrev/
>      >
>      > I tested on aarch64.  It generates the same crash report as x86_64 when it
>      > does hit HaltNode.  Halt reason is displayed. I paste report on the JBS.
>      > I ran hotspot:tier1 on aarch64 fastdebug build.  It passed except for 3
>      > relevant failures[1].
>      >
>      > I plan to do that on aarch64 only, but it’s trivial on other architectures, so I
>      > bravely modified them all.  May I invite s390, SPARC arm32 maintainers take a
>      > look at it?
>      > If it goes through the review, I hope a sponsor can help me to push the
>      > submit repo and see if it works.
>      >
>      > [1] those 3 tests failed on aarch64 with/without my changes.
>      > gc/shenandoah/mxbeans/TestChurnNotifications.java#id2
>      > gc/shenandoah/mxbeans/TestChurnNotifications.java#id1
>      > gc/shenandoah/mxbeans/TestPauseNotifications.java#id1
>      >
>      > thanks,
>      > -lx
>      >
> 
>