RFR: 8303737: C2: Load can bypass subtype check that enforces it's from the right object type [v4]

[Tobias Hartmann]

On Fri, 15 Sep 2023 10:33:13 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This patch has 3 test cases:
>> 
>> TestAddPChainMismatchedBase.java
>> TestAddPChainMismatchedBase2.java
>> 
>> Both fail with a "Base pointers must match" assert
>> 
>> TestLoadBypassesClassCast.java
>> 
>> fails with: assert(Universe::is_in_heap(result)) failed: object not in heap
>> 
>> All failures are related to 8139771 (Eliminating CastPP nodes at Phis
>> when they all come from a unique input may cause crash). The problem
>> there was that if a Phi merges say:
>> 
>> 
>> (Phi (CastPP in) in)
>> 
>> 
>> it was transformed to `in`. Doing that would cause the control
>> dependency from the `Phi` on the null check that guards the `CastPP`
>> to be dropped. As demonstrated by the test case of 8139771, a load
>> could then float above the null check that should have guaranteed the
>> object being read from was non null.
>> 
>> The fix for 8139771 was to transform:
>> 
>> 
>> (Phi (CastPP in) in)
>> 
>> 
>> to:
>> 
>> 
>> (CastPP in)
>> 
>> 
>> That `CastPP` inherits the type of the `Phi` and is flagged as a
>> special kind of cast node so it's not eliminated if the `CastPP`
>> doesn't narrow the type of its input. As a result some depending load
>> cannot float above the cast.
>> 
>> Because that was found to be too conservative, code was added to try
>> to eliminate cast node added as replacement of a `Phi` by looking for
>> a dominating cast with the same input and a narrower type that the
>> cast. The idea was that if there was some dominating null check then
>> we would not need the cast. As TestLoadBypassesClassCast.java shows,
>> this is flawed.
>> 
>> TestLoadBypassesClassCast.java demonstrates that what used to happen
>> with a null check (a load that bypasses a null check) can still happen
>> with a type check: in the case of the test, the load of `objectField`
>> from `o` happens before `o` is checked to be of type `A` (the holder
>> of the field). When `o` is not of type `A`, the result of the load is
>> never used by compiled code but if the gc runs while the result of the
>> load is live (as it does in the test case), gc code sees something
>> that's not a valid oop and crashes.
>> 
>> In the test case, the same logic is duplicated in 2 branches of
>> ifs. There are 2 loads of `objectField`, one in each copy of the
>> logic. After transformation, the `objectField` loads lose their
>> dependency on type checks for `A` and common above a safepoint causing
>> a load from something that may not be a `A` to be live across a
>> safepoint.
>> 
>> Now looking at one copy of the logic:
>> 
>> - right above the `ob...
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   whitespace

That looks good to me. Correctness and performance testing also passed.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15595#pullrequestreview-1635381283