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

8275874: [JVMCI] only support aligned reads in c2v_readFieldValue #6109

Closed
wants to merge 3 commits into from

Conversation

dougxc
Copy link
Member

@dougxc dougxc commented Oct 25, 2021

JDK-8275645 resulted in loosing single-copy atomicity for reads in c2v_readFieldValue. This PR fixes that only doing aligned reads in c2v_readFieldValue.


Progress

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

Issue

  • JDK-8275874: [JVMCI] only support aligned reads in c2v_readFieldValue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6109

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 25, 2021

👋 Welcome back dnsimon! 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
Copy link

@openjdk openjdk bot commented Oct 25, 2021

@dougxc The following labels will be automatically applied to this pull request:

  • build
  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added build hotspot-compiler labels Oct 25, 2021
@dougxc dougxc marked this pull request as ready for review Oct 25, 2021
@openjdk openjdk bot added the rfr label Oct 25, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 25, 2021

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented Oct 25, 2021

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

8275874: [JVMCI] only support aligned reads in c2v_readFieldValue

Reviewed-by: never, shade

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

  • 71d593e: 8275162: Use varargs in 'def' macros in mutexLocker.cpp
  • 7ca053d: 8251904: vmTestbase/nsk/sysdict/vm/stress/btree/btree010/btree010.java fails with ClassNotFoundException: nsk.sysdict.share.BTree0LLRLRLRRLR
  • 4be88d5: 8275047: Optimize existing fill stubs for AVX-512 target
  • 63e0f34: 8275767: JDK source code contains redundant boolean operations in jdk.charsets
  • 4961373: 8275137: jdk.unsupported/sun.reflect.ReflectionFactory.readObjectNoDataForSerialization uses wrong signature
  • 174f553: 8275869: Problem list applications/jcstress/copy.java on Linux-aarch64
  • 3ff085e: 8275582: Don't purge metaspace mapping lists
  • 10e1610: 8251134: Unwrapping a key with a Private Key generated by Microsoft CNG fails
  • 4361945: 8185844: MSCAPI doesn't list aliases correctly
  • 337a9b7: 8269853: Prefetch::read should accept pointer to const
  • ... and 6 more: https://git.openjdk.java.net/jdk/compare/7f94302ceca001ded89ba9a653bf176ef90b16cd...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 Oct 25, 2021
@dougxc
Copy link
Member Author

@dougxc dougxc commented Oct 25, 2021

@shipilev , it would be great if you could review this.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Oct 26, 2021

Isn't the title of this issue expressed incorrectly?

@dougxc dougxc changed the title 8275874: [JVMCI] use volatile accessors for all unaligned reads in c2v_readFieldValue 8275874: [JVMCI] use volatile accessors for aligned reads in c2v_readFieldValue Oct 26, 2021
@dougxc
Copy link
Member Author

@dougxc dougxc commented Oct 26, 2021

Isn't the title of this issue expressed incorrectly?

Yes - thanks for pointing that out.

@openjdk openjdk bot added ready and removed ready labels Oct 26, 2021
@magicus
Copy link
Member

@magicus magicus commented Oct 26, 2021

/label remove build

@openjdk openjdk bot removed the build label Oct 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 26, 2021

@magicus
The build label was successfully removed.

Copy link
Contributor

@shipilev shipilev left a comment

It definitely looks better than the current code, but I think my original concern still stands. If there is a caller code that does the non-aligned volatile access, that caller code is in error, and we should not silently downgrade it to non-aligned non-volatile access, like this patch does.

If I read your previous comments correctly, you mention here that Graal produces such accesses? If so, then Graal is in error, and this code should fail. Instead of waiting for SIGBUS, though, we should probably reinstate the isVolatile argument to this method, and add the alignment checks in the pre-check block, verifying that volatile accesses should always be aligned.

In other words, the SIGBUS that you were getting is the valid failure, indicating the actual problem with the access. What this code needs is to verify the "volatile should be aligned" condition more precisely, and then callers have to fit this requirement.

@dougxc
Copy link
Member Author

@dougxc dougxc commented Oct 26, 2021

we should not silently downgrade it to non-aligned non-volatile access

I'm not so sure. The javadoc for Unsafe.getLongUnaligned includes:

     * <p> The read will be atomic with respect to the largest power
     * of two that divides the GCD of the offset and the storage size.
     * For example, getLongUnaligned will make atomic reads of 2-, 4-,
     * or 8-byte storage units if the offset is zero mod 2, 4, or 8,
     * respectively.  There are no other guarantees of atomicity.

This implies there's no way someone can ask (in Java) for an unaligned volatile access.

How about I clarify the javadoc for the CompilerToVM.readFieldValue methods as follows:

diff --git a/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java b/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java
index 032d21ca235..06c78b37fd8 100644
--- a/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java
+++ b/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java
@@ -783,15 +783,23 @@ final class CompilerToVM {

     /**
      * Reads the current value of a static field. If {@code expectedType} is non-null, then the
-     * object is exptected to be a subtype of {@code expectedType} and extra sanity checking is
+     * object is expected to be a subtype of {@code expectedType} and extra sanity checking is
      * performed on the offset and kind of the read being performed.
+     *
+     * The read is performed with volatile load semantics if is aligned (i.e.,
+     * {@code offset % kind.getByteCount()) == 0}). For unaligned reads, non-volatile semantics are
+     * used.
      */
     native JavaConstant readFieldValue(HotSpotResolvedObjectTypeImpl object, HotSpotResolvedObjectTypeImpl expectedType, long offset, JavaKind kind);

     /**
      * Reads the current value of an instance field. If {@code expectedType} is non-null, then the
-     * object is exptected to be a subtype of {@code expectedType} and extra sanity checking is
+     * object is expected to be a subtype of {@code expectedType} and extra sanity checking is
      * performed on the offset and kind of the read being performed.
+     *
+     * The read is performed with volatile load semantics if is aligned (i.e.,
+     * {@code offset % kind.getByteCount()) == 0}). For unaligned reads, non-volatile semantics are
+     * used.
      */
     native JavaConstant readFieldValue(HotSpotObjectConstantImpl object, HotSpotResolvedObjectTypeImpl expectedType, long offset, JavaKind kind);

@shipilev
Copy link
Contributor

@shipilev shipilev commented Oct 26, 2021

we should not silently downgrade it to non-aligned non-volatile access

I'm not so sure. The javadoc for Unsafe.getLongUnaligned includes: [...] This implies there's no way someone can ask (in Java) for an unaligned volatile access.

Yes. Note that getLongUnaligned is the addition for getLong, not getLongVolatile. "volatility" is not something optional, it is explicit, this is why there are getLong and getLongVolatile. The getLong/getLongUnaligned access is already non-volatile, and so it can be non-aligned. The job for getLongUnaligned is to then figure out if hardware can withstand the coarse unaligned load, or, if not, split it in the individual non-atomic accesses. The getLongVolatile is volatile, full stop. If it is called on unaligned offset, receiving SIGBUS or other kind of error is the expected behavior.

In other words, if caller asks for volatile access, it should either get the volatile (atomic) access, or get the error if such access is not possible, or do some sort of atomic recovery (probably involves heavy locking, so this is seldom an option). The access handling code should not be allowed to decay bad volatile loads into "good" non-volatile loads to avoid the reasonable hardware exception. Instead, it should explicitly fail on bad accesses, thus communicating to the caller that the requested "volatility" property cannot be satisfied for non-aligned accesses.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Oct 26, 2021

Expanding some more on this. I think expressing "volatility" in terms of "alignment" is a dubious API choice. Mostly because it loses the opportunity to check for explicitly-volatile-but-accidentally-misaligned accesses. The code that was removed in the original change (4dec8fc) looked reasonable: volatile fields are accessed with explicit volatility. If it turns out their offsets are unaligned, this code should throw the error. But with this patch, such a malformed access would just be silently downgraded. I am disliking this part.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Oct 26, 2021

As I read the original change (4dec8fc) more, I am puzzled some more. Apart from fields that carry their isVolatile properties, some other things, like constants, were accessed as volatiles unconditionally. Assuming the volatility is indeed needed there, then what this patch does is breaking that property for constants that reside at unfortunate (unaligned) offsets, right? That does not seem correct.

@adinn
Copy link
Contributor

@adinn adinn commented Oct 26, 2021

Indeed, the problem here is not that the hardware is disallowing a volatile read at an unaligned address but that the compiler is being asked to generate a volatile read at an unaligned address. That is a symptom of the calling code either using an invalid layout, or otherwise computing an invalid offset.

It's no good trying to fix that symptom because, as Aleksey says, there is no fix that will preserve both volatility and single-copy atomicity. The fix needs to be made to to the root problem i.e. correct whatever code determined that a volatile field could legitimately reside at an unaligned address.

@dougxc
Copy link
Member Author

@dougxc dougxc commented Oct 26, 2021

Thanks for the clarifications @shipilev and @adinn .

I think the best thing to do is to constrain CompilerToVM.readFieldValue to only support aligned reads, the only use case that really matters. I've just pushed a change that implements and tests this.
Graal already handles attempts to read that violate the sanity checks done by JVMCI.

Copy link
Contributor

@shipilev shipilev left a comment

Thank you, this looks much safer to me. A few minor nits below.

return result.toArray(new Object[result.size()][]);
}
@DataProvider(name = "unalignedPrimitive")
public static Object[][] getUnalingedPrimitiveJavaKinds() {
Copy link
Contributor

@shipilev shipilev Oct 26, 2021

Choose a reason for hiding this comment

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

Suggested change
public static Object[][] getUnalingedPrimitiveJavaKinds() {
public static Object[][] getUnalignedPrimitiveJavaKinds() {

@@ -37,7 +37,7 @@
* @return the read value encapsulated in a {@link JavaConstant} object of {@link JavaKind} kind
* @throws IllegalArgumentException if the read is out of bounds of the object or {@code kind}
* is {@link JavaKind#Void} or not {@linkplain JavaKind#isPrimitive() primitive}
* kind or {@code bits} is not 8, 16, 32 or 64
* kind or {@code bits} is not 8, 16, 32 or 64 or the read is unaligned
Copy link
Contributor

@shipilev shipilev Oct 26, 2021

Choose a reason for hiding this comment

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

Suggested change
* kind or {@code bits} is not 8, 16, 32 or 64 or the read is unaligned
* kind or {@code bits} is not 8, 16, 32 or 64, or the read is unaligned

(not sure about this, but feels better with additional comma)

@shipilev
Copy link
Contributor

@shipilev shipilev commented Oct 26, 2021

Also, synopsis had once again diverged from the direction this PR is going.

@dougxc dougxc changed the title 8275874: [JVMCI] use volatile accessors for aligned reads in c2v_readFieldValue 8275874: [JVMCI] only support aligned reads in c2v_readFieldValue Oct 26, 2021
@dougxc
Copy link
Member Author

@dougxc dougxc commented Oct 26, 2021

Also, synopsis had once again diverged from the direction this PR is going.

I've updated the bug and PR description to match the current (final?) direction. I've also fixed the nits - thanks for your detailed eye!

@dougxc
Copy link
Member Author

@dougxc dougxc commented Oct 26, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 26, 2021

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

  • f1f5e26: 8275872: Sync J2DBench run and analyze Makefile targets with build.xml
  • 19f76c2: 8275079: Remove unnecessary conversion to String in java.net.http
  • e5cd269: 8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda proxy class info
  • 82f4aac: 8259609: C2: optimize long range checks in long counted loops
  • 574f890: 8275720: CommonComponentAccessibility.createWithParent isWrapped causes mem leak
  • 7c88a59: 8275809: crash in [CommonComponentAccessibility getCAccessible:withEnv:]
  • c9dec2f: 8273299: Unnecessary Vector usage in java.security.jgss
  • b98ed55: 8275819: [TableRowAccessibility accessibilityChildren] method is ineffective
  • 71d593e: 8275162: Use varargs in 'def' macros in mutexLocker.cpp
  • 7ca053d: 8251904: vmTestbase/nsk/sysdict/vm/stress/btree/btree010/btree010.java fails with ClassNotFoundException: nsk.sysdict.share.BTree0LLRLRLRRLR
  • ... and 14 more: https://git.openjdk.java.net/jdk/compare/7f94302ceca001ded89ba9a653bf176ef90b16cd...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 26, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 26, 2021

@dougxc Pushed as commit 2448b3f.

💡 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-compiler integrated
6 participants