Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8323950: Null CLD while loading shared lambda proxy class with javaagent active #17602

Conversation

calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented Jan 27, 2024

A simple fix for an assert failure when an archived interface of an archived lambda proxy class is being transformed using
a java agent during runtime. Please refer to the bug report for details.

Testing: passed tiers 1 - 4 (including the new test).


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-8323950: Null CLD while loading shared lambda proxy class with javaagent active (Bug - P2)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17602

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

Using diff file

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

Webrev

Link to Webrev Comment

@calvinccheung
Copy link
Member Author

/label add hotspot-runtime

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 27, 2024

👋 Welcome back ccheung! 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.

@calvinccheung calvinccheung marked this pull request as ready for review January 27, 2024 00:26
@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jan 27, 2024
@openjdk
Copy link

openjdk bot commented Jan 27, 2024

@calvinccheung
The hotspot-runtime label was successfully added.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 27, 2024
@mlbridge
Copy link

mlbridge bot commented Jan 27, 2024

Webrevs

Copy link
Contributor

@matias9927 matias9927 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@fisk
Copy link
Contributor

fisk commented Jan 30, 2024

These nulled out CLD fields in class loaders are troubling to me. The GCs are not built to cope with them. The code for marking through the heap assumes that surely a ClassLoader has a corresponding CLD, and crashes if it does not.

While we can try to chase after every closure where this is a problem and add a null check, I have to wonder if it is the right solution.

I wonder, if the CLD could be archived as well, or something like that.

@openjdk
Copy link

openjdk bot commented Jan 30, 2024

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

8323950: Null CLD while loading shared lambda proxy class with javaagent active

Reviewed-by: matsaave, iklam

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

  • a1d65eb: 8324125: Improve class initialization barrier in TemplateTable::_new for RISC-V
  • b6d364a: 8324865: windows-x64-slowdebug still does not build after JDK-8324840
  • 64c3642: 8242564: javadoc crashes:: class cast exception com.sun.tools.javac.code.Symtab$6
  • e999dfc: 8323503: x86: Shorter movptr(reg, imm) for 32-bit unsigned immediates
  • 84deeb6: 8324667: fold Parse::seems_stable_comparison()
  • fb07bbe: 8324717: Remove HotSpotJVMCICompilerFactory
  • d1e6763: 8324733: [macos14] Problem list tests which fail due to macOS bug described in JDK-8322653
  • c1281e6: 8324678: Replace NULL with nullptr in HotSpot gtests
  • a6bdee4: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files
  • 951b5f8: 8324723: GHA: Upgrade some actions to avoid deprecated Node 16
  • ... and 22 more: https://git.openjdk.org/jdk/compare/62b3293df0442b06cd00488774db7b608baca774...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 30, 2024
@calvinccheung
Copy link
Member Author

Thanks for taking a look.

These nulled out CLD fields in class loaders are troubling to me. The GCs are not built to cope with them. The code for marking through the heap assumes that surely a ClassLoader has a corresponding CLD, and crashes if it does not.

While we can try to chase after every closure where this is a problem and add a null check, I have to wonder if it is the right solution.

The CLD for a shared class is usually setup during SystemDictionary::load_shared_class. In this case, since a super type of a shared class is being transformed during runtime, the SystemDictionary::load_shared_class returns null early due to the fail check in check_shared_class_super_types. So I think the fix is okay for now; it is just a small adjustment to the original fix for JDK-8296433.

I wonder, if the CLD could be archived as well, or something like that.

We can file an RFE later if it deems beneficial to do so.

@calvinccheung
Copy link
Member Author

Thanks @matias9927, @iklam for the review.

/integrate

@openjdk
Copy link

openjdk bot commented Jan 30, 2024

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

  • f57c722: 8324880: Rename get_stack_trace.h
  • f0024f5: 8324734: Relax too-strict assert(VM_Version::supports_evex()) in Assembler::locate_operand()
  • fd8adf3: 8324856: Serial: Move Generation::is_in to DefNewGeneration
  • a1d65eb: 8324125: Improve class initialization barrier in TemplateTable::_new for RISC-V
  • b6d364a: 8324865: windows-x64-slowdebug still does not build after JDK-8324840
  • 64c3642: 8242564: javadoc crashes:: class cast exception com.sun.tools.javac.code.Symtab$6
  • e999dfc: 8323503: x86: Shorter movptr(reg, imm) for 32-bit unsigned immediates
  • 84deeb6: 8324667: fold Parse::seems_stable_comparison()
  • fb07bbe: 8324717: Remove HotSpotJVMCICompilerFactory
  • d1e6763: 8324733: [macos14] Problem list tests which fail due to macOS bug described in JDK-8322653
  • ... and 25 more: https://git.openjdk.org/jdk/compare/62b3293df0442b06cd00488774db7b608baca774...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jan 30, 2024
@openjdk openjdk bot closed this Jan 30, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 30, 2024
@openjdk
Copy link

openjdk bot commented Jan 30, 2024

@calvinccheung Pushed as commit d51aaf6.

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

@calvinccheung calvinccheung deleted the 8323950-null-CLD-loading-lambda-proxy branch January 30, 2024 21:19
@calvinccheung
Copy link
Member Author

/backport jdk22

@openjdk
Copy link

openjdk bot commented Feb 1, 2024

@calvinccheung the backport was successfully created on the branch backport-calvinccheung-d51aaf63 in my personal fork of openjdk/jdk22. To create a pull request with this backport targeting openjdk/jdk22: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 d51aaf63 from the openjdk/jdk repository.

The commit being backported was authored by Calvin Cheung on 30 Jan 2024 and was reviewed by Matias Saavedra Silva and Ioi Lam.

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/jdk22:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants