RFR: 8166188: G1 Needs pre barrier on dereference of weak JNI handles

[dean.long at oracle.com]

Hi Kim.  I took a look at the sharedRuntime_<arch> and templateInterpreterGenerator_<arch> for arm, sparc, and x86.

In sharedRuntime_arm.cpp, I suggest simplifying:

1750 #ifdef AARCH64
1751 __ cbz(R0, done); // Use NULL as-is.
1752 STATIC_ASSERT(JNIHandles::weak_tag_mask == 1u);
1753 __ tbz(R0, 0, not_weak); // Test for jweak tag.
1754 #else // !AARCH64
1755     __ cmp(R0, 0);
1756 __ b(done, eq); // Use NULL as-is.
1757 __ tst(R0, JNIHandles::weak_tag_mask); // Test for jweak tag.
1758 __ b(not_weak, eq);
1759 #endif // !AARCH64

to the equivalent:

1751 __ cbz(R0, done); // Use NULL as-is.
1752 STATIC_ASSERT(JNIHandles::weak_tag_mask == 1u);
1753 __ tbz(R0, 0, not_weak); // Test for jweak tag.

as cbz and tbz expand to cmp/b and tst/b on arm32.  The others look good.

dl

On 2/6/17 5:03 PM, Kim Barrett wrote:
> Apologies for taking so long to respond to earlier comments.  I've
> been distracted by some other tasks, and it took a while to figure out
> what to do about the problem Mikael reported.
>
> Martin Doerr - Thanks for the ppc and s390 native call generator
> modifications.  Do you want a contributed-by credit when pushed?
>
> Andrew Haley - Thanks for the syntax fix.  The new version with always
> tagged jweaks (see below) makes testing easier, and there's a new test
> included.  Testing the G1-specific behavior still needs the whitebox
> extension.  I've updated the G1-specific test (which is still not part
> of the changeset for JDK 9).
>
> Per Liden - I tried your suggestion of making jweak unconditionally
> tagged.  It doesn't eliminate much of the INCLUDE_ALL_GCS
> conditionals, since those are still needed to protect references to
> G1-specific code.  (The littering of otherwise generic runtime code
> with this G1-specific stuff should be a cleanup task for JDK 10.)
> However, it does simplify some of the code, and simplifies testing.
> The cost is a "bit-test and (predicted taken) conditional branch" in
> the handling of an oop result from a native call.  I agree that's
> unlikely to be noticable, and the benefits seem worthwhile, so I've
> made that change.
>
> I've attempted to update the platform-specific native call return
> handling changes for all of the platforms, including those I don't
> have access to (aarch64, ppc, s390).  I also fixed the zero change I'd
> made earlier.  And I made shark error, to force anyone using it to fix
> the handling of an oop result by SharkNativeWrapper::initialize().
> There might be other zero/shark changes that I've missed.  All of
> these should be checked by appropriate platform experts.
>
> I had to fix a JVMTI bug as part of this; FollowReferences wasn't
> doing as much argument checking as expected.  This was unconvered by a
> closed test that unexpectedly asserted.  This test would have
> similarly asserted previously if CheckJNICalls was true; I removed
> that conditionalization in JNIHandles::resolve, as it was only there
> to avoid an expensive check for the jobject being a jweak, and in the
> new code we already know it isn't.  There might be other JVMTI
> functions with similar issues.
>
> Mikael - There are some ugly worms lurking under that rock!  I think
> I've dealt with the problem, tripping over some other bugs along the
> way.  Thanks for the test case.  I tweaked it a bit and included it in
> the updated changeset.
>
> The passing of arguments involves collecting them into a
> JavaCallArguments object.  oop arguments are in some handlized form
> (either jobject or Handle).  The existing code heavily cheats; for
> example, it assumes one can correctly cast from a jobject to an oop*
> and dereference that pointer.  Obviously, a tagged jweak breaks that
> assumption.  Even worse, there are casts from oop* to oop and back!
> (These are so nasty they *must* use C-style cast syntax.) I'm thinking
> about some followup cleanup bugs in that area.
>
> To address this, I changed JavaCallArguments to avoid conversions
> from jobject to Handle/oop*.  To support that, it now keeps more
> accurate track of the state of the object references in the collected
> arguments.  It also delays resolving jobjects to their designated
> oops, avoiding intermediate Handle objects.  (This was a place where
> cheating was going on, in order to avoid intermediate Handle
> allocations.  We're now accomplishing that without crashing through
> the JNIHandles abstraction.)
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8166188
>
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04
> incr: http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04.inc
> The full webrev may be more useful than the incremental; some of the
> incremental changes are pretty hard to read.
>
> Some additional webrevs that might be helpful:
> http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04.other-platforms
> Incremental changes for aarch64, ppc, and s390 supplied by Andrew and
> Martin.
>
> http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04.always_tag[.full]
> Incremental [full] changes to always tag jweaks.
> The full webrev may be more useful than the incremental; some of the
> incremental changes are pretty hard to read.
>
> http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04.java_call
> Incremental changes to address the java call problem found by Mikael.
>
> hotspot.04 == hotspot.02 + hotspot.04.inc
> hotspot.04 == hotspot.04.always_tag.full + hotspot.04.java_call
> hotspot.04 == hotspot.02 + hotspot.04.other-platforms +
>                hotspot.04.always_tag + hotspot.04.java_call
>
> The updated G1-specific test requiring the whitebox extension is
> http://cr.openjdk.java.net/~kbarrett/8166188/test.04
>
> The whitebox extension has not changed:
> http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hsroot.01/
> http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hotspot.01/
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20170207/3680bd06/attachment.html>