[lworld] RFR: 8264414: [lworld] [AArch64] TestBufferTearing.java fails with C1

[Nick Gasson]

On Thu, 1 Apr 2021 12:52:07 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> We see failures like this on AArch64 when MyValue.incrementAndCheck() is
>> compiled with C1:
>> 
>>   java.lang.RuntimeException: Inconsistent field values: expected 0 to equal 675128
>>         at jdk.test.lib.Asserts.fail(Asserts.java:594)
>>         at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
>>         at jdk.test.lib.Asserts.assertEQ(Asserts.java:178)
>>         at compiler.valhalla.inlinetypes.MyValue.incrementAndCheck(TestBufferTearing.java:81)
>>         at compiler.valhalla.inlinetypes.TestBufferTearing$Runner.run(TestBufferTearing.java:124)
>> 
>> The barrier that is usually inserted on return from a method that wrote
>> final fields should be sufficient to prevent another thread seeing the
>> zero-initialised intermediate state.  However this barrier isn't
>> inserted at the moment because method()->is_object_constructor() is
>> false for primitive class constructors.
>> 
>> C2 has a similar guard around the memory barrier in Parse::do_exits().
>> I'm not sure if that needs amending as well but I've not seen any
>> failures due to it.
>
> Hi Nick,
> 
> I think we also need a barrier in `GraphBuilder::access_field` when loading from a flattened field and in `GraphBuilder::load_indexed` (the initialization code is emitted in `LIRGenerator::access_flattened_array`) when loading from a flattened array.
> 
> It would be good to have tests that triggers this.
> 
> Best regards,
> Tobias

So I've managed to generate the following test which triggers this:

public class BarrierTest {

    public static Point[] points = new Point[1];
    static volatile boolean running = true;

    public static void writePoint(int i) {
        Rect r = new Rect(new Point(i, i), new Point(i + 1, i + 1));
        points[0] = r.a;   // Load from flattened field here
    }

    private static void checkMissingBarrier() {
        while (running) {
            // Should not see zero-initialised object here
            if (points[0].x == 0 || points[0].y == 0)
                throw new IllegalStateException();
        }
    }
    
    public static void main(String[] args) throws InterruptedException {
        points[0] = new Point(1, 1);
        
        Thread[] threads = new Thread[10];
        for (int i = 0; i < 10; i++) {
            threads[i] = new Thread(BarrierTest::checkMissingBarrier);
            threads[i].start();
        }
                
        for (int i = 2; i < 1_000_000; i++)
            writePoint(i);
        
        running = false;
        
        for (int i = 0; i < 10; i++)
            threads[i].join();
    }
}

Where `Rect` and `Point` are primitive classes with the obvious definition. Then `checkMissingBarrier()` will throw an exception if run with `-XX:InlineFieldMaxFlatSize=-1  -XX:FlatArrayElementMaxSize=0 -XX:TieredStopAtLevel=1`. The field access `r.a` creates an on-heap copy of the flattened field and then stores that reference into the `points` array (which can't be flattened because `FlatArrayElementMaxSize=0`). Without a memory barrier after copying the flattened field contents in `GraphBuilder::access_field` another thread can read through that reference and see the zero-initialised memory. 

I'm not sure if there's an easier way of triggering this without having mis-matched InlineFieldMaxFlatSize and FlatArrayElementMaxSize? To see this I think we need to read from a flattened field and then store into a non-flattened field of the same type that's accessible by another thread.

-------------

PR: https://git.openjdk.java.net/valhalla/pull/376