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

Add null/array checks for OffHeap NotStaticField atomic case #20889

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rmnattas
Copy link
Contributor

@rmnattas rmnattas commented Jan 7, 2025

When current code transforms an atomic call from the java/util/concurrent/atomic/ package into an intrinsic, it skips generating the static check given that it's known to not be a static field being accessed (flagged by isNotStaticField). But with OffHeap a null and array checks are still required to check if loading the dataAddrPtr is necessary for correct address calculation.

Current code adds the OffHeap checks except for java/util/concurrent/atomic/ calls which was fine in some JDK levels as it uses VarHandles but for JDK8 it still uses processUnsafeAtomicCall() and without the OffHeap check its targeting the wrong address.

This PR adds inserting the OffHeap null and array checks to the isNotStaticField case.
I tried sharing some code between the case with and without isNotStaticField but some are copied over as it would need a larger re-write of processUnsafeAtomicCall()

@rmnattas rmnattas marked this pull request as ready for review January 8, 2025 19:27
@rmnattas
Copy link
Contributor Author

rmnattas commented Jan 8, 2025

fyi @zl-wang @midronij

* n96n lload <temp slot 3>
* n97n iload <temp slot 4>
*/
// Create another helper call that loads from ramStatics
Copy link
Contributor

Choose a reason for hiding this comment

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

at least, this example seemed contradictory to the setting of isNotStaticField (i.e. definitely true if it is an atomic call ... this example is under isNotStaticField being false). overdoing something here? plus, it sounded like an existing bug for gencon anyway? (previously, it needs to handle ramStatics too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That part is moved not added, I guess its confusing given that the other code between the if-condition and that comment have moved out to a shared path. But the comment only talks about the subsequent section of code.
https://github.com/eclipse-openj9/openj9/blob/v0.49.0-release/runtime/compiler/optimizer/J9RecognizedCallTransformer.cpp#L774-L808
I'll try to clean-up the code more making it less-confusing.

@zl-wang
Copy link
Contributor

zl-wang commented Jan 9, 2025

i still had impression i reviewed @midronij fix for off-heap in this very area ... i think it was merged. could it be in omr side instead? remember she worked on fixing fetchAndAdd for off-heap (i haven't searched the history yet).

@midronij
Copy link
Contributor

midronij commented Jan 9, 2025

@zl-wang I believe this is the PR you're referring to: #20333

Maybe Abdul can confirm if this is the case, but from what I can tell at least, I think the issue is that my changes linked above only take effect when isNotStaticField is false. They assume that if the object is known to be a non-static field at compile time, that no further checks are necessary. This works for gencon, since there are only two possible ways that the address is calculated:

  1. Object is a static field: address = ramStaticsFromClass + (offset & ~mask)
  2. Object is a non-static field: address = object base address + offset

So in the case where it is known at compile time that the object is NOT a static field (i.e.: isNotStaticField == true), we don't need to perform any runtime checks.

However, offheap adds a third case: if the object is a non-NULL array, then address = dataAddr + offset. So even if isNotStaticField is true, we still need to perform array and NULL checks to determine how the address should be calculated (i.e.: whether we need to load dataAddr or not).

@rmnattas
Copy link
Contributor Author

rmnattas commented Jan 9, 2025

Yes, the issue is with isNotStaticField being true in an OffHeap case.

It seems that it wasn't an issue as isNotStaticField is true for classes under java.util.concurrent.atomic which uses VarHandle in JDK11+, but in JDK8 it still uses the unsafe.getAndAdd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants