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

[Christian Hagedorn]

On Wed, 6 Sep 2023 14:55:01 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 `objectField` load, a `Phi` (that merges the 2
>   banches of `if (flag) {` and has type non null Object) is tested to
>   be a subtype of `A`....

Nice analysis! Took me some time to grasp it but after thinking about it some more after our offline discussion, I think it's a good way to fix this problem. 

Apart from some small code style comments, it looks good to me!

src/hotspot/share/opto/castnode.cpp line 112:

> 110:     return false;
> 111:   }
> 112:   if (((ConstraintCastNode&) n)._dependency != _dependency) {

Maybe you can put `((ConstraintCastNode&) n)._dependency` into a variable as you are using it a couple of times.

src/hotspot/share/opto/castnode.hpp line 58:

> 56:   public:
> 57:   ConstraintCastNode(Node* n, const Type* t, ConstraintCastNode::DependencyType dependency,
> 58:                      const TypeTuple* types)

I suggest to rename `types` to `extra_types` to match the field name. Same in the other `make*` methods.

src/hotspot/share/opto/castnode.hpp line 107:

> 105:   bool higher_equal_types(PhaseGVN* phase, const Node* other) const;
> 106: 
> 107:   int nb_extra_types() const {

Maybe we could rename this to `extra_types_count()` to make it more clear.

src/hotspot/share/opto/castnode.hpp line 109:

> 107:   int nb_extra_types() const {
> 108:     return _extra_types == nullptr ? 0 : _extra_types->cnt();
> 109:   }

New line:
Suggestion:

  }

src/hotspot/share/opto/cfgnode.cpp line 2136:

> 2134:       // going away
> 2135:       Node* cast = nullptr;
> 2136:       const TypeTuple* extra_types = collect_types(phase, r, phi_type);

Completely optional but you could just directly use `bottom_type()` and `in(0)` inside `collect_types` instead of passing it with `r` and `phi_type`.

src/hotspot/share/opto/cfgnode.cpp line 2575:

> 2573: // Sort the types using an arbitrary order so a list of some types always hashes to the same TypeTuple (and TypeTuple
> 2574: // pointer comparison is enough to tell if 2 list of types are the same or not)
> 2575: const TypeTuple* PhiNode::collect_types(PhaseGVN* phase, const Node* r, const Type* phi_type) const {

I suggest to rename `r` -> `region`.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15595#pullrequestreview-1626554813
PR Review Comment: https://git.openjdk.org/jdk/pull/15595#discussion_r1325769268
PR Review Comment: https://git.openjdk.org/jdk/pull/15595#discussion_r1325757382
PR Review Comment: https://git.openjdk.org/jdk/pull/15595#discussion_r1325741552
PR Review Comment: https://git.openjdk.org/jdk/pull/15595#discussion_r1325741930
PR Review Comment: https://git.openjdk.org/jdk/pull/15595#discussion_r1325774629
PR Review Comment: https://git.openjdk.org/jdk/pull/15595#discussion_r1325737586