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

8285301: C2: assert(!requires_atomic_access) failed: can't ensure atomicity #8422

Closed
wants to merge 3 commits into from

Conversation

TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Apr 27, 2022

We hit asserts in BarrierSetC2::load_at_resolved and BarrierSetC2::store_at_resolved when running with -XX:+AlwaysAtomicAccesses because the corresponding code paths have not been implemented yet. Although the assert triggers on all platforms, it only affects long and double accesses on 32-bit systems (everything else is atomic anyway).

I moved the requires_atomic_access logic into LoadNode::make and StoreNode::make and refactored related code.

I noticed that LoadNode::convert_to_reinterpret_load and StoreNode::convert_to_reinterpret_store did not properly check for requires_atomic_access and fixed that as well.

I'm currently running all tests with -XX:+AlwaysAtomicAccesses.

Thanks,
Tobias


Progress

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

Issue

  • JDK-8285301: C2: assert(!requires_atomic_access) failed: can't ensure atomicity

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8422

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 27, 2022

👋 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 openjdk bot added the rfr label Apr 27, 2022
@openjdk
Copy link

openjdk bot commented Apr 27, 2022

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

  • hotspot

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 label Apr 27, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 27, 2022

Webrevs

const TypePtr* at, Node *val, BasicType bt, MemOrd mo);
static StoreNode* make(PhaseGVN& gvn, Node* c, Node* mem, Node* adr,
const TypePtr* at, Node* val, BasicType bt,
MemOrd mo = MemNode::unordered, bool require_atomic_access = false);
Copy link
Contributor

@vnkozlov vnkozlov Apr 27, 2022

Choose a reason for hiding this comment

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

I think it was intentionally MemOrd mo parameter did not specify default value so that value is always specified at all call sites. I think this change is not required for the fix.

Copy link
Member Author

@TobiHartmann TobiHartmann Apr 28, 2022

Choose a reason for hiding this comment

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

Okay, I reverted that change.

bool require_atomic_access = (Opcode() == Op_LoadL && ((LoadLNode*)this)->require_atomic_access()) ||
(Opcode() == Op_LoadD && ((LoadDNode*)this)->require_atomic_access());
Copy link
Contributor

@vnkozlov vnkozlov Apr 27, 2022

Choose a reason for hiding this comment

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

You can call Opcode() only once (it is virtual call - expensive).

bool require_atomic_access = (Opcode() == Op_StoreL && ((StoreLNode*)this)->require_atomic_access()) ||
(Opcode() == Op_StoreD && ((StoreDNode*)this)->require_atomic_access());
Copy link
Contributor

@vnkozlov vnkozlov Apr 27, 2022

Choose a reason for hiding this comment

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

Call Opcode() once.

Copy link
Member Author

@TobiHartmann TobiHartmann Apr 28, 2022

Choose a reason for hiding this comment

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

Done.

@dean-long
Copy link
Member

dean-long commented Apr 27, 2022

The new TestAlwaysAtomicAccesses test seems suspiciously simple. I guess the idea is to catch issues during startup with -Xcomp. A comment saying that could prevent someone from thinking the test does nothing.

Copy link
Member Author

@TobiHartmann TobiHartmann left a comment

Thanks for the reviews, Vladimir and Dean.

The new TestAlwaysAtomicAccesses test seems suspiciously simple. I guess the idea is to catch issues during startup with -Xcomp. A comment saying that could prevent someone from thinking the test does nothing.

Right, we have quite a few such tests for flags that are rarely used. A simple run with -Xcomp is enough to trigger the originally report issues. More issues are then triggered by the other tests that I modified. I added a comment explaining the purpose.

const TypePtr* at, Node *val, BasicType bt, MemOrd mo);
static StoreNode* make(PhaseGVN& gvn, Node* c, Node* mem, Node* adr,
const TypePtr* at, Node* val, BasicType bt,
MemOrd mo = MemNode::unordered, bool require_atomic_access = false);
Copy link
Member Author

@TobiHartmann TobiHartmann Apr 28, 2022

Choose a reason for hiding this comment

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

Okay, I reverted that change.

bool require_atomic_access = (Opcode() == Op_StoreL && ((StoreLNode*)this)->require_atomic_access()) ||
(Opcode() == Op_StoreD && ((StoreDNode*)this)->require_atomic_access());
Copy link
Member Author

@TobiHartmann TobiHartmann Apr 28, 2022

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Looks good.

@openjdk
Copy link

openjdk bot commented Apr 28, 2022

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

8285301: C2: assert(!requires_atomic_access) failed: can't ensure atomicity

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

  • bba456a: 8285676: Add missing @param tags for type parameters on classes and interfaces
  • b9d1e85: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
  • b718578: 8285011: gc/arguments/TestUseCompressedOopsFlagsWithUlimit.java fails after JDK-8280761
  • 2d8d140: 8285690: CloneableReference subtest should not throw CloneNotSupportedException
  • ea83b44: 8280510: AArch64: Vectorize operations with loop induction variable
  • 36bf6fb: 8285728: Alpine Linux build fails with busybox tar
  • 091637c: 8285630: Fix a configure error in RISC-V cross build
  • ccf0e8b: 8285755: JDK-8285093 changed the default for --with-output-sync
  • d7514b0: 8285595: Assert frame anchor doesn't change in safepoints/handshakes
  • 5629c75: 8284848: C2: Compiler blackhole arguments should be treated as globally escaping
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/4714fdcd6a1615b9d357dab0116a579c1cd5bfb5...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 label Apr 28, 2022
@TobiHartmann
Copy link
Member Author

TobiHartmann commented Apr 29, 2022

Vladimir, Dean, thanks for the reviews!

@TobiHartmann
Copy link
Member Author

TobiHartmann commented Apr 29, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Apr 29, 2022

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

  • 40f19c0: 8264666: Change implementation of safeAdd/safeMult in the LCMSImageLayout class
  • 1e28fcb: 8155701: The compiler fails with an AssertionError: typeSig ERROR
  • 99388ef: 8283624: Create an automated regression test for RFE-4390885
  • 94b533a: 8285699: riscv: Provide information when hitting a HaltNode
  • e2e943a: 8285688: Add links to JEPs and JSRs to SourceVersion
  • 80cf59f: 8285610: TreeInfo.pathFor and its uses appear to be dead code
  • 21b62fe: 8195589: T6587786.java failed after JDK-8189997
  • 8190217: 8285496: DocLint does not check for missing @param tags for type parameters on classes and interfaces
  • 64d98ba: 8285094: Test java/awt/Frame/InvisibleOwner/InvisibleOwner.java failing on Linux
  • 573eace: 8285504: Minor cleanup could be done in javax.net
  • ... and 33 more: https://git.openjdk.java.net/jdk/compare/4714fdcd6a1615b9d357dab0116a579c1cd5bfb5...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Apr 29, 2022
@openjdk openjdk bot closed this Apr 29, 2022
@openjdk openjdk bot removed ready rfr labels Apr 29, 2022
@openjdk
Copy link

openjdk bot commented Apr 29, 2022

@TobiHartmann Pushed as commit 0a4a640.

💡 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 integrated
3 participants