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

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

Closed
wants to merge 3 commits into from

Conversation

nick-arm
Copy link
Member

@nick-arm nick-arm commented Apr 1, 2021

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.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8264414: [lworld] [AArch64] TestBufferTearing.java fails with C1

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 376

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

Using diff file

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

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.
@bridgekeeper
Copy link

bridgekeeper bot commented Apr 1, 2021

👋 Welcome back ngasson! A progress list of the required criteria for merging this PR into lworld 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 bot commented Apr 1, 2021

@nick-arm 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:

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

Reviewed-by: thartmann, fparain

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

  • e9c78ce: 8265118: [lworld] C1 should optimize inline type checkcasts
  • 4f88959: 8264895: [lworld] assert(!InstanceKlass::cast(receiver_klass)->is_not_initialized()) failed: receiver_klass must be initialized
  • 571322e: 8264977: [lworld] A primitive class field by name val confuses javac
  • f3950ad: 8264978: [lworld] Various small code cleanups
  • c68ce89: 8244227: [lworld] Explore an implementation where the reference projection and value projection types are backed by a single class symbol
  • f816f29: 8264085: [lworld] C2 compilation fails with assert "inline type should be loaded"
  • 937f9b3: 8264586: [lworld] C2 compilation fails due to infinite loop in PhaseIterGVN::optimize

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @fparain) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@mlbridge
Copy link

mlbridge bot commented Apr 1, 2021

Webrevs

@TobiHartmann
Copy link
Member

TobiHartmann commented Apr 1, 2021

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

@nick-arm
Copy link
Member Author

nick-arm commented Apr 8, 2021

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.

@nick-arm
Copy link
Member Author

nick-arm commented Apr 9, 2021

@TobiHartmann I've added the two missing membars and a test.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Very nice that you were able to come up with a regression test!

Regarding the combination of InlineFieldMaxFlatSize and FlatArrayElementMaxSize, couldn't you achieve the same result by using an Object or Interface array/field to prevent flattening?

The fix looks good to me but I think we also need membars in GraphBuilder::access_field (there are two places where we create a buffer via new NewInlineTypeInstance). Would be great if you could add a regression test for that as well.

@nick-arm
Copy link
Member Author

nick-arm commented Apr 12, 2021

Regarding the combination of InlineFieldMaxFlatSize and FlatArrayElementMaxSize, couldn't you achieve the same result by using an Object or Interface array/field to prevent flattening?

Yes that's much simpler, thanks.

The fix looks good to me but I think we also need membars in GraphBuilder::access_field (there are two places where we create a buffer via new NewInlineTypeInstance). Would be great if you could add a regression test for that as well.

Yes there's also the case for the "delayed" load-indexed as in array[n].field. I saw that before but for some reason thought it didn't need the membar. I've added a test for that too so all three cases fail without the corresponding membar.

While I was testing that I found it would fail occasionally before while writeRefs() was running in the interpreter before C1 compiled it. So I think there's also a missing membar at the end of InterpreterMacroAssembler::read_inlined_field() which I've added (not necessary for x86).

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Very nice. The compiler changes look good to me!

@fparain could you take a look at the interpreter changes?

Copy link
Collaborator

@fparain fparain left a comment

Looks good to me.
Thank you for finding and fixing it!

Fred

@nick-arm
Copy link
Member Author

nick-arm commented Apr 14, 2021

/integrate

@openjdk openjdk bot added the sponsor label Apr 14, 2021
@openjdk
Copy link

openjdk bot commented Apr 14, 2021

@nick-arm
Your change (at version 0e41b24) is now ready to be sponsored by a Committer.

@TobiHartmann
Copy link
Member

TobiHartmann commented Apr 15, 2021

/sponsor

@openjdk
Copy link

openjdk bot commented Apr 15, 2021

@TobiHartmann @nick-arm Since your change was applied there have been 7 commits pushed to the lworld branch:

  • e9c78ce: 8265118: [lworld] C1 should optimize inline type checkcasts
  • 4f88959: 8264895: [lworld] assert(!InstanceKlass::cast(receiver_klass)->is_not_initialized()) failed: receiver_klass must be initialized
  • 571322e: 8264977: [lworld] A primitive class field by name val confuses javac
  • f3950ad: 8264978: [lworld] Various small code cleanups
  • c68ce89: 8244227: [lworld] Explore an implementation where the reference projection and value projection types are backed by a single class symbol
  • f816f29: 8264085: [lworld] C2 compilation fails with assert "inline type should be loaded"
  • 937f9b3: 8264586: [lworld] C2 compilation fails due to infinite loop in PhaseIterGVN::optimize

Your commit was automatically rebased without conflicts.

Pushed as commit 2e6c276.

💡 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
3 participants