RFR: 8339694: ciTypeFlow does not correctly handle unresolved constant dynamic of array type [v4]

[Tobias Hartmann]

On Wed, 16 Oct 2024 07:48:35 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> After [JDK-8280473](https://bugs.openjdk.org/browse/JDK-8280473), the CI will represent an unresolved constant dynamic as unloaded `ciConstant` of a `java/lang/Object` `ciInstanceKlass`:
>> 
>> https://github.com/openjdk/jdk/blob/6133866150cf6131ab578f1537f84c239703fa67/src/hotspot/share/ci/ciEnv.cpp#L721-L723
>> 
>> Now with a constant dynamic of array type, we hit an assert in type flow analysis on `*astore` because the type is not a primitive array type:
>> 
>> https://github.com/openjdk/jdk/blob/6133866150cf6131ab578f1537f84c239703fa67/src/hotspot/share/ci/ciTypeFlow.cpp#L967-L971
>> 
>> ->
>> https://github.com/openjdk/jdk/blob/6133866150cf6131ab578f1537f84c239703fa67/src/hotspot/share/ci/ciTypeFlow.hpp#L343-L346
>> 
>> The same problem exists with object array types. New `TestUnresolvedConstantDynamic` triggers both.
>> 
>> Similar to `unloaded_ciinstance` used by [JDK-8280473](https://bugs.openjdk.org/browse/JDK-8280473) (see [here](https://github.com/openjdk/jdk/commit/88fc3bfdff7f89a02fcfb16909df144e6173c658#diff-7c032de54e85754d39e080fd24d49b7469543b163f54229eb0631c6b1bf26450R784)), we could now add an unloaded `ciObjArray` and `ciTypeArray` to represent an unresolved dynamic constant of array type. We would then add a trap when parsing the array access. However, this requires rather invasive changes for this edge case and it wouldn't make much sense because even though type flow analysis would continue, we would add a trap when parsing `_ldc` anyway if the constant is not loaded: 
>> https://github.com/openjdk/jdk/blob/037f11b864734734dd7fbce029b2e8b4bc17f3ab/src/hotspot/share/opto/parse2.cpp#L1962-L1979
>> 
>> I propose to do the same in type flow analysis, i.e., trap early in `_ldc` when the constant is not loaded.
>> 
>> Thanks,
>> Tobias
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   More cleanups

I cleaned this up a bit more. The `iter().is_in_error()` branch in parsing is already dead without my changes because type flow will always trap. With my changes, the `!constant.is_loaded()` branch is now dead as well. I removed both and added asserts.

Given that even without my changes, we would always trap for an unresolved constant during parsing, I don't think continuing type flow analysis does make sense. In theory, it could even have a negative impact. Here's an example:


Class obj = ...;
if (b) {
    obj = LoadedClass.class;
} else {
    // We always trap here during parsing
    obj = UnloadedClass.class;
}


If we don't trap in the else branch during type flow analysis, the type of `obj` will be set to unloaded after the if. This is suboptimal because we always trap in the else branch during parsing, so the type can never be unloaded.

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

PR Comment: https://git.openjdk.org/jdk/pull/21470#issuecomment-2416013432