8317336: Assertion error thrown during 'this' escape analysis #16001

archiecobbs · 2023-10-01T19:48:46Z

The 'this' escape analyzer was assuming that lambas could always be treated like blocks, in the sense that we can assume a block can never end in a plain expression. This assumption is not true for lambda's of the form () -> foo which do exactly that. In the case that foo was this, an assertion failure would ensue.

This patch explicitly clears any leftover ExprRef's after evaluating a lambda body; this restores the asserted invariant when the lambda body is an expression.

This also fixes another variant of the same bug caused by certain enhanced for() loops. In this case, the leftover ExprRef was from evaluating the loop expression. This was just a side-effect of the analyzer failing to override TreeScanner to actually assign the loop expression to the loop variable (that will be fixed later in another patch).

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-8317336: Assertion error thrown during 'this' escape analysis (Bug - P4)

Reviewers

Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16001

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-10-01T19:50:09Z

👋 Welcome back acobbs! 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-10-01T19:51:50Z

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

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-10-01T19:55:57Z

Webrevs

The mistaken assumption was that a lambda body is always a JCBlock. It can also be a JCExpression, in which case it could result in an ExprRef 'this' reference.

vicente-romero-oracle

looks sensible

openjdk · 2023-10-11T18:59:06Z

@archiecobbs 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:

8317336: Assertion error thrown during 'this' escape analysis

Reviewed-by: vromero

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 77 new commits pushed to the master branch:

b92de54: 8317964: java/awt/Mouse/MouseModifiersUnitTest/MouseModifiersUnitTest_Standard.java fails on macosx-all after JDK-8317751
2a80160: 8314283: Support for NSS tests on aarch64 platforms
3f6d016: 8314896: additional clarifications to reversed() default methods' implementation requirements
2d46b29: 8317874: Add @sealedGraph to StringTemplate.Processor.Linkage
8f8c45b: 8315716: RISC-V: implement ChaCha20 intrinsic
8a9c4d5: 8317675: Serial: Move gc/shared/generation to serial folder
bcafec5: 8316958: Add test for unstructured locking
731fb4e: 8317797: G1: Remove unimplemented predict_will_fit
7976151: 8313438: [s390x] build broken after JDK-8301996
ca96fd3: 8317839: Exclude java/nio/channels/Channels/SocketChannelStreams.java on AIX
... and 67 more: https://git.openjdk.org/jdk/compare/f7deaf4bef21cb86fcc8a6c10454fbd6bbbba2d9...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vicente-romero-oracle) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

archiecobbs · 2023-10-11T20:19:29Z

/integrate

@vicente-romero-oracle, thanks for the reviews.

openjdk · 2023-10-11T20:21:36Z

@archiecobbs
Your change (at version 043e8fe) is now ready to be sponsored by a Committer.

vicente-romero-oracle · 2023-10-12T16:49:30Z

/sponsor

openjdk · 2023-10-12T16:51:54Z

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

16fd43c: 8316693: Simplify at-requires checkDockerSupport()
c1f698d: 8317956: Make jdk.internal.util.Architecture current architecture final
9355431: 8316426: Optimization for HexFormat.formatHex
32ccf01: 8317772: NMT: Make peak values available in release builds
4c79e7d: 8170817: G1: Returning MinTLABSize from unsafe_max_tlab_alloc causes TLAB flapping
7633a76: 8317998: Temporarily disable malformed control flow assert to reduce noise in testing
00ef9f9: 8316947: Write a test to check textArea triggers MouseEntered/MouseExited events properly
77dc891: 8317963: Serial: Remove unused GenerationIsInReservedClosure
d95b548: 8315850: Improve AbstractMap anonymous Iterator classes
424de29: 8317866: replace NET_SocketAvailable
... and 83 more: https://git.openjdk.org/jdk/compare/f7deaf4bef21cb86fcc8a6c10454fbd6bbbba2d9...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-12T16:52:02Z

@vicente-romero-oracle @archiecobbs Pushed as commit a8473b7.

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

archiecobbs · 2023-10-13T14:30:23Z

/backport jdk21u

openjdk · 2023-10-13T14:33:28Z

@archiecobbs the backport was successfully created on the branch archiecobbs-backport-a8473b70 in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit a8473b70 from the openjdk/jdk repository.

The commit being backported was authored by Archie Cobbs on 12 Oct 2023 and was reviewed by Vicente Romero.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u:

$ git fetch https://github.com/openjdk-bots/jdk21u.git archiecobbs-backport-a8473b70:archiecobbs-backport-a8473b70
$ git checkout archiecobbs-backport-a8473b70
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u.git archiecobbs-backport-a8473b70

Fix incorrect invariant assertion in this escape analysis.

8ad1247

openjdk bot added the rfr Pull request is ready for review label Oct 1, 2023

openjdk bot added the compiler compiler-dev@openjdk.org label Oct 1, 2023

archiecobbs added 6 commits October 3, 2023 13:03

Add fix and unit test for another variant of the assertion failure.

03cda07

Replace 8ad1247 with more correct fix.

457bcca

The mistaken assumption was that a lambda body is always a JCBlock. It can also be a JCExpression, in which case it could result in an ExprRef 'this' reference.

Merge branch 'master' into JDK-8317336

09e4283

Fix inadvertent variable renaming.

bd2ec94

Refactor patch to minimize it to just the required bugfix.

95fde46

Fix regression test class names.

043e8fe

vicente-romero-oracle approved these changes Oct 11, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 11, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 11, 2023

openjdk bot added the integrated Pull request has been integrated label Oct 12, 2023

openjdk bot closed this Oct 12, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8317336: Assertion error thrown during 'this' escape analysis #16001

8317336: Assertion error thrown during 'this' escape analysis #16001

archiecobbs commented Oct 1, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 1, 2023

openjdk bot commented Oct 1, 2023

mlbridge bot commented Oct 1, 2023 •

edited

Loading

vicente-romero-oracle left a comment

openjdk bot commented Oct 11, 2023

archiecobbs commented Oct 11, 2023

openjdk bot commented Oct 11, 2023

vicente-romero-oracle commented Oct 12, 2023

openjdk bot commented Oct 12, 2023

openjdk bot commented Oct 12, 2023

archiecobbs commented Oct 13, 2023

openjdk bot commented Oct 13, 2023

8317336: Assertion error thrown during 'this' escape analysis #16001

8317336: Assertion error thrown during 'this' escape analysis #16001

Conversation

archiecobbs commented Oct 1, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 1, 2023

openjdk bot commented Oct 1, 2023

mlbridge bot commented Oct 1, 2023 • edited Loading

Webrevs

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 11, 2023

archiecobbs commented Oct 11, 2023

openjdk bot commented Oct 11, 2023

vicente-romero-oracle commented Oct 12, 2023

openjdk bot commented Oct 12, 2023

openjdk bot commented Oct 12, 2023

archiecobbs commented Oct 13, 2023

openjdk bot commented Oct 13, 2023

archiecobbs commented Oct 1, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 1, 2023 •

edited

Loading