8338566: Lazy creation of exception instances is not thread safe #20950

TobiHartmann · 2024-09-11T14:17:30Z

Similar to JDK-8251923, we need a store-store barrier before publishing a handle because otherwise another thread could observe the handle before it's fully initialized and read null from it. This affects architectures with a weak memory model like AArch64.

Unfortunately, this only happened twice in our testing and I was never able to reproduce it.

Thanks,
Tobias

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-8338566: Lazy creation of exception instances is not thread safe (Bug - P3)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)
Dean Long (@dean-long - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20950

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

Using diff file

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

Webrev

Link to Webrev Comment

…ishing handle

bridgekeeper · 2024-09-11T14:18:38Z

👋 Welcome back thartmann! 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 · 2024-09-11T14:19:08Z

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

8338566: Lazy creation of exception instances is not thread safe

Reviewed-by: shade, kvn, dlong

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

418bb42: 8340073: Support "%z" time zone abbreviation format in TZ files
b26645f: 8339883: Open source several AWT/2D related tests
858b4f1: 8339960: GenShen: Fix inconsistencies in generational Shenandoah behavior
59407fa: 8310525: DynamicLauncher for JDP test needs to try harder to find a free port
89759c8: 8321935: Define the term 'standard doclet'
65b9aba: 8339769: Incorrect error message during startup if working directory does not exist
1640bd2: 8340186: Shenandoah: Missing load_reference_barrier_phantom_narrow match in is_shenandoah_lrb_call
996790c: 8339954: Print JVMCI names with the Compiler.{perfmap,codelist,CodeHeap_Analytics} diagnostic commands
e1ebeef: 8340131: Refactor internal makeHiddenClassDefiner to take option mask instead of Set
05b9d47: 8340194: Replace usage of -ms with -Xms in LauncherCommon.gmk make file
... and 49 more: https://git.openjdk.org/jdk/compare/55a7cf14453b6cd1de91362927b2fa63cba400a1...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 · 2024-09-11T14:19:44Z

@TobiHartmann 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.

mlbridge · 2024-09-11T14:22:35Z

Webrevs

01: Full - Incremental (155cd71d)
00: Full (5b4d9d11)

shipilev

It looks to me the issue is not exactly in CI, but in the fact that we can publish a racily constructed global JNI handle. If so, shouldn't we be adding storestore at the exit path in JNIHandles::make_global to be absolutely sure we covered all uses like these?

TobiHartmann · 2024-09-11T14:38:20Z

Right, I think it depends on if we want to make JNIHandles::make_global thread-safe or not. There are quite a few uses of JNIHandles::make_global and I don't know how performance sensitive these are.

shipilev · 2024-09-11T14:42:03Z

I think we better fix it in JNIHandles::make_global, since both JDK-8251923 and this bug shows how fragile and hard to reproduce this failure mode is. Let's hear from others?

I don't mind this PR to go in, if we follow it with the RFE that replaces the per-use storestore-s with a (pun intended) more global one.

TobiHartmann · 2024-09-11T14:54:17Z

Right, fine with me. Let's see what others think.

vnkozlov

Good

dholmes-ora · 2024-09-11T20:51:44Z

Safe publication is the responsibility of the publisher, so this PR seems fine to me in that regard. I don't think make_global should end in a storestore!

But is this storestore sufficient to make this publication scenario perfectly thread-safe? I would have expected to see a release_store/load_acquire pairing in the ciEnv::*Exception_instance() methods.

dean-long · 2024-09-12T02:13:23Z

I agree, the ciEnv::*Exception_instance() methods look like a problem. They are using shared static jobjects without synchronization. If there is a race, one compiler thread can overwrite the existing handle that another compiler thread used to resolve the oop. I think we need synchronization or compare-and-swap when assigning to the static handle to prevent a leak. We may be OK without the load-acquire on the read side, if we rely on the data dependency like interpreter/generated code does.

TobiHartmann · 2024-09-12T07:39:02Z

Thanks for looking into this, David and Dean. Good points, I agree that we would need to make this completely thread-safe to prevent a leak. Looking at the code again, I wonder why we even do all this lazily, especially since we already create NullPointerException and ArithmeticException eagerly at VM startup:

jdk/src/hotspot/share/memory/universe.cpp

Lines 1086 to 1089 in 438121b

    
           // Setup preallocated NullPointerException/ArithmeticException 
        
           // (used for a cheap & dirty solution in compiler exception handling) 
        
           _null_ptr_exception.init_if_empty(vmSymbols::java_lang_NullPointerException(), CHECK_false); 
        
           _arithmetic_exception.init_if_empty(vmSymbols::java_lang_ArithmeticException(), CHECK_false);

Couldn't we do the same for ArrayIndexOutOfBoundsException, ArrayStoreException and ClassCastException? This would save us quite some complexity and I think the startup / footprint overhead is negligible.

dean-long · 2024-09-12T09:48:28Z

@TobiHartmann Yes, that seems like the best idea. I was going to suggest moving the fields into the CompilerThread, which gets rid of the race and limits the redundant objects, but I like your idea better.

TobiHartmann · 2024-09-13T09:12:04Z

Okay, here's a new version with eager exception creation. Looks much cleaner to me.

shipilev

Cleaner, right? No concurrency problems, exceptions get CDS archived. This might affect startup a little, but I would not expect it to matter.

I suggest to rename the bug into something else, given the whole ciEnv::get_or_create_exception is gone.

TobiHartmann · 2024-09-13T12:09:40Z

Thanks Aleksey! I changed the title.

TobiHartmann · 2024-09-16T12:40:29Z

@dean-long, @vnkozlov please (re-)approve. Thanks!

vnkozlov

Good.

dean-long

Nice improvement.

TobiHartmann · 2024-09-17T10:37:46Z

Thanks for the reviews Vladimir and Dean!

TobiHartmann · 2024-09-17T10:37:51Z

/integrate

openjdk · 2024-09-17T10:39:33Z

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

8b6e277: 8340273: Remove CounterHalfLifeTime
c6721a0: 8340009: Improve the output from assert_different_registers
7834662: 8340119: Remove oopDesc::size_might_change()
10050a7: 8332442: C2: refactor Mod cases in Compile::final_graph_reshaping_main_switch()
7849f25: 8340184: Bug in CompressedKlassPointers::is_in_encodable_range
a4cf191: 8339793: Fix incorrect APX feature enabling with -XX:-UseAPX
3e03e66: 8340176: Replace usage of -noclassgc with -Xnoclassgc in test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest2.java
99d7185: 8340144: C1: remove unused Compilation::_max_spills
418bb42: 8340073: Support "%z" time zone abbreviation format in TZ files
b26645f: 8339883: Open source several AWT/2D related tests
... and 57 more: https://git.openjdk.org/jdk/compare/55a7cf14453b6cd1de91362927b2fa63cba400a1...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-17T10:39:40Z

@TobiHartmann Pushed as commit 269cd38.

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

8338566: Missing membar in ciEnv::get_or_create_exception before publ…

5b4d9d1

…ishing handle

openjdk bot added the rfr Pull request is ready for review label Sep 11, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Sep 11, 2024

shipilev reviewed Sep 11, 2024

View reviewed changes

vnkozlov approved these changes Sep 11, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 11, 2024

Create exceptions eagerly

155cd71

openjdk bot removed the ready Pull request is ready to be integrated label Sep 13, 2024

shipilev approved these changes Sep 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 13, 2024

TobiHartmann changed the title ~~8338566: Missing membar in ciEnv::get_or_create_exception before publishing handle~~ 8338566: Lazy creation of exception instances is not thread safe Sep 13, 2024

vnkozlov approved these changes Sep 16, 2024

View reviewed changes

dean-long approved these changes Sep 16, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 17, 2024

openjdk bot closed this Sep 17, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 17, 2024

8338566: Lazy creation of exception instances is not thread safe #20950

8338566: Lazy creation of exception instances is not thread safe #20950

Uh oh!

Conversation

TobiHartmann commented Sep 11, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Sep 11, 2024

Uh oh!

openjdk bot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 11, 2024

Uh oh!

mlbridge bot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

shipilev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Sep 11, 2024

Uh oh!

shipilev commented Sep 11, 2024

Uh oh!

TobiHartmann commented Sep 11, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Sep 11, 2024

Uh oh!

dean-long commented Sep 12, 2024

Uh oh!

TobiHartmann commented Sep 12, 2024

Uh oh!

dean-long commented Sep 12, 2024

Uh oh!

TobiHartmann commented Sep 13, 2024

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Sep 13, 2024

Uh oh!

TobiHartmann commented Sep 16, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

dean-long left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Sep 17, 2024

Uh oh!

TobiHartmann commented Sep 17, 2024

Uh oh!

openjdk bot commented Sep 17, 2024

Uh oh!

openjdk bot commented Sep 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

TobiHartmann commented Sep 11, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 11, 2024 •

edited

Loading

mlbridge bot commented Sep 11, 2024 •

edited

Loading

shipilev left a comment •

edited

Loading