February 2017 - s390x-port-dev

RFR(10): 8172020: Internal Error (cpu/arm/vm/frame_arm.cpp:571): assert(obj == __null || Universe::heap()->is_in(obj)) failed: sanity check #
by Chris Plummer 27 Feb '17

27 Feb '17

Hello, Please review the following: http://cr.openjdk.java.net/~cjplummer/8172020/webrev.01/webrev.hotspot/ https://bugs.openjdk.java.net/browse/JDK-8172020 [Note there are s390, ppc, and aarch64 changes in this bug fix that I will need to have someone verify for me or I won't be pushing them.] This bug turns up with a closed test so the CR is marked confidential. I'll try to provided the necessary details here. Sorry the explanation is lengthy, but it is a complex bug (with a relatively simple fix). If you are on the s390, ppc, or aarch64 teams and just want to help verify the fix, I would say it's not necessary to understand the details of the bug. What's most important is you test the changes. Unfortunately you won't be able to reproduce the issue since the test is closed, but you should at least verify the fix builds and introduces no new issues. I did test the open aarch64 port since I have the hardware, and someone was kind enough to provide me with binaries both without the patch (which reproduced the bug) and with the patch (which passed). No other aarch64 testing was done. Now on to an explanation of the bug and the fix: The test is testing the JDI/JVMTI forceEarlyReturn support. After the debugger side spawns the debugee process and the debugee thread is created, a vm.suspend() is issued to suspend all threads on the debugee side. The debugger side then issues a JDI forceEarlyReturn, which will cause the debugee thread to exit the currently executing method. Although the intent is to force exit of the run() method, the run() method does make some method calls to some very small methods with "inline" in their name, such as inlineMethodReturningObject(). If forceEarlyReturn is done while in one of them, JDI throws InvalidTargetException, which the test correctly handles and will pass if that happens. If the run() method is not yet compiled, what *normally* happens to force the thread to suspend is to swap in the interpreter safepoint dispatch table. This will result in a call_VM out to the interpreter to handle the safepointing. When later the thread is resumed (after initiating the forceEarlyReturn), there is code on the return side of call_VM to make sure the earlyret is handled immediately before any other bytecodes are exectuted. Thus the method that was executing at the time of the forceEarlyReturn should be the same as the method when the earlyret is handled. In fact it should be the same bytetcode(bci). Like I said, this is what *normally* happens, but is not the way the bug is exposed. The bug turns up when the run() method is not yet compiled, and when the suspend is attempted we are in one of the small "inline" methods, and it is compiled. In this case thread suspension is achieved in a different manner. When a compiled method executes its return code, it first pops its frame, and then the " poll_return" safepoint polling instruction is executed. This allows the thread to be suspended. We are technically in the run() method at this point since the frame of the "inline" method has been popped. Therefore the forceEarlyReturn is allowed. When the thread is resumed, the return from the "inline" method finally happens, which puts us in code generated by TemplateInterpreterGenerator::generate_return_entry_for(). However, there are no checks for an earlyret here, so we start executing bytecodes until we finally hit an earlyret check. This happens when the next invokespecial is done. There is a call_VM after the frame is pushed, and the call_VM always does an earlyret check. The problem is now the topmost frame (in the case where it asserts) is inlineMethodReturningObject(). Since it returns an object type, the earlyret code expects there to be an oop in the earlyret slot on the stack. But there isn't because the forceEarlyReturn was done on the void run() method. So the earlyret code fetches garbage, does an validity test on it, and gets the assert you see in the bug title. So what is needed here is an earlyret check after the compiled "inline" method returns, and before any new bytecodes are executed. Since TemplateInterpreterGenerator::generate_return_entry_for() is always executed when the compiled "inline" method returns, the check was added here. For completeness, the popframe check was also added. The header file changes are just to make these methods public so they can be called. I tested the failed test at least 100 times on each supported platform. The assert usually showed up at least 1 in 50 times. I also ran a large set of tests on all supported platforms, including everything we do for nightly testing except for some of the longest running tests. thanks, Chris

3 4

RFR (XS) 8175267: [s390] cleanup stub code "handler_for_unsafe_access"
by Schmidt, Lutz 22 Feb '17

22 Feb '17

Hi all, May I kindly request reviews for my very small cleanup change? Further down the road, I would need a sponsor, too. Bug: https://bugs.openjdk.java.net/browse/JDK-8175267 Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8175267/ Description: The "handler_for_unsafe_access” is a relic from former times, having survived only on s390x. The stub is generated, but never used (called). This change removes everything related. Thanks, Lutz

4 5

URGENT: RFR: [BACKOUT] fix for JDK-8166188
by Daniel D. Daugherty 17 Feb '17

17 Feb '17

Greetings, Sending this backout code review to the same aliases used for: JDK-8166188 G1 Needs pre barrier on dereference of weak JNI handles https://bugs.openjdk.java.net/browse/JDK-8166188 more as a heads up that JDK-8166188 might be getting backed out. I only need reviews from Kim and Jesper. Doing a contingent backout of Kim's fix for the above due to some unexpected test failures with some JDK tests: JDK-8175085 8166188 causes JDK test failures https://bugs.openjdk.java.net/browse/JDK-8175085 JDK-8175086 [BACKOUT] fix for JDK-8166188 https://bugs.openjdk.java.net/browse/JDK-8175086 Webrev URL: http://cr.openjdk.java.net/~dcubed/8175086-webrev/0-jdk9-hs/ I used "hg backout 3bc68ff68250" which created a child changeset of Kim's: changeset: 12639:3bc68ff68250 user: kbarrett date: Wed Feb 15 22:19:13 2017 -0500 summary: 8166188: G1 Needs pre barrier on dereference of weak JNI handles The backout changeset: changeset: 12644:ee34d6aabf5f parent: 12639:3bc68ff68250 user: dcubed date: Thu Feb 16 10:41:19 2017 -0800 summary: 8175086: [BACKOUT] fix for JDK-8166188 and then I used "hg merge" to merge with current tip. This was a "branch" merge with no conflicts. Sanity checks: - The parent of Kim's changeset is: changeset: 12638:35db0413819a user: roland date: Wed Feb 15 17:26:37 2017 -0800 summary: 8174164: SafePointNode::_replaced_nodes breaks with irreducible loops - hg diff -r 35db0413819a -r ee34d6aabf5f shows no output so the backout is clean. - $ wc -l files.list 37 files.list - $ hg diff -r 35db0413819a -r tip `cat files.list` shows no output so the merge with the tip is clean. Thanks, in advance, for reviews from Jesper and Kim!! Dan

4 5

RFR: 8166188: G1 Needs pre barrier on dereference of weak JNI handles
by Kim Barrett 15 Feb '17

15 Feb '17

Please review this change to jweak to support G1. The problem being addressed is that, when using G1, dereferencing a jweak must ensure the obtained referent is ensured live, just as for other weak reference types. This has always been a requirement, and failure to do so appears to be a since-day1 G1 bug. There are two categories of places that need to address this issue: JNIHandles::resolve and friends, and returning a jobject from native code to Java. In both of these places the jobject whose contained oop is being extracted might really be a jweak. jweak is representationally indistinguishable from jobject; jweak is just a typedef for jobject. The documentation says a jweak can be used anywhere a jobject is permitted, making that type equivalence necessary for a C API. (A C++ API might be able to have distinct types via inheritance, though would still need a method for distinguishing them at runtime. But since JNI is a C API...). The basic approach being taken is to "tag" jweaks by setting the low order bit (recall that jweak == jobject == _jobject*), in order to distinguish them from non-jweak jobjects. This tagging is only being done when the selected GC needs the distinction, e.g. G1. This gives us a representational difference between jobject and jweak on which to base the different behavior, hiding that distinction behind the existing API. JNIHandles::resolve and friends are changed to unconditionally strip off any potential weak tag when translating from jobject to to oop*. That's cheaper than testing for the conditional use of tagging and then stripping. Also stripped when deleting JNI handles. TemplateInterpreterGenerator::generate_native_entry and SharedRuntime::generate_native_wrapper are changed to conditionally emit code to apply the G1 pre-barrier to the value obtained from a jobject result, when the result is tagged as a jweak, which only occurs when using G1. For arm32/arm64, this required moving the g1_write_barrier_pre definitions from InterpreterMacroAssembler to MacroAssembler. Also moved g1_write_barrier_post, for consistency. Note: I've not tested the aarch64 changes; I'll need someone with access to that platform to test. Note: I've not implemented this change for ppc or s390; I'll need someone else to deal with those platforms. (It's been a very long time since I wrote any ppc assembly code, and I've never seen an s390.) Note: I've done a minimal change for zero. Code elsewhere suggests zero doesn't support G1, and I followed that. And finally, JNIHandles::make_weak_global conditionally tags the result. CR: https://bugs.openjdk.java.net/browse/JDK-8166188 Webrev: http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.02/ In addition to the above webrev, I've also included separate webrevs for a test of this change, and for a whitebox extension needed for that test. The whitebox extension is my in-progress work on JDK-8169517, which is an enhancement targeted for JDK 10, and is not part of this change. Since the test depends on that whitebox extension, the test can't be included with the change either, and will be deferred to JDK 10. But I'm including webrevs for both here, for for reference by reviewers, and for testing on the platforms I don't have access to. http://cr.openjdk.java.net/~kbarrett/8166188/test.03/ http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hsroot.01/ http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hotspot.01/ Testing: All hotspot jtreg tests run locally (Linux x86_64). In particular: - gc/TestReturnJNIWeak (new) - runtime/SameObject (uses NewWeakGlobalRef) Ad hoc hotspot nightly test on Oracle supported platforms. This includes some closed tests that use NewWeakGlobalRef.

8 26