JDK-8299324 : inline_native_setCurrentThread lacks GC barrier for Shenandoah
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 20,21
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2022-12-23
  • Updated: 2023-08-01
  • Resolved: 2023-02-06
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 21
21 b09Fixed
Related Reports
Relates :  
JDK-8313248 - C2: setScopedValueCache intrinsic exposes nullptr pre-values to store barriers
Relates :  
JDK-8299032 - Interface IN_NATIVE oop stores for C2
Description
inline_native_setCurrentThread was added for loom. It should have been the first IN_NATIVE oop store in C2. Instead of interfacing this properly, a raw store was used. For Shenandoah, this is not enough. Shenandoah scans OopStorage concurrently, and therefore, SATB barriers are required, so the root snapshot is kept consistent.
Comments
Changeset: 3ac2bedd Author: William Kemper <wkemper@openjdk.org> Committer: Paul Hohensee <phh@openjdk.org> Date: 2023-02-06 19:53:20 +0000 URL: https://git.openjdk.org/jdk/commit/3ac2beddbaa4e974f6d16d578505473a2e1d2a75
06-02-2023
A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/12300 Date: 2023-01-30 22:32:05 +0000
30-01-2023
Reopening to address the issues pointed out by Erik. This isn't really a duplicate of JDK-8299032, but it is related.
26-01-2023
I've changed the status to "closed as dup" instead of "resolved" as there is no change set. But please feel free to open this again as there seem to be more problems as pointed out by [~eosterlund].
26-01-2023
While it’s true that Shenandoah does indeed override store_at_resolved, the very first thing that function does is to bail out if the access isn’t IN_HEAP, which an IN_NATIVE access is not. That implies that Shenandoah will just generate a raw access, which is kind of the problem I’m getting at. Even if tests are passing, this is wrong as long as these IN_NATIVE roots are scanned concurrently. For G1 it’s okay, because they are scanned STW. Getting the JVM to crash though because of this is exceptionally hard. For the missed barrier to cause a crash, this edge would have to be the last edge in the graph to a virtual thread, so that when the thread is set to the new current thread, the previous current thread isn’t kept alive, even though it’s part of the snapshot-at-the-beginning. And while that is a violation of the constraints of the concurrent marking algorithm, someone would have to store Thread.currentThread() into a field during concurrent marking as well, and the last edge in the Java heap to said thread would have to be removed just before marking started, so that it’s really just the root having a reference to the thread. I believe that is all possible, and that hence this isn’t sound unless the contract for SATB marking is honoured, and pre-write barriers are emitted as required by concurrent root processing.
26-01-2023
Shenandoah already overrides `store_at_resolved` which is now invoked for `inline_native_setCurrentThread` after the changes for JDK-8299032 (https://github.com/openjdk/jdk/commit/e7fa150bc15b1bf5ab8921bfdf1a628ae08f5624).
26-01-2023
I've tried the naive change of just removing the assert from `BarrierSetC2::store_at_resolved`. I've run `hotspot_gc` and `hotspot_loom` tests without any errors. None of the gc jtreg tests exercise this code. Was the original assert meant to prevent card tables from seeing an oop that wouldn't be covered by any card? or?
09-01-2023
William, could you look at this, please?
03-01-2023
ILW = inconsistent root snapshot for Shenandoah with Loom, only Shenandoah, no workaround = MLH = P4
03-01-2023