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

[Tobias Hartmann]

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

This pull request has now been integrated.

Changeset: 61e072d1
Author:    Tobias Hartmann <thartmann at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/61e072d11c8e0cb5879bb733ed1fdd2144326bfd
Stats:     122 lines in 3 files changed: 122 ins; 0 del; 0 mod

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

Reviewed-by: kvn, xliu

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

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