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

rwestrel · 2023-09-06T14:55:01Z

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.
the subtype checks is split throught Phi but the CheckCastPP
stays at the merge point.
The Phi is replaced by a CastPP to non null Object.
The CastPP is replaced by a dominating CastPP to non null that's
at the beginning of the method.
The if (flag) goes away and the CheckCastPP becomes control
dependent on the range check for the array load that's above. That
range check is replaced by a dominating range check, itself at the
beginning of the method. Now the CheckCastPP is above any subtype
check. The load can float as high as the CheckCastPP.

The flaw here I think is the logic from 8139771 that assumes a cast
node can be replaced by any cast with a narrower or equal type. In
that case, the cast replaces a Phi that merges types not null A
and not null Object which as a result has type non null Object. It
would be fine to replace the cast with a dominating cast that's
narrower that the type of each input to the Phi (so in this case non
null A and non null Object). That's not the case for the cast of
the null check. The problem is when the Phi is transformed to a
cast, the types of its inputs are lost. What I propose in the fix is
to attach those types to the cast and change the logic that decides
whether a cast is redundant (whether because of the type of its input
or because of a dominating cast) so it goes over the list of attached
types.

How does that relate to the other 2 test cases that fail because in a
chain of Addp nodes, not all nodes have the same base (some casted,
some not)? What I found looking at those test cases is that the graph
contained some casts created as a result of the PhiNode::Ideal that
were "obviously" useless but that are conservatively kept. Those casts
were involved in a chain of transformations that cause the assert. The
new logic does a better job of removing the casts and make the
failures go away. So the change doesn't directly fix it but, at the
very least, it makes it less likely and I'm not sure there's more work
needed at this point unless we see that failure again.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8303737: C2: Load can bypass subtype check that enforces it's from the right object type (Bug - P3)

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to 8c3e9f2c

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15595/head:pull/15595
$ git checkout pull/15595

Update a local copy of the PR:
$ git checkout pull/15595
$ git pull https://git.openjdk.org/jdk.git pull/15595/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15595

View PR using the GUI difftool:
$ git pr show -t 15595

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15595.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-06T14:56:43Z

👋 Welcome back roland! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-09-06T14:59:52Z

@rwestrel The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-09-06T15:04:18Z

Webrevs

test/hotspot/jtreg/compiler/controldependency/TestLoadBypassesClassCast.java

chhagedorn

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!

chhagedorn · 2023-09-14T10:22:25Z

src/hotspot/share/opto/cfgnode.cpp

+// Collect types at casts that are going to be eliminated at that Phi and store them in a TypeTuple.
+// Sort the types using an arbitrary order so a list of some types always hashes to the same TypeTuple (and TypeTuple
+// pointer comparison is enough to tell if 2 list of types are the same or not)
+const TypeTuple* PhiNode::collect_types(PhaseGVN* phase, const Node* r, const Type* phi_type) const {


I suggest to rename r -> region.

chhagedorn · 2023-09-14T10:26:00Z

src/hotspot/share/opto/castnode.hpp

+
+  bool higher_equal_types(PhaseGVN* phase, const Node* other) const;
+
+  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

chhagedorn · 2023-09-14T10:39:45Z

src/hotspot/share/opto/castnode.hpp

-  ConstraintCastNode(Node *n, const Type *t, DependencyType dependency)
-    : TypeNode(t,2), _dependency(dependency) {
+  ConstraintCastNode(Node* n, const Type* t, ConstraintCastNode::DependencyType dependency,
+                     const TypeTuple* types)


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

chhagedorn · 2023-09-14T10:51:22Z

src/hotspot/share/opto/castnode.cpp

+  if (!TypeNode::cmp(n)) {
+    return false;
+  }
+  if (((ConstraintCastNode&) n)._dependency != _dependency) {


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

chhagedorn · 2023-09-14T10:56:34Z

src/hotspot/share/opto/cfgnode.cpp

@@ -2133,10 +2133,12 @@ Node *PhiNode::Ideal(PhaseGVN *phase, bool can_reshape) {
      // Add casts to carry the control dependency of the Phi that is
      // going away
      Node* cast = nullptr;
+      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.

openjdk · 2023-09-14T11:05:19Z

@rwestrel This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: chagedorn, thartmann

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 81 new commits pushed to the master branch:

0f77d25: 8315684: Parallelize sun/security/util/math/TestIntegerModuloP.java
311c746: 8316859: RISC-V: Disable detection of V through HWCAP
0f0c5b2: 8316648: jrt-fs.jar classes not reproducible between standard and bootcycle builds
3d6e775: 8316669: ImmutableOopMapSet destructor not called
837783c: 8316670: Remove effectively unused nmethodBucket::_count
89e068b: 8316556: Fix typos in java.desktop
481cfc7: 8287325: AArch64: fix virtual threads with -XX:UseBranchProtection=pac-ret
f0ff001: 8315742: Open source several Swing Scroll related tests
a2391a9: 8316053: Open some swing tests 3
d2d7d9a: 8315882: Open some swing tests 2
... and 71 more: https://git.openjdk.org/jdk/compare/62c0a1b9ac6462233f3ee06af470be9844e9e226...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

…ClassCast.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

TobiHartmann

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

chhagedorn

Looks good, thanks for the updates!

rwestrel · 2023-09-26T06:52:44Z

/integrate

openjdk · 2023-09-26T06:55:06Z

Going to push as commit 52983ed.
Since your change was applied there have been 92 commits pushed to the master branch:

9e6cb62: 8316851: Add @sealedGraph to Executable
3fe6e0f: 8308479: [s390x] Implement alternative fast-locking scheme
e2e8e8e: 8312136: Modify runtime/ErrorHandling/TestDwarf.java to split dwarf and decoder testing
0dce4c1: 8313220: Remove Windows specific workaround in LCMS.c for _snprintf
e5f05b5: 8312191: ColorConvertOp.filter for the default destination is too slow
be9cc73: 8315871: Opensource five more Swing regression tests
b65f4f7: 8313403: Remove unused 'mask' field from JFormattedTextField
e3201d1: 8310631: test/jdk/sun/nio/cs/TestCharsetMapping.java is spuriously passing
9291b46: 8313804: JDWP support for -Djava.net.preferIPv6Addresses=system
afa4833: 8271268: Fix Javadoc links for Stream.mapMulti
... and 82 more: https://git.openjdk.org/jdk/compare/62c0a1b9ac6462233f3ee06af470be9844e9e226...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-26T06:55:17Z

@rwestrel Pushed as commit 52983ed.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

rwestrel added 2 commits September 6, 2023 15:32

fix & test

6ccc86e

comments

003c175

openjdk bot added the rfr Pull request is ready for review label Sep 6, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Sep 6, 2023

turbanoff reviewed Sep 6, 2023

View reviewed changes

test/hotspot/jtreg/compiler/controldependency/TestLoadBypassesClassCast.java Outdated Show resolved Hide resolved

chhagedorn approved these changes Sep 14, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 14, 2023

rwestrel and others added 3 commits September 15, 2023 10:46

Update test/hotspot/jtreg/compiler/controldependency/TestLoadBypasses…

ff26dc2

…ClassCast.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

Update src/hotspot/share/opto/castnode.hpp

558dcde

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

Merge branch 'master' into JDK-8303737

0eba398

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 15, 2023

rwestrel added 2 commits September 15, 2023 11:16

review

bed907c

whitespace

8c3e9f2

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 15, 2023

TobiHartmann approved these changes Sep 20, 2023

View reviewed changes

chhagedorn approved these changes Sep 25, 2023

View reviewed changes

rwestrel added 2 commits September 25, 2023 15:17

Merge branch 'master' into JDK-8303737

a85f0b9

rename

42b430c

chhagedorn approved these changes Sep 25, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 26, 2023

openjdk bot closed this Sep 26, 2023

openjdk bot removed the ready Pull request is ready to be integrated label Sep 26, 2023

openjdk bot removed the rfr Pull request is ready for review label Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rwestrel commented Sep 6, 2023 •

edited by openjdk bot

bridgekeeper bot commented Sep 6, 2023

openjdk bot commented Sep 6, 2023

mlbridge bot commented Sep 6, 2023 •

edited

chhagedorn left a comment

chhagedorn Sep 14, 2023

chhagedorn Sep 14, 2023

chhagedorn Sep 14, 2023

chhagedorn Sep 14, 2023

chhagedorn Sep 14, 2023

openjdk bot commented Sep 14, 2023 •

edited

TobiHartmann left a comment

chhagedorn left a comment

rwestrel commented Sep 26, 2023

openjdk bot commented Sep 26, 2023

openjdk bot commented Sep 26, 2023


		bool higher_equal_types(PhaseGVN* phase, const Node* other) const;

		int nb_extra_types() const {

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

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

Conversation

rwestrel commented Sep 6, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 6, 2023

openjdk bot commented Sep 6, 2023

mlbridge bot commented Sep 6, 2023 • edited

Webrevs

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn Sep 14, 2023

Choose a reason for hiding this comment

chhagedorn Sep 14, 2023

Choose a reason for hiding this comment

chhagedorn Sep 14, 2023

Choose a reason for hiding this comment

chhagedorn Sep 14, 2023

Choose a reason for hiding this comment

chhagedorn Sep 14, 2023

Choose a reason for hiding this comment

openjdk bot commented Sep 14, 2023 • edited

TobiHartmann left a comment

Choose a reason for hiding this comment

chhagedorn left a comment

Choose a reason for hiding this comment

rwestrel commented Sep 26, 2023

openjdk bot commented Sep 26, 2023

openjdk bot commented Sep 26, 2023

rwestrel commented Sep 6, 2023 •

edited by openjdk bot

mlbridge bot commented Sep 6, 2023 •

edited

openjdk bot commented Sep 14, 2023 •

edited