RFR: 8295486: Inconsistent constant field values observed during compilation

[Tobias Hartmann]

On Thu, 5 Jan 2023 13:26:11 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> We hit a "not monotonic" assert because the new type of a load from a stable final field is more narrow than the old type which contradicts the assumption that types should only go from TOP to BOTTOM during CCP:
> 
> old: `narrowoop: java/lang/Integer:BotPTR:exact *`
> new: `narrowoop: java/lang/Integer java.lang.Integer {0x000000062c41e548} ...`
> 
> or 
> 
> old: `narrowoop: java/lang/Integer java.lang.Integer {0x000000062c41e538} ...`
> new: `narrowoop: java/lang/Integer java.lang.Integer {0x000000062c41e548} ...`
> 
> The problem is that a stable field can be (re-)initialized during compilation and since the value is not cached, contradicting types can be observed. In `LoadNode::Value`, we re-read the field value each time: 
> 
> https://github.com/openjdk/jdk/blob/872384707e89d03ede655aad16f360dc94f10152/src/hotspot/share/opto/memnode.cpp#L1994-L1997
> 
> https://github.com/openjdk/jdk/blob/872384707e89d03ede655aad16f360dc94f10152/src/hotspot/share/opto/type.cpp#L332-L337
> 
> The same problem exists for loads from stable arrays:
> https://github.com/openjdk/jdk/blob/872384707e89d03ede655aad16f360dc94f10152/src/hotspot/share/opto/memnode.cpp#L1923
> 
> Caching the field value is not feasible as it would require a cache per ciInstance for all the fields and per ciArray for all the elements. Alternatively, we could keep track of the lookup and only do it once but that would also be lots of additional complexity for a benign issue.
> 
> Instead, I propose to skip verification during CCP when folding loads from stable fields. Non-stable, constant fields are not affected as `null` is a valid value for them and they would already be folded before CCP.
> 
> Thanks,
> Tobias

Vladimirs, Jatin, thanks for the reviews and discussion!

The question is what we want to achieve with this patch:
1) Prevent C2 from crashing / asserting when observing inconsistent field values
2) Prevent C2 from applying optimizations based on inconsistent field values
3) Prevent C2 from constant folding stale values

I think my proposed patch achieves 1) which is also the only issue we ever observed in real code: Racy initialization of method handles for indy string concat in core libraries code. The verification code proposed by ([JDK-8024042](https://bugs.openjdk.org/browse/JDK-8024042)) would catch this. Constant folding a stale value does not matter here, semantically, we can use any of the equivalent method handles that we observe. It just confuses CCP verification. Convergence of CCP is also not an issue because we don't update a node's type indefinitely but only if the type of a relevant input node changed.

Regarding 2): Couldn't any such wrong/undesired behaviour happen with execution in the interpreter as well? The only difference would be that optimized C2 compiled code would **always** behave that way. But according to the specification for `@Stable`, that's okay:

https://github.com/openjdk/jdk/blob/5c8c67c523279de728248f54382c40fb20d0ab63/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java#L74-L80

Also, doesn't the above specification mean that all non-synchronized initializations of stable fields can lead to undefined behavior?

> Imaging you read same stable field on separate paths and then compare constants.

That will also happen with interpreted code when the stable field is written between the two reads.

The only complete solution would be 3), which, as Vladimir I. already pointed out, does not seem feasible given that we would need to intercept all writes to stable fields and array elements.

I'm not against caching but I'm wondering how much sense it makes to apply an expensive and complex (partial) fix to C2 while C1 and the interpreter are still affected. Shouldn't the remaining issues be fixed in Java code if undefined/unexpected behavior is ever observed?

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

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