7176515: ExceptionInInitializerError for an enum with multiple switch statements

[archiecobbs]

This patch is both a minor optimization and a fix for JDK-7176515.

JDK-7176515 relates to how javac handles switch statements on Enum values, which are effectively "syntactic sugar".

The simple approach would be to just get the enum's ordinal() value and then revert to a normal integer value switch on that. However, this snapshots the ordinal values at compile-time, and thus can fail if the Enum class is recompiled later.

Presumably to avoid that possibility (are there any other reasons?) the compiler instead uses a lookup table. This table is dynamically constructed at runtime by a static initializer in a synthetic class. This avoids the "stale ordinal" problem of the simple approach, but it also creates a potential class initialization loop if there happens to an Enum switch inside an Enum class constructor, as demonstrated by the bug.

This patch makes the following change: If we are handling an Enum switch, and the Enum class we are switching on is also the class we are compiling, then just go with the simple approach, because the "stale ordinal" problem can't happen in this case so there's no need to build a runtime lookup table.

This results in two improvements:

• It avoids building the lookup table for switches on self
• It fixes JDK-7176515 as stated

Although the reproducing test case provided in JDK-7176515 gets fixed by this patch, the underlying issue is still there and can still be triggered with a slightly more complicated test case (included, but commented out).

So JDK-7176515 could be left open (or a new bug created) and a separate discussion had about how to "really" fix it.

Part of this discussion should be defining what that means, i.e., the boundaries of the bug. There are some scenarios that are basically impossible to fix, for example, two Enum classes whose constructors take as a parameter instances of the other Enum class and then switch on it. That cycle has nothing to do with how switches are handled.

---

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

Issues

• JDK-7176515: ExceptionInInitializerError for an enum with multiple switch statements
• JDK-8299760: ExceptionInInitializerError for an enum with multiple switch statements, follow-up

Reviewers

• Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to 05fb5e76

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10797

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

Using diff file

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

[bridgekeeper]

👋 Welcome back archiecobbs! 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]

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

• 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.

[mlbridge]

Webrevs

• 10: Full - Incremental (e6ef2acf)
• 09: Full - Incremental (e0d4c0e0)
• 08: Full - Incremental (8987541c)
• 07: Full - Incremental (53be679b)
• 06: Full - Incremental (6f7f30f7)
• 05: Full - Incremental (f8b189ec)
• 04: Full - Incremental (153ef39e)
• 03: Full - Incremental (05fb5e76)
• 02: Full - Incremental (b5da18df)
• 01: Full - Incremental (8cd5b49c)
• 00: Full (b5337927)

[bridgekeeper]

@archiecobbs 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!

[archiecobbs]

keepalive

[vicente-romero-oracle]

lgtm

[openjdk]

@archiecobbs 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:

7176515: ExceptionInInitializerError for an enum with multiple switch statements
8299760: ExceptionInInitializerError for an enum with multiple switch statements, follow-up

Reviewed-by: vromero

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 34 new commits pushed to the master branch:

• bf917ba: 8304687: Move add_to_hierarchy
• 63d4afb: 8304671: javac regression: Compilation with --release 8 fails on underscore in enum identifiers
• e2cfcfb: 6817009: Action.SELECTED_KEY not toggled when using key binding
• af4d560: 8303951: Add asserts before record_method_not_compilable where possible
• c433862: 6245410: javax.swing.text.html.CSS.Attribute: BACKGROUND_POSITION is not w3c spec compliant
• 91f407d: 8029301: Confusing error message for array creation method reference
• e73411a: 8304376: Rename t1/t2 classes in com/sun/jdi/CLETest.java to avoid class duplication error in IDE
• a2d8f63: 8288730: Add type parameter to Lookup::accessClass and Lookup::ensureInitialized
• 3777455: 8302191: Performance degradation for float/double modulo on Linux
• 760c012: 8304683: Memory leak in WB_IsMethodCompatible
• ... and 24 more: https://git.openjdk.org/jdk/compare/42723dcb1862da598092bb499056940d78a8bdac...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vicente-romero-oracle) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[vicente-romero-oracle]

I think that a follow-up JIRA issue should be filed to track the situation in which the bug can still occur

[archiecobbs]

I think that a follow-up JIRA issue should be filed to track the situation in which the bug can still occur

Agreed (though it's not something I have permission to do).

[vicente-romero-oracle]

I think that a follow-up JIRA issue should be filed to track the situation in which the bug can still occur

Agreed (though it's not something I have permission to do).

true, I have created: https://bugs.openjdk.org/browse/JDK-8299760

[archiecobbs]

/integrate

[openjdk]

@archiecobbs
Your change (at version 05fb5e7) is now ready to be sponsored by a Committer.

[archiecobbs]

I've made a couple of improvements to this patch. As a result the follow-up bug 8299760, and also 8219412 - Eager enum class initialization with enum switch are now fixed.

Improvement 1

The previous patch avoided generating a mapping table when the enum being switched on was the same class as the class currently being compiled. This is because there is no possibility of the ordinal values changing at runtime due to a recompilation. But this is too conservative - the same thing is true for any enum type defined in the same top-level class. So we generalize the test from "same class" as "in the same top-level class compilation". This change fixes the follow-up bug 8299760.

Improvement 2

All of the mapping tables for any enum type switched were being jammed into the same compiler-generated class. This meant that all of these enum types were initialized at the same time, regardless of which one actually experiences a 'first use'. This is what causes 8219412. The fix here is simple - put each mapping table into its own compiler-generated class.

/solves 8299760
/solves 8219412

[openjdk]

@archiecobbs
Adding additional issue to solves list: 8299760: ExceptionInInitializerError for an enum with multiple switch statements, follow-up.

[openjdk]

@archiecobbs
Adding additional issue to solves list: 8219412: Eager enum class initialization with enum switch.

[archiecobbs]

On second thought, the 2nd improvement belongs in a separate PR, as there probably is more discussion required.

Reverting that part of the change from this PR.

/issue remove 8219412

[openjdk]

@archiecobbs
Removing additional issue from issue list: 8219412.

[bridgekeeper]

@archiecobbs 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!

[bridgekeeper]

@archiecobbs This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[archiecobbs]

/open

[openjdk]

@archiecobbs This pull request is now open

[archiecobbs]

I think that's an unrelated test failure:

STDERR:
Unrecognized VM option 'UseCompressedClassPointers'
Error: Could not create the Java Virtual Machine.

But also I believe this problem was fixed a while ago, so I'll merge in the latest master shortly and that should make the test failure go away.

Thanks.

[vicente-romero-oracle]

I'm still seeing test: test/hotspot/jtreg/runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java failing with this patch

[archiecobbs]

I'm still seeing test: test/hotspot/jtreg/runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java failing with this patch

Strange... on github everything is passing now: https://github.com/archiecobbs/jdk/actions/runs/4473109724

But when I run that specific test on my laptop, it fails for me too.

So I guess this test is not included in the standard test suite?

In any case should be fixed in e6ef2ac.

[jaikiran]

Hello Archie,

I'm still seeing test: test/hotspot/jtreg/runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java failing with this patch

Strange... on github everything is passing now: https://github.com/archiecobbs/jdk/actions/runs/4473109724

But when I run that specific test on my laptop, it fails for me too.

GitHub actions job for the jdk repo is configured to run only tier1 tests. The tests are grouped into different tiers in a file called TEST.groups. For hotspot that file resides here and the tier1 definition is this https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/TEST.groups#L545. You will notice that it's configured to run (among other things), the tier1_runtime which is defined in that same file https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/TEST.groups#L364. That group is defined to run everything under runtime/ directory except the ones marked with a - (minus) sign and one of those is https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/TEST.groups#L404 tier1_runtime_appcds_exclude which is defined here https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/TEST.groups#L513, which except for a few tests, excludes everything else under runtime/cds/appcds/ directory, so I believe that's why this test/hotspot/jtreg/runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java isn't run in that job.

[archiecobbs]

I'm still seeing test: test/hotspot/jtreg/runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java failing with this patch

Strange... on github everything is passing now: https://github.com/archiecobbs/jdk/actions/runs/4473109724
But when I run that specific test on my laptop, it fails for me too.

GitHub actions job for the jdk repo is configured to run only tier1 tests.

Thanks very much for that clarification.

[archiecobbs]

/integrate

[openjdk]

@archiecobbs
Your change (at version e6ef2ac) is now ready to be sponsored by a Committer.

[vicente-romero-oracle]

/sponsor

[openjdk]

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

• dd23ee9: 8303917: Update ISO 639 language codes table
• 6f67abd: 8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
• 568dd57: 8304716: Clean up G1Policy::calc_max_old_cset_length()
• af0504e: 8304691: Remove jlink --post-process-path option
• 3859faf: 8231349: Move intrinsic stubs generation to compiler runtime initialization code
• f37674a: 8304711: Combine G1 root region abort and wait into a single method
• 7f9e691: 8304712: Only pass total number of regions into G1Policy::calc_min_old_cset_length
• 51035a7: 8294137: Review running times of java.math tests
• 46cca1a: 4842457: (bf spec) Clarify meaning of "(optional operation)"
• 6fa25cc: 8184444: The compiler error "variable not initialized in the default constructor" is not apt in case of static final variables
• ... and 37 more: https://git.openjdk.org/jdk/compare/42723dcb1862da598092bb499056940d78a8bdac...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vicente-romero-oracle @archiecobbs Pushed as commit ac6af6a.

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