Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271055: Crash during deoptimization with "assert(bb->is_reachable()) failed: getting result from unreachable basicblock" with -XX:+VerifyStack #4902

Closed
wants to merge 5 commits into from

Conversation

kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Jul 26, 2021

Hi, I'm trying to fix JDK-8271055. The root cause is that some basic blocks are not interpreted because there is no trap bytecode inside them, these basic blocks eventually become unreachable and lead to a crash. Maybe we need to coordinate compiler and hotspot groups to fix this crash.


The attached test generates the following bytecode:

  void test();
    descriptor: ()V
    flags: (0x0000)
    Code:
      stack=2, locals=3, args_size=1
         0: iconst_1
         1: istore_1
         2: iload_1
         3: bipush        100
         5: if_icmpge     29
         8: iinc          1, 1
        11: goto          2
        14: astore_2
        15: iload_1
        16: bipush        100
        18: if_icmpge     27
        21: iinc          1, 1
        24: goto          15
        27: aload_2
        28: athrow
        29: return

Javac generates some unreachable basic blocks(BCI 14 - 28), and there is no exception table for them, VM knows nothing about them. I found the same bug report at JDK-8022186 which was marked as Fixed, but I think it was not properly fixed, In some similar cases, javac cannot work well, I have filed https://bugs.openjdk.java.net/browse/JDK-8271254 for another javac bug.

Going back to this bug, the proposed change is to insert a nop in empty try-block so that javac can generate the correct exception table. Now generated bytecodes look like this:

  void test();
    descriptor: ()V
    flags: (0x0000)
    Code:
      stack=2, locals=3, args_size=1
         0: iconst_1
         1: istore_1
         2: nop
         3: iload_1
         4: bipush        100
         6: if_icmpge     30
         9: iinc          1, 1
        12: goto          3
        15: astore_2
        16: iload_1
        17: bipush        100
        19: if_icmpge     28
        22: iinc          1, 1
        25: goto          16
        28: aload_2
        29: athrow
        30: return
      Exception table:
         from    to  target type
             2     3    15   any

However, this is not the end. Even if the exception table is correctly generated, VM only enters GenerateOopMap::do_exception_edge when the bytecode may be trapped. In order to setup handler block, we need to relax this restriction to allow even bytecodes such as nop to correctly enter GenerateOopMap::do_exception_edge. Otherwise, handler block will not be interpreted, its stack is problematic and finally hits the assertion.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8271055: Crash during deoptimization with "assert(bb->is_reachable()) failed: getting result from unreachable basicblock" with -XX:+VerifyStack

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4902/head:pull/4902
$ git checkout pull/4902

Update a local copy of the PR:
$ git checkout pull/4902
$ git pull https://git.openjdk.java.net/jdk pull/4902/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4902

View PR using the GUI difftool:
$ git pr show -t 4902

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4902.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 26, 2021

👋 Welcome back yyang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jul 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 26, 2021

@kelthuzadx The following labels will be automatically applied to this pull request:

  • compiler
  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added hotspot compiler labels Jul 26, 2021
@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Jul 26, 2021

/add hotspot-runtime
/add hotspot-compiler
/remove hotspot

@openjdk
Copy link

@openjdk openjdk bot commented Jul 26, 2021

@kelthuzadx Unknown command add - for a list of valid commands use /help.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 26, 2021

@kelthuzadx Unknown command add - for a list of valid commands use /help.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Jul 26, 2021

/label add hotspot-runtime
/label add hotspot-compiler
/label remove hotspot

@openjdk
Copy link

@openjdk openjdk bot commented Jul 26, 2021

@kelthuzadx Unknown command remove - for a list of valid commands use /help.

@openjdk openjdk bot added the hotspot-runtime label Jul 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 26, 2021

@kelthuzadx
The hotspot-runtime label was successfully added.

@openjdk openjdk bot added the hotspot-compiler label Jul 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 26, 2021

@kelthuzadx
The hotspot-compiler label was successfully added.

@openjdk openjdk bot removed the hotspot label Jul 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 26, 2021

@kelthuzadx
The hotspot label was successfully removed.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 26, 2021

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented Jul 26, 2021

⚠️ @kelthuzadx This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Just wondering, did you sync up with @chhagedorn who (still) has this assigned? Not sure if he already started working on it.

@@ -784,6 +784,11 @@ JRT_LEAF(BasicType, Deoptimization::unpack_frames(JavaThread* thread, int exec_m
// a given bytecode or the state after, so we try both
if (!Bytecodes::is_invoke(cur_code) && cur_code != Bytecodes::_athrow) {
// Get expression stack size for the next bytecode
if(UseNewCode) {
Copy link
Member

@TobiHartmann TobiHartmann Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you intended to remove this from the final version of the patch.

Copy link
Member Author

@kelthuzadx kelthuzadx Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry... I haven't sync up with @chhagedorn, I picked up a starter issue to work on it and missed assignee. I will update my query condition to filter unassigned issue later.

@kelthuzadx kelthuzadx closed this Jul 26, 2021
@kelthuzadx kelthuzadx deleted the JDK-8271055 branch Jul 26, 2021
@chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Jul 26, 2021

Hi @kelthuzadx, please make sure to always have a JBS issue assigned to you before creating a PR for it. If the issue is already assigned you can do a sync with the assignee and ask to take it over.

In this case, I have not started working on it, yet, so you can take it over and reopen the PR again.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Jul 26, 2021

Thank you @chhagedorn! I will take care of this later.

@kelthuzadx kelthuzadx restored the JDK-8271055 branch Jul 26, 2021
@kelthuzadx kelthuzadx reopened this Jul 26, 2021
@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Jul 26, 2021

P.S. In summary, I think there are two bugs: one for javac, one for VM.

-- Javac side:
The proposed fix is to insert a "nop".

I'm not good at this area, hope to have more experts' comments.

--- VM side:
To verify what I said, we can consider the below case which does not contain trap bytecodes in some unreachable basic blocks:


public class VerifyStackWithUnreachableBlock {
    public static void main(String[] strArr) {
        VerifyStackWithUnreachableBlock _instance = new VerifyStackWithUnreachableBlock();
        for (int i = 0; i < 10000; i++) {
            _instance.test();
        }
    }

    void test() {
        int i8 = 1;
        try {
            int p=0;
            p++;
        } finally {
            for (; i8 < 100; i8++) {
            }
        }
    }
}

It crashes with the same assertion:

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/home/qingfeng.yy/jdktip/src/hotspot/share/oops/generateOopMap.cpp:2220), pid=130592, tid=130631
#  assert(bb->is_reachable()) failed: getting result from unreachable basicblock
#
# JRE version: OpenJDK Runtime Environment (18.0) (slowdebug build 18-internal+0-adhoc.qingfengyy.jdktip)
# Java VM: OpenJDK 64-Bit Server VM (slowdebug 18-internal+0-adhoc.qingfengyy.jdktip, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0xa505b1]  GenerateOopMap::result_for_basicblock(int)+0xb5
#
# No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
#

---------------  S U M M A R Y ------------

Command Line: -Dtest.vm.opts= -Dtest.tool.vm.opts= -Dtest.compiler.opts= -Dtest.java.opts= -Dtest.jdk=/home/qingfeng.yy/jdktip/build/linux-x86_64-server-slowdebug/images/jdk -Dcompile.jdk=/home/qingfeng.yy/jdktip/build/linux-x86_64-server-slowdebug/images/jdk -Dtest.timeout.factor=1.0 -Dtest.root=/home/qingfeng.yy/jdktip/test/hotspot/jtreg -Dtest.name=compiler/interpreter/VerifyStackWithUnreachableBlock.java -Dtest.file=/home/qingfeng.yy/jdktip/test/hotspot/jtreg/compiler/interpreter/VerifyStackWithUnreachableBlock.java -Dtest.src=/home/qingfeng.yy/jdktip/test/hotspot/jtreg/compiler/interpreter -Dtest.src.path=/home/qingfeng.yy/jdktip/test/hotspot/jtreg/compiler/interpreter -Dtest.classes=/home/qingfeng.yy/jdktip/JTwork/classes/0/compiler/interpreter/VerifyStackWithUnreachableBlock.d -Dtest.class.path=/home/qingfeng.yy/jdktip/JTwork/classes/0/compiler/interpreter/VerifyStackWithUnreachableBlock.d -XX:+IgnoreUnrecognizedVMOptions -XX:+VerifyStack com.sun.javatest.regtest.agent.MainWrapper /home/qingfeng.yy/jdktip/JTwork/compiler/interpreter/VerifyStackWithUnreachableBlock.d/main.0.jta

Host: e69e13043.et15sqa, Intel(R) Xeon(R) Platinum 8163 CPU @ 2.50GHz, 96 cores, 503G, Alibaba Group Enterprise Linux Server release 7.2 (Paladin)
Time: Mon Jul 26 22:46:38 2021 CST elapsed time: 1.894403 seconds (0d 0h 0m 1s)

---------------  T H R E A D  ---------------

Current thread (0x00007f415c3a5ab0):  JavaThread "MainThread" [_thread_in_Java, id=130631, stack(0x00007f40cd9bc000,0x00007f40cdabd000)]

Stack: [0x00007f40cd9bc000,0x00007f40cdabd000],  sp=0x00007f40cdab8c50,  free space=1011k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0xa505b1]  GenerateOopMap::result_for_basicblock(int)+0xb5
V  [libjvm.so+0xfa41b3]  OopMapForCacheEntry::compute_map(Thread*)+0x155
V  [libjvm.so+0xfa4d23]  OopMapCacheEntry::fill(methodHandle const&, int)+0xdd
V  [libjvm.so+0xfa5b4b]  OopMapCache::compute_one_oop_map(methodHandle const&, int, InterpreterOopMap*)+0x4d
V  [libjvm.so+0x83ef0e]  Deoptimization::unpack_frames(JavaThread*, int)+0x642
v  ~DeoptimizationBlob
....

After applying this patch, it works.


pre-submit tests failed due to change about javac. I will fix them if changes involving javac in this PR is reviewed.

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Aug 10, 2021

HotSpot and javac should be fixed independently, please file a new bug for javac.

Regarding the change to GenerateOopMap::do_exception_edge, is that sufficient to fix the bug even without the change to javac? Because HotSpot should also handle bytecode not generated by javac.

The fix looks reasonable to me but someone from runtime who knows that code better should have a look.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Aug 10, 2021

HotSpot and javac should be fixed independently, please file a new bug for javac.

Regarding the change to GenerateOopMap::do_exception_edge, is that sufficient to fix the bug even without the change to javac? Because HotSpot should also handle bytecode not generated by javac.

The fix looks reasonable to me but someone from runtime who knows that code better should have a look.

Hi Tobias,

It's not sufficient to fix this bug without javac changes. But it reveals another crash that I posted above. Maybe I should file a new issue for that and fix it? Later when javac bug is solved, this bug can also be set to resolve.

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Aug 10, 2021

Okay but then the fix is incomplete because the VM also needs to handle bytecode not generated by javac. Other compilers or hand written bytecode might still miss these traps.Of course, if the bytecode violates the Spec, the verifier should catch this but otherwise the VM should not crash or assert.

Maybe I should file a new issue for that and fix it?

I think it's just a different failure mode and should be fixed with this bug.

Later when javac bug is solved, this bug can also be set to resolve.

No, as explained above, other compilers could still generate this bytecode shape. We need to handle it properly in the VM.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Aug 10, 2021

Okay but then the fix is incomplete because the VM also needs to handle bytecode not generated by javac. Other compilers or hand written bytecode might still miss these traps.Of course, if the bytecode violates the Spec, the verifier should catch this but otherwise the VM should not crash or assert.

Maybe I should file a new issue for that and fix it?

I think it's just a different failure mode and should be fixed with this bug.

Later when javac bug is solved, this bug can also be set to resolve.

No, as explained above, other compilers could still generate this bytecode shape. We need to handle it properly in the VM.

Make sense! I will prepare a more proper fix to solve this even without javac change. (But it may take a while since I'm busy on other stuffs now...)

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 7, 2021

@kelthuzadx This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@kelthuzadx kelthuzadx closed this Sep 7, 2021
@kelthuzadx kelthuzadx deleted the JDK-8271055 branch Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler hotspot-compiler hotspot-runtime rfr
3 participants