RFR: 8362117: C2: compiler/stringopts/TestStackedConcatsAppendUncommonTrap.java fails with a wrong result due to invalidated liveness assumptions for data phis [v2]

[Daniel Skantz]

On Wed, 3 Sep 2025 08:02:04 GMT, Daniel Skantz <dskantz at openjdk.org> wrote:

>> 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.
>
> Daniel Skantz has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - store intermediate calculations
>  - direction convention

Thanks! I get the impression that JDK-6892658 was written with no complex dependencies in mind, and that the result projection was assumed to be directly used by the constructor or as a parameter in the second StringBuilder chain, so it would have been safe to remove. In JDK-7179138, we added support for some null checks, which we might also want to keep supporting.

In the string concat optimization, the toString and append calls are all removed, and instead individual stores or array copies are emitted for each of the arguments that make up the StringBuilder. I don't see an easy way to retain the information needed to support arbitrary conditional code such as checks for equality on this result. Previous attempts to support additional patterns have been complicated: JDK-7179138 has several follow-up bugs, and JDK-8341696 was backed out.

I am thinking to either go for a spot fix (in this PR I am validating an assumption made in JDK-8291775), or possibly a more general constraint procedure where the result projection uses are bounded to what is known and safe: (potentially null-checked) uses in constructor or append calls of the second StringBuilder.

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

PR Comment: https://git.openjdk.org/jdk/pull/27028#issuecomment-3451041567