RFR: 8290705: StringConcat::validate_mem_flow asserts with "unexpected user: StoreI"

[Vladimir Kozlov]

On Thu, 21 Jul 2022 10:55:41 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

src/hotspot/share/opto/stringopts.cpp line 1032:

> 1030:           if (PrintOptimizeStringConcat) {
> 1031:             tty->print_cr("unexpected control use of Initialize");
> 1032:             use->dump(2);

What output of `dump(2)` you got in your case? It could be more than needed if `use` has a lot of inputs.
How about next to output only interesting info?:

ptr->in(0)->dump(); // Initialize node
use->dump(1);
tty->cr();

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

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