RFR: AArch64: org.openjdk.jcstress.tests.varhandles.DekkerTest fails (original) (raw)

Andrew Dinn adinn at redhat.com
Tue Mar 6 12:23:07 UTC 2018

Could someone please review the following patch to /shared code/ which fixes an AArch64 breakage that was inadvertently introduced by JDK-8181211:

webrev: http://cr.openjdk.java.net/~adinn/8198950/webrev.00 JIRA: https://bugs.openjdk.java.net/browse/JDK-8198950

The patch applies to jdk/hs. It also applies cleanly to jdk/jdk10.

I would like it to be considered for inclusion in jdk10 if at all possible because it patches a critical error in handling of volatile reads that may result in incorrect memory synchronization.

The problem:

JDK-8181211 modified generations of load nodes in library_call.c to allow them to float when

i) the load is known to be from a heap address (i.e. an object) ii) the object reference is know (from type info) to be non-null iii) the offset for the load is known to lie within object's extent

In this case the load is created with a null control, allowing it to float.

In the case where this is a volatile load passing a null control is not really appropriate because the load should not be able to float.

Before 8181211 the load inherited its control and memory link from the leading CPUOrder memory barrier that precedes the load node. After 8181211, the null control passed in to the load create call may eventually be replaced by control flow from from an earlier node (e.g. a guarding ifnull node). This change does not actually allow the load to float because it is still dominated directly in the memory flow by the memory feed from the CPUOrder membar.

The problem this causes for AArch64 is that the back end generator attempts to replace ldr; membar_acquire sequences with ldar. In order to do so it has to spot that a load is actually volatile and associated with a trailing Acquire membar. It is currently relying on the now invalid assumption that the load will be dominated in both control and memory flow by the preceding CPUOrder membar. The generator falls back to generating ldr; membar_acquire.

Unfortunately, the other half of the generation scheme, which replaces membar_release; str; membar_volatile with an stlr instruction is still performed. The transformation is only valid if stlr instructions are paired with ldar instruction which is why the Dekker test fails.

The fix:

This patch tweaks the changes originally made to /shared code/ in 8181211 to ensure that a null control is only passed when a non-volatile load is being created (i.e. when needs_membar is false). I chose to fix it this way because:

i) it models the control flow correctly ii) this re-establishes the status quo ante for volatile loads which should, therefore be very low risk while not prejudicing the performance gain for non-volatile loads iii) the alternative tack of modifying the graph pattern matching done by the AArch64 back-end generator is a relatively complex change.

alternative jdk10 fix:

If this is not able to go in as a fix for jdk10 right now then it will still be necessary to implement an alternative fix before jdk10 is released otherwise AArch64 will be horribly broken.

The least complicated alternative is to switch the default setting for AArch64 product flag UseBarriersForVolatile to true, so that the transformations are disabled.

It would also be possible to rewrite the back end generator so that it only checks for a dominating memory feed from the CPUOrder barrier when trying to identify a volatile load.

Testing:

I reran jcstress tests on AArch64 and the change fixes the failing Dekker tests without introducing any new failures (3 opaque tests were failing before and after the patch). I ran all jdk tier1 jtreg tests on AArch64 and they passed.

As a change to shared code this also needs testing against the submit tree to ensure it does not break x86 code (which I am in the process of doing).

regards,

Andrew Dinn

Senior Principal Software Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

More information about the hotspot-dev mailing list