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

[Roland Westrelin]

> 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`....

Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:

 - review
 - Merge branch 'master' into JDK-8303737
 - Update src/hotspot/share/opto/castnode.hpp
   
   Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
 - Update test/hotspot/jtreg/compiler/controldependency/TestLoadBypassesClassCast.java
   
   Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
 - comments
 - fix & test

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/15595/files
  - new: https://git.openjdk.org/jdk/pull/15595/files/558dcde3..bed907cb

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=15595&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15595&range=01-02

  Stats: 19552 lines in 660 files changed: 9769 ins; 6505 del; 3278 mod
  Patch: https://git.openjdk.org/jdk/pull/15595.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/15595/head:pull/15595

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