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

JDK-8313626: C2 crash due to unexpected exception control flow #15292

Closed
wants to merge 4 commits into from

Conversation

tobiasholenstein
Copy link
Contributor

@tobiasholenstein tobiasholenstein commented Aug 15, 2023

Problem

The following JASM code:

static Method test1:"()V" stack 1 {
    try t;
        invokestatic m:"()V";
        return;

        catch t java/lang/Throwable;
        stack_map class java/lang/Throwable;
        athrow;
    endtry t;
}

produces this java bytecode

static void m();
  Code:
     0: return

static void test1();
  Code:
     0: invokestatic  #4      // Method m:()V
     3: return
     4: athrow
  Exception table:
     from    to  target type
         0     5     4   Class java/lang/Throwable

from https://docs.oracle.com/javase/specs/jvms/se20/jvms20.pdf exception_table[] (p.116)

The values of the two items start_pc and end_pc indicate the ranges in the code array at which the exception handler is active. The value of start_pc must be a valid index into the code array of the opcode of an instruction. The value of end_pc either must be a valid index into the code array of the opcode of an instruction or must be equal to code_length, the length of the code array. The value of start_pc must be less than the value of end_pc.
The start_pc is inclusive and end_pc is exclusive; that is, the exception handler must be active while the program counter is within the interval [start_pc, end_pc).

handler_pc
The value of the handler_pc item indicates the start of the exception handler. The value of the item must be a valid index into the code array and must be the index of the opcode of an instruction.

and from §athrow (p.420)

The objectref must be of type reference and must refer to an object that is an instance of class Throwable or of a subclass of Throwable. It is popped from the operand stack. The objectref is then thrown by searching the current method (§2.6) for the first exception handler that matches the class of objectref, as given by the algorithm in §2.10.
If an exception handler that matches objectref is found, it contains the location of the code intended to handle this exception. The pc register is reset to that location, the operand stack of the current frame is cleared, objectref is pushed back onto the operand stack, and execution continues.

In out case: [start_pc=0, end_pc=5) and handler_pc=4 and objectref=Class java/lang/Throwable

By this definition we have indeed valid bytecode for test1(). Therefore we would expect C2 to create an infinite loop for

     4: athrow

The C2 graph indeed shows an infinite loop 92/81:
graph1

During IGVN the graph degenerates:
1)
graph2
2)
graph3
3)
graph4
And in the end we get an assert(false) failed: malformed control flow

Solution

We usually have a safepoint in infinite loops. The edge case that an exception can cause an infinite loop was not handled. With normal Java it is not possible to create such in infinite loop with try-catch, but with Jasm/bytecode it is allowed. Fix: By adding a safepoint to the backedge
safepoint1
we prevent the infinite loop from being removed during IGVN
safepoint2

Testing

We also found some other test cases that are very similar;
test2 is similar to test1. The endless loop is from/to 5: athrow

static void test2();
  Code:
     0: invokestatic  #6                  // Method m:()V
     3: return
     4: return
     5: athrow
  Exception table:
     from    to  target type
         0     3     4   Class java/lang/Exception
         0     6     5   Class java/lang/Throwable
  • in test3 and test4 th() gets inlined athrow has then a backedge to new that creates an infinite loop and is missing a safepoint
public static void th() throws java.lang.Exception;
  Code:
     0: new           #9                  // class java/lang/Throwable
     3: dup
     4: invokespecial #3                  // Method java/lang/Throwable."<init>":()V
     7: athrow

static void test3();
  Code:
     0: invokestatic  #6                  // Method m:()V
     3: iconst_1
     4: istore_0
     5: iconst_0
     6: istore_1
     7: return
     8: invokestatic  #4                  // Method th:()V
    11: return
  Exception table:
     from    to  target type
         0    12     8   Class java/lang/Throwable

static void test4();
  Code:
     0: invokestatic  #6                  // Method m:()V
     3: iconst_1
     4: istore_0
     5: iconst_0
     6: istore_1
     7: return
     8: iconst_1
     9: istore_0
    10: invokestatic  #4                  // Method th:()V
    13: return
  Exception table:
     from    to  target type
         0    14     8   Class java/lang/Throwable

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8313626: C2 crash due to unexpected exception control flow (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15292/head:pull/15292
$ git checkout pull/15292

Update a local copy of the PR:
$ git checkout pull/15292
$ git pull https://git.openjdk.org/jdk.git pull/15292/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15292

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15292.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 15, 2023

👋 Welcome back tholenstein! 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
Copy link

openjdk bot commented Aug 15, 2023

@tobiasholenstein The following label will be automatically applied to this pull request:

  • hotspot-compiler

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

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Aug 15, 2023
@tobiasholenstein tobiasholenstein marked this pull request as ready for review August 18, 2023 13:33
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 18, 2023
@mlbridge
Copy link

mlbridge bot commented Aug 18, 2023

Webrevs

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@openjdk
Copy link

openjdk bot commented Aug 18, 2023

@tobiasholenstein This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8313626: C2 crash due to unexpected exception control flow

Reviewed-by: thartmann, chagedorn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 86 new commits pushed to the master branch:

  • fdac6a6: 8312180: (bf) MappedMemoryUtils passes incorrect arguments to msync (aix)
  • 7521211: 8314265: Fix -Wconversion warnings in miscellaneous runtime code
  • 2f04bc5: 8313697: [XWayland][Screencast] consequent getPixelColor calls are slow
  • 33d5dfd: 8314543: gitattributes: make diffs easier to read
  • 5058854: 8314389: AttachListener::pd_set_flag obsolete
  • 891c3f4: 8307356: Metaspace: simplify BinList handling
  • 0299364: 8314249: Refactor handling of invokedynamic in JVMCI ConstantPool
  • 96778dd: 8314169: Combine related RoundingMode logic in j.text.DigitList
  • 808bb1f: 8314246: javax/swing/JToolBar/4529206/bug4529206.java fails intermittently on Linux
  • 6445314: 8314477: Improve definition of "prototypical type"
  • ... and 76 more: https://git.openjdk.org/jdk/compare/12326770dc4116dd3b374c3a50fabfa1f27249dd...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 18, 2023
Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

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

That looks good to me, too!

@dean-long
Copy link
Member

Can you add a JASM test that doesn't put the exception handler in unreachable code?

@TobiHartmann
Copy link
Member

Isn't MissingSafepointOnTryCatch::testInfinite such an example?

@dean-long
Copy link
Member

Isn't MissingSafepointOnTryCatch::testInfinite such an example?

Yes, my mistake.

@tobiasholenstein
Copy link
Contributor Author

Thanks @TobiHartmann and @chhagedorn for the reviews!

/integrate

@openjdk
Copy link

openjdk bot commented Aug 23, 2023

Going to push as commit f8203cb.
Since your change was applied there have been 125 commits pushed to the master branch:

  • 2be469f: 8314743: Use of uninitialized local in SR_initialize after JDK-8314114
  • 571c435: 8313374: --enable-ccache's CCACHE_BASEDIR breaks builds
  • d1de3d0: 8313901: [TESTBUG] test/hotspot/jtreg/compiler/codecache/CodeCacheFullCountTest.java fails with java.lang.VirtualMachineError
  • a0d0f21: 8314752: Use google test string comparison macros
  • 7e843c2: 8284772: GHA: Use GCC Major Version Dependencies Only
  • ba6cdbe: 8309214: sun/security/pkcs11/KeyStore/CertChainRemoval.java fails after 8301154
  • 9f4a9fe: 8312434: SPECjvm2008/xml.transform with CDS fails with "can't seal package nu.xom"
  • 7c169a4: 8312232: Remove sun.jvm.hotspot.runtime.VM.buildLongFromIntsPD()
  • 2eae13c: 8214248: (fs) Files:mismatch spec clarifications
  • ce1ded1: 8314749: Remove unimplemented _Copy_conjoint_oops_atomic
  • ... and 115 more: https://git.openjdk.org/jdk/compare/12326770dc4116dd3b374c3a50fabfa1f27249dd...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 23, 2023
@openjdk openjdk bot closed this Aug 23, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 23, 2023
@openjdk
Copy link

openjdk bot commented Aug 23, 2023

@tobiasholenstein Pushed as commit f8203cb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
4 participants