8362117: C2: compiler/stringopts/TestStackedConcatsAppendUncommonTrap.java fails with a wrong result due to invalidated liveness assumptions for data phis #27028

danielogh · 2025-09-01T07:04:25Z

This PR addresses a wrong compilation during string optimizations.

During stacked string concatenation of two StringBuilder links SB1 and SB2, the pattern "append -> Phi -> Region -> (True, False) -> If -> Bool -> CmpP -> Proj (Result) -> toString" may be observed, where toString is the end of SB1, and the simple diamond is part of SB2.

After JDK-8291775, the Bool test to the diamond If is set to a constant zero to allow for folding the simple diamond away during IGVN, while not letting the top() value from the result projection of SB1 propagate through the graph too quickly. The assumption was that any data Phi of the Region would go away during PhaseRemoveUseless as they are no longer live -- I think that in the case of JDK-8291775, the user of phi was the constructor of SB2. However, in the attached test case, the Phi stays live as it's a parameter (input to an append) of SB2 and will be used during the transformation in copy_string. When the diamond region is later folded, the Phi's user picks up the wrong input corresponding to the false branch.

The proposed solution is to disable the stacked concatenation optimization for this specific pattern. This might be pragmatic as it's an edge case and there's already a bug tail: JDK-8271341-> JDK-8291775 -> JDK-8362117.

Testing: T1-3 (aed5952).

Extra testing: ran T1-3 on Linux with an instrumented build and verified that the pattern I am excluding in this PR is not seen during any other compilation than that of the proposed regression test.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8362117: C2: compiler/stringopts/TestStackedConcatsAppendUncommonTrap.java fails with a wrong result due to invalidated liveness assumptions for data phis (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27028/head:pull/27028
$ git checkout pull/27028

Update a local copy of the PR:
$ git checkout pull/27028
$ git pull https://git.openjdk.org/jdk.git pull/27028/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27028

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27028.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-01T07:05:34Z

👋 Welcome back dskantz! 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 · 2025-09-01T07:06:52Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-09-01T07:07:36Z

@danielogh The following label will be automatically applied to this pull request:

hotspot-compiler

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

mlbridge · 2025-09-01T07:11:24Z

Webrevs

01: Full - Incremental (5638f221)
00: Full (8e93056d)

eme64 · 2025-09-01T14:05:58Z

src/hotspot/share/opto/stringopts.cpp

@@ -1062,6 +1067,29 @@ bool StringConcat::validate_control_flow() {
        // XXX should check for possibly merging stores.  simple data merges are ok.
        // The IGVN will make this simple diamond go away when it
        // transforms the Region. Make sure it sees it.
+
+        // First exclude the following pattern:
+        // append -> Phi -> Region -> (True, False) -> If -> Bool -> CmpP -> Proj (Result) -> toString;


2 things:

I was a bit confused about the -> directionality. Just to confirm: toString happens first, then the if-diamond, then the append, right? If yes: I would have reversed the order here. Then again, I'm not super familiar with string opts, so maybe the convention is different here than elsewhere.

Are you sure this can only happen with diamonds? What about nested diamonds?

Ah I see the condition above already checks that it can only be a diamond.

eme64 · 2025-09-01T14:10:56Z

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsPhiUseOfDiamondRegion.java

+    static String f() {
+        String s = "a";
+        s = new StringBuilder().append(s).append(s).toString();
+        s = new StringBuilder().append(s).append((s == "xx") ? s : "aa").toString();
+        Asserts.assertEQ(s, "aaaa"); // in particular, we should not have s.equals("aaxx").
+        return s;
+    }
+}


I wonder if we could write some kind of StringBuilder fuzzer. Not saying it has to happen as part of this fix. But it seems we have issues with very similar patterns. And they seem quite basic: chains, diamonds, etc.

Would probably not be too hard to use the template framework to generate some random shapes, and verify the result the compiled code gives vs the interpreter.

I think this is a good idea for sure.

eme64 · 2025-09-01T14:14:46Z

@danielogh Thanks for working on this!
I'd love to review, but I'm not very familiar with string opts. Would you mind explaining in a bit more detail what would have gone wrong here?

Phi stays live as it's a parameter (input to an append) of SB2 and will be used during the transformation in copy_string. When the diamond region is later folded, the Phi's user picks up the wrong input corresponding to the false branch.

eme64 · 2025-09-01T14:24:46Z

src/hotspot/share/opto/stringopts.cpp

@@ -1062,6 +1067,29 @@ bool StringConcat::validate_control_flow() {
        // XXX should check for possibly merging stores.  simple data merges are ok.
        // The IGVN will make this simple diamond go away when it
        // transforms the Region. Make sure it sees it.
+
+        // First exclude the following pattern:
+        // append -> Phi -> Region -> (True, False) -> If -> Bool -> CmpP -> Proj (Result) -> toString;


Ah I see the condition above already checks that it can only be a diamond.

eme64 · 2025-09-01T14:24:51Z

src/hotspot/share/opto/stringopts.cpp

+        Node* v1 = ptr->in(1)->in(0)->in(1)->in(1)->in(1);
+        Node* v2 = ptr->in(1)->in(0)->in(1)->in(1)->in(2);


You may want to use some intermediate results and give them names.
For example:
Node* iff = ptr->in(1)->in(0)
You seem to make an assumption that the input of the bool is a cmp, right? Did you check that? Or is it somehow guaranteed? What if in some edge-case of an edge-case it is something else that has only one input? Could that happen?

I'm not sure if there is a guarantee, but it appears to be a pre-existing assumption that is asserted later in eliminate_unneeded_control:

jdk/src/hotspot/share/opto/stringopts.cpp

Line 268 in b06459d

assert(cmp->is_Cmp(), "unexpected if shape");

dean-long · 2025-09-05T01:39:42Z

This seems to be missing the root cause of the problem. From what I can tell, we have two string concats here, with the 2nd dependent on the first. But we incorrectly decide to coalesce them into a single concat, which then causes havoc when eliminate_unneeded_control() starts nuking edges without regard for the dependency.

dean-long · 2025-09-05T01:56:28Z

Hmm, I see now that validate_control_flow() does limit coalescing, but I'm worried that the pattern matching may not catch all problematic cases.

danielogh · 2025-09-05T06:30:19Z

Yes, validate_control_flow() is used for individual and coalesced concatenations, but just re-using those same checks for coalesced concatenations has been shown to not be sufficient in recent bugs -- in particular when the result of SB1 is used in unexpected ways in SB2. I am not convinced that we have covered all the cases yet. Would it be an idea to fix this issue and then go for the fuzzing approach next to cover more patterns (follow-up RFE), or is there a more general pattern we could prevent here already?

danielogh added 4 commits August 29, 2025 06:14

fix

9fc209e

revert to unfolded version of is_diamond

0cc3e9b

add an assert

aed5952

ws

8e93056

openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org rfr Pull request is ready for review labels Sep 1, 2025

eme64 reviewed Sep 1, 2025

View reviewed changes

danielogh added 2 commits September 3, 2025 06:11

direction convention

7f5eb0b

store intermediate calculations

5638f22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8362117: C2: compiler/stringopts/TestStackedConcatsAppendUncommonTrap.java fails with a wrong result due to invalidated liveness assumptions for data phis #27028

8362117: C2: compiler/stringopts/TestStackedConcatsAppendUncommonTrap.java fails with a wrong result due to invalidated liveness assumptions for data phis #27028

danielogh commented Sep 1, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Sep 1, 2025

Uh oh!

openjdk bot commented Sep 1, 2025

Uh oh!

openjdk bot commented Sep 1, 2025

Uh oh!

mlbridge bot commented Sep 1, 2025 •

edited

Loading

Uh oh!

eme64 Sep 1, 2025

Uh oh!

eme64 Sep 1, 2025

Uh oh!

eme64 Sep 1, 2025

Uh oh!

danielogh Sep 1, 2025

Uh oh!

eme64 commented Sep 1, 2025

Uh oh!

eme64 Sep 1, 2025

Uh oh!

eme64 Sep 1, 2025

Uh oh!

danielogh Sep 1, 2025

Uh oh!

dean-long commented Sep 5, 2025

Uh oh!

dean-long commented Sep 5, 2025

Uh oh!

danielogh commented Sep 5, 2025

Uh oh!

Uh oh!

		Node* v1 = ptr->in(1)->in(0)->in(1)->in(1)->in(1);
		Node* v2 = ptr->in(1)->in(0)->in(1)->in(1)->in(2);

8362117: C2: compiler/stringopts/TestStackedConcatsAppendUncommonTrap.java fails with a wrong result due to invalidated liveness assumptions for data phis #27028

Are you sure you want to change the base?

8362117: C2: compiler/stringopts/TestStackedConcatsAppendUncommonTrap.java fails with a wrong result due to invalidated liveness assumptions for data phis #27028

Conversation

danielogh commented Sep 1, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Sep 1, 2025

Uh oh!

openjdk bot commented Sep 1, 2025

Uh oh!

openjdk bot commented Sep 1, 2025

Uh oh!

mlbridge bot commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

eme64 Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

eme64 Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

eme64 Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

danielogh Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

eme64 commented Sep 1, 2025

Uh oh!

eme64 Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

eme64 Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

danielogh Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

dean-long commented Sep 5, 2025

Uh oh!

dean-long commented Sep 5, 2025

Uh oh!

danielogh commented Sep 5, 2025

Uh oh!

Uh oh!

danielogh commented Sep 1, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 1, 2025 •

edited

Loading