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

8275049: [ZGC] missing null check in ZNMethod::log_register #5892

Closed

Conversation

TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Oct 11, 2021

The VM crashes while trying to read (*p)->klass() in "ZNMethod::log_register" on PPC64. We need a null check. See JBS for details.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8275049: [ZGC] missing null check in ZNMethod::log_register

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5892/head:pull/5892
$ git checkout pull/5892

Update a local copy of the PR:
$ git checkout pull/5892
$ git pull https://git.openjdk.java.net/jdk pull/5892/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5892

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5892.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 11, 2021

👋 Welcome back mdoerr! 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 openjdk bot added the rfr Pull request is ready for review label Oct 11, 2021
@openjdk
Copy link

openjdk bot commented Oct 11, 2021

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

  • hotspot-gc

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.

@openjdk openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Oct 11, 2021
@mlbridge
Copy link

mlbridge bot commented Oct 11, 2021

Webrevs

Copy link
Contributor

@fisk fisk left a comment

The oops we read here are written in c1_Runtime, while holding the Patching_lock. However, the Patching_lock is not held when registering. In other words, someone could be writing an oop while we are reading it in this loop, due to C1 patching. If the loads re-order in here, we may crash the VM. Since we have plain loads, the compiler is free to re-order. I think the solution I would go with, is to use the CompiledICLocker instead where we patch the code in C1. That ends up taking the per-nmethod lock, that we can hold while logging this. That makes sure that accessing the oops implies mutual exclusion. Then we can remove the Patching_lock, since it is only ever used in that one place.

@TheRealMDoerr
Copy link
Contributor Author

TheRealMDoerr commented Oct 12, 2021

Hi Erik,
your description sounds like the plain reads are what are problematic. (Oop creation always needs to release the header initialization. Otherwise, it's a bug there.) I agree with that. I could use Atomic::load(p) or Atomic::load_acquire(p). What do you think about that?
Fundamental locking changes are out of scope, here. That could be done separately if desired. This issue is about the missing null check. Note that we may need to backport this fix, so we should better keep it simple.

@fisk
Copy link
Contributor

fisk commented Oct 12, 2021

I think we could just Atomic::load(p) for now to deal with the null check, and then see if we want to deal with the other race condition another day.

@@ -126,8 +126,10 @@ void ZNMethod::log_register(const nmethod* nm) {
oop* const begin = nm->oops_begin();
oop* const end = nm->oops_end();
for (oop* p = begin; p < end; p++) {
const char* external_name = (*p) == nullptr ? "null"
: (*p)->klass()->external_name();
Copy link
Contributor

@pliden pliden Oct 12, 2021

Choose a reason for hiding this comment

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

Apart from doing an Atomic::load() here, I'd also suggest we write "N/A" instead of "null", since the class isn't really null.

@TheRealMDoerr
Copy link
Contributor Author

TheRealMDoerr commented Oct 12, 2021

Thanks for your suggestions. They make sense.

fisk
fisk approved these changes Oct 12, 2021
Copy link
Contributor

@fisk fisk left a comment

Looks good.

@openjdk
Copy link

openjdk bot commented Oct 12, 2021

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

8275049: [ZGC] missing null check in ZNMethod::log_register

Reviewed-by: nradomski, eosterlund, pliden

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

  • 124f823: 8268764: Use Long.hashCode() instead of int-cast where applicable
  • 8657f77: 8271514: support JFR use of new ThreadsList::Iterator
  • b8bd259: 8271737: Only normalize the cached user.dir property once
  • 89999f7: 8275131: Exceptions after a touchpad gesture on macOS
  • 07b1f1c: 8274548: (fc) FileChannel gathering write fails with IOException "Invalid argument" on macOS 11.6
  • f623460: 8274911: testlibrary_tests/ir_framework/tests/TestIRMatching.java fails with "java.lang.RuntimeException: Should have thrown exception"
  • e393c5e: 8275074: Cleanup unused code in JFR LeakProfiler
  • e16b93a: 8274770: [PPC64] resolve_jobject needs a generic implementation to support load barriers
  • 1ab6414: 8275051: Shenandoah: Correct ordering of requested gc cause and gc request flag
  • b460d6d: 8275091: /src/jdk.management.jfr/share/classes/module-info.java has non-canonical order
  • ... and 17 more: https://git.openjdk.java.net/jdk/compare/b7af890574b3c13122fe7de987a8c9458c05f625...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 Oct 12, 2021
log_oops.print(" Oop[" SIZE_FORMAT "] " PTR_FORMAT " (%s)",
(p - begin), p2i(*p), (*p)->klass()->external_name());
(p - begin), p2i(*p), external_name);
Copy link
Contributor

@pliden pliden Oct 12, 2021

Choose a reason for hiding this comment

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

p2i(*p) should be p2i(o) now

@@ -126,8 +126,11 @@ void ZNMethod::log_register(const nmethod* nm) {
oop* const begin = nm->oops_begin();
oop* const end = nm->oops_end();
for (oop* p = begin; p < end; p++) {
oop o = Atomic::load(p); // C1 PatchingStub may replace it concurrently.
const char* external_name = o == nullptr ? "N/A"
: o->klass()->external_name();
Copy link
Contributor

@pliden pliden Oct 12, 2021

Choose a reason for hiding this comment

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

Can we please make this a single line, and add parenthesis around (o == nullptr)

@@ -126,8 +126,11 @@ void ZNMethod::log_register(const nmethod* nm) {
oop* const begin = nm->oops_begin();
oop* const end = nm->oops_end();
for (oop* p = begin; p < end; p++) {
oop o = Atomic::load(p); // C1 PatchingStub may replace it concurrently.
Copy link
Contributor

@pliden pliden Oct 12, 2021

Choose a reason for hiding this comment

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

Can we please make this const oop o = ...

@TheRealMDoerr
Copy link
Contributor Author

TheRealMDoerr commented Oct 12, 2021

Good catch. Thanks!

pliden
pliden approved these changes Oct 12, 2021
Copy link
Contributor

@pliden pliden left a comment

Thanks for fixing. Looks good!

fisk
fisk approved these changes Oct 12, 2021
@TheRealMDoerr
Copy link
Contributor Author

TheRealMDoerr commented Oct 13, 2021

Thanks for the reviews!
/integrate

@openjdk
Copy link

openjdk bot commented Oct 13, 2021

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

  • ab34cce: 8275186: Suppress warnings on non-serializable array component types in xml
  • b1b8350: 8275171: ProblemList compiler/codegen/aes/TestAESMain.java on linux-x64 and windows-x64 in -Xcomp mode
  • 03c2b73: 8275128: Build hsdis using normal build system
  • 124f823: 8268764: Use Long.hashCode() instead of int-cast where applicable
  • 8657f77: 8271514: support JFR use of new ThreadsList::Iterator
  • b8bd259: 8271737: Only normalize the cached user.dir property once
  • 89999f7: 8275131: Exceptions after a touchpad gesture on macOS
  • 07b1f1c: 8274548: (fc) FileChannel gathering write fails with IOException "Invalid argument" on macOS 11.6
  • f623460: 8274911: testlibrary_tests/ir_framework/tests/TestIRMatching.java fails with "java.lang.RuntimeException: Should have thrown exception"
  • e393c5e: 8275074: Cleanup unused code in JFR LeakProfiler
  • ... and 20 more: https://git.openjdk.java.net/jdk/compare/b7af890574b3c13122fe7de987a8c9458c05f625...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 13, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 13, 2021
@openjdk
Copy link

openjdk bot commented Oct 13, 2021

@TheRealMDoerr Pushed as commit cf82867.

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

@TheRealMDoerr TheRealMDoerr deleted the 8275049_ZGC_log_register branch Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated
4 participants