RFR: 8290705: StringConcat::validate_mem_flow asserts with "unexpected user: StoreI" [v2]

[Xin Liu]

On Fri, 22 Jul 2022 05:38:01 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> C2's string concatenation optimization (`OptimizeStringConcat`) does not correctly handle side effecting instructions between  StringBuffer Allocate/Initialize and the call to the constructor. In the failing test, see `SideEffectBeforeConstructor::test`, a `result` field is incremented just before the constructor is invoked. The string concatenation optimization still merges the allocation, constructor and `toString` calls and incorrectly re-wires the store to before the concatenation. As a result, passing `null` to the constructor will incorrectly increment the field before throwing a NullPointerException. With a debug build, we hit an assert in `StringConcat::validate_mem_flow` due to the unexpected field store. This is an old bug and an extreme edge case as javac would not generate such code.
>> 
>> The following comment suggests that this case should be covered by `StringConcat::validate_control_flow()`:
>> https://github.com/openjdk/jdk/blob/3582fd9e93d9733c6fdf1f3848e0a093d44f6865/src/hotspot/share/opto/stringopts.cpp#L834-L838
>> 
>> However, the control flow analysis does not catch this case. I added the missing check.
>> 
>> Thanks,
>> Tobias
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Modified debug printing code

This fix is reasonable.  LGTM. (I am not a reviewer). 

A side node to myself:  any nodes with side effect between Initialize and <init>() must commit because <init> may throw an exception.

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

Marked as reviewed by xliu (Committer).

PR: https://git.openjdk.org/jdk/pull/9589