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

JDK-8263455: NMT: assert on registering a region which completely engulfs an existing region #2942

Conversation

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Mar 11, 2021

tl;dr:
fixes an error in an NMT function which decides whether memory regions overlap. Shows up as an assert in gtests but the error is old and may cause wrong NMT results.


I am testing a prototype for JDK-8256844 which makes NMT late initializable. One of the many benefits that will bring is that we can now run gtests with NMT enabled.

Which exercises NMT in new ways: we promptly crash in the metaspace tests, which do a lot of arbitrary, random, but entirely valid range commits as part of the VirtualSpaceNode stress tests:

[ RUN ] metaspace.virtual_space_node_test_5_vm
# To suppress the following error report, specify this argument
# after -XX: or in .hotspotrc: SuppressErrorAt=/virtualMemoryTracker.hpp:243
assert failed: assert(rgn.base() >= end()) failed: Sanity#
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (/shared/projects/openjdk/jdk-jdk/source/src/hotspot/share/services/virtualMemoryTracker.hpp:243), pid=183572, tid=183572
# assert(rgn.base() >= end()) failed: Sanity
#

Stack:

(gdb) bt
#0 0x00007ffff5b36cca in VirtualMemoryRegion::compare (this=0x555555b02278, rgn=...) at /shared/projects/openjdk/jdk-jdk/source/src/hotspot/share/services/virtualMemoryTracker.hpp:243
#1 0x00007ffff6cf6823 in compare_committed_region (r1=..., r2=...) at /shared/projects/openjdk/jdk-...
(gdb) p *this
$1 = {_base_address = 0x7fffb24c0000 "", _size = 1179648}
(gdb) p rgn
$2 = (const VirtualMemoryRegion &) @0x555555b02318: {_base_address = 0x7fffb2460000 "", _size = 2424832}

As we can see, the new committed to-be-registered region [0x7fffb2460000...7FFFB26B0000) completely engulfs an existing region [0x7fffb24c0000...0x7FFFB25E0000).

This triggers an assert in VirtualMemoryRegion:

  inline int compare(const VirtualMemoryRegion& rgn) const {
    if (overlap_region(rgn.base(), rgn.size())) {
      return 0;
    } else if (base() >= rgn.end()) {
      return 1;
    } else {
      assert(rgn.base() >= end(), "Sanity"); <<<
      return -1;
    }
  }

which calls

  inline bool overlap_region(address addr, size_t sz) const {
    assert(sz > 0, "Invalid size");
    assert(size() > 0, "Invalid size");
    return contain_address(addr) ||
           contain_address(addr + sz - 1);
  }

but VirtualMemoryRegion::overlap_region does not handle the engulfing case correctly.


Fix: fixed the overlap function to handle the engulfing case.

Tests: GAs; manually executed runtime/NMT; manually tested (with my prototype VM) that the gtest works after applying this fix.


Progress

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

Issue

  • JDK-8263455: NMT: assert on registering a region which completely engulfs an existing region

Reviewers

Download

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

To update a local copy of the PR:
$ git checkout pull/2942
$ git pull https://git.openjdk.java.net/jdk pull/2942/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 11, 2021

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 11, 2021

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

  • hotspot-runtime

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.

Loading

@tstuefe tstuefe marked this pull request as ready for review Mar 12, 2021
@openjdk openjdk bot added the rfr label Mar 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 12, 2021

Webrevs

Loading

Copy link
Contributor

@zhengyu123 zhengyu123 left a comment

Looks good.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

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

8263455: NMT: assert on registering a region which completely engulfs an existing region

Reviewed-by: zgu, coleenp

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

  • 83a49ef: 8263753: two new tests from JDK-8261671 fail with "Error. can not find ClassFileInstaller in test directory or libraries"
  • 24afa36: 8263726: divideToIntegralValue typo on BigDecimal documentation
  • cdf78e4: 8262298: G1BarrierSetC2::step_over_gc_barrier fails with assert "bad barrier shape"
  • 7674da4: 8262398: Shenandoah: Disable nmethod barrier and stack watermark when running with passive mode
  • 4f4ca0e: 8261671: X86 I2L conversion can be skipped for certain masked positive values
  • 5d87a21: 8263361: Incorrect arraycopy stub selected by C2 for SATB collectors
  • e152cc0: 8263677: Improve Character.isLowerCase/isUpperCase lookups
  • b63b5d4: 8263732: ProblemList serviceability/sa/ClhsdbSymbol.java on ZGC
  • 000012a: 8148937: (str) Adapt StringJoiner for Compact Strings
  • a707fcb: 8263723: [BACKOUT] MoveAndUpdateClosure::do_addr calls function with side-effects in an assert
  • ... and 88 more: https://git.openjdk.java.net/jdk/compare/273f8bdf5fe14e65e40d3d908f58dd682bb472ed...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.

Loading

@openjdk openjdk bot added the ready label Mar 15, 2021
@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Mar 15, 2021

Looks good.

Thank you Zhengyu!

Loading

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Mar 17, 2021

May I have a second review please? This is a very trivial patch.

Loading

Copy link
Contributor

@coleenp coleenp left a comment

Looks good. I had to work it out...

Loading

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Mar 18, 2021

Looks good. I had to work it out...

Thanks Coleen! Its actually an old well known recipe. I did not invent it :)

Loading

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Mar 18, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 18, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

@tstuefe Since your change was applied there have been 105 commits pushed to the master branch:

  • 2b93ae0: 8261480: MetaspaceShared::preload_and_dump should check exceptions
  • 81ba578: 8263676: AArch64: one potential bug in C1 LIRGenerator::generate_address()
  • 9225a23: 8263108: Class initialization deadlock in java.lang.constant
  • 5d5813a: 8263757: Remove serviceability/sa/ClhsdClasses.java from ZGC problem list
  • 50ff0d4: 8263756: Fix ZGC ProblemList entry for serviceability/sa/ClhsdbSymbol.java
  • 99b39aa: 8262807: Note assumptions of core reflection modeling and parameter handling
  • 26234b5: 8254979: Class.getSimpleName() returns non-empty for lambda and method
  • 83a49ef: 8263753: two new tests from JDK-8261671 fail with "Error. can not find ClassFileInstaller in test directory or libraries"
  • 24afa36: 8263726: divideToIntegralValue typo on BigDecimal documentation
  • cdf78e4: 8262298: G1BarrierSetC2::step_over_gc_barrier fails with assert "bad barrier shape"
  • ... and 95 more: https://git.openjdk.java.net/jdk/compare/273f8bdf5fe14e65e40d3d908f58dd682bb472ed...master

Your commit was automatically rebased without conflicts.

Pushed as commit 444a80b.

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

Loading

@tstuefe tstuefe deleted the JDK-8233455-NMT-assert-on-engulfing-regions branch Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants