RFR(S): 8205609: [PPC64] Fix PPC64 part of 8010319 and TLH without UseSIGTRAP on AIX

[Doerr, Martin]

Hi David and Götz,

thanks for reviewing.
Yes, I think it would be good to add tests for -XX:-UseSIGTRAP when we find some time. Not only for safepoint polls, SIGTRAP is also used for null and range checks for example.

Best regards,
Martin


-----Original Message-----
From: Lindenmaier, Goetz 
Sent: Mittwoch, 27. Juni 2018 11:27
To: Doerr, Martin <martin.doerr at sap.com>; David Holmes <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net
Subject: RE: RFR(S): 8205609: [PPC64] Fix PPC64 part of 8010319 and TLH without UseSIGTRAP on AIX

Hi Martin, 
 
the change looks good. 
I would appreciate a test with -XX:-UseSIGTRAP.
I guess you can run any test with that flag, maybe with a change like the one below.
Maybe you want to do a follow up with the test change. 

Best regards,
  Goetz.

diff -r 0358dad944c7 test/hotspot/jtreg/runtime/handshake/HandshakeWalkStackTest.java
--- a/test/hotspot/jtreg/runtime/handshake/HandshakeWalkStackTest.java	Wed Jun 27 12:56:21 2018 +0530
+++ b/test/hotspot/jtreg/runtime/handshake/HandshakeWalkStackTest.java	Wed Jun 27 11:17:54 2018 +0200
@@ -22,7 +22,7 @@
  *
  */
 
-/*
+/**
  * @test HandshakeWalkStackTest
  * @library /testlibrary /test/lib
  * @build HandshakeWalkStackTest
@@ -31,6 +31,16 @@
  * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI HandshakeWalkStackTest
  */
 
+/**
+ * @test HandshakeWalkStackTest
+ * @library /testlibrary /test/lib
+ * @requires vm.simpleArch == "ppc64" | vm.simpleArch == "ppc64le"
+ * @build HandshakeWalkStackTest
+ * @run driver ClassFileInstaller sun.hotspot.WhiteBox
+ *                              sun.hotspot.WhiteBox$WhiteBoxPermission
+ * @run main/othervm -Xbootclasspath/a:. -XX:-UseSIGTRAP -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI HandshakeWalkStackTest
+ */
+
 import jdk.test.lib.Asserts;
 import sun.hotspot.WhiteBox;


> -----Original Message-----
> From: Doerr, Martin
> Sent: Dienstag, 26. Juni 2018 13:02
> To: David Holmes <david.holmes at oracle.com>; hotspot-runtime-
> dev at openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Subject: RE: RFR(S): 8205609: [PPC64] Fix PPC64 part of 8010319 and TLH
> without UseSIGTRAP on AIX
> 
> Hi David,
> 
> indeed, this is very confusing. The constant pool cache contents were
> changed for "Better interface invocations" so the register names don't fit to
> the usage in invokeinterface anymore (on all platforms).
> 
> You're right, Rmethod should be used. I had copied the wrong name.
> 
> I just noticed that the required tests were not run before I had created the
> webrev. I'll run them with the new webrev.
> 
> New webrev using Rmethod + improved comments:
> http://cr.openjdk.java.net/~mdoerr/8205609_ppc64_TI_and_TLH_fixes/we
> brev.01/
> 
> Thank you and best regards,
> Martin
> 
> 
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Dienstag, 26. Juni 2018 04:11
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-
> dev at openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Subject: Re: RFR(S): 8205609: [PPC64] Fix PPC64 part of 8010319 and TLH
> without UseSIGTRAP on AIX
> 
> Hi Martin,
> 
> On 26/06/2018 12:34 AM, Doerr, Martin wrote:
> > Hi,
> >
> > I'd like to fix issues with the current PPC64 implementation:
> >
> >    *   Wrong register usage in template interpreter after JDK-8010319.
> 
> Well that's disappointing - I thought it had all been pre-tested.
> 
> This code is quite confusing. We have:
> 
> 3584   prepare_invoke(byte_no, Rinterface_klass, Rret_addr, Rmethod,
> Rreceiver, Rflags, Rscratch1);
> 
> but the definition of prepare_invoke is:
> 
> 3332 void TemplateTable::prepare_invoke(int byte_no,
> 3333               Register Rmethod,  // linked method (or i-klass)
> 3334               Register Rret_addr,// return address
> 3335               Register Rindex,   // itable index, MethodType, etc.
> 3336               Register Rrecv,    // If caller wants to see it.
> 3337               Register Rflags,   // If caller wants to test it.
> 3338               Register Rscratch
> 3339                                    ) {
> 
> So this passes Rinterface_klass where Rmethod is expected; and Rmethod
> where Rindex is expected. So in that sense I think the register content
> will be correct but the name seems completely wrong! ???
> 
> Cheers,
> David
> 
> >    *   SafepointMechanism implementation for AIX misses the protection of
> the bad page (ThreadLocalHandshakes with -XX:-UseSIGTRAP). I prefer to
> implement it using 2 pages like on the other platforms.
> >    *   Remove unused function which is obsolete since the introduction of
> ThreadLocalHanshakes.
> >
> > Please review:
> >
> http://cr.openjdk.java.net/~mdoerr/8205609_ppc64_TI_and_TLH_fixes/we
> brev.00/
> >
> > Thanks and best regards,
> > Martin
> >