[aarch64-port-dev ] RFR: AArch64: org.openjdk.jcstress.tests.varhandles.DekkerTest fails

[Andrew Dinn]

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