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

8263775: C2: igv_print() crash unexpectedly when called from debugger #3065

Closed
wants to merge 1 commit into from

Conversation

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Mar 18, 2021

When investigating another c2 crash, I found igv_print crashed unexpectedly when called from the debugger. The problem is no matter whether we have created an igv printer in Compile::igv_print_method_to_file:

void Compile::igv_print_method_to_file(const char* phase_name, bool append) {
const char* file_name = "custom_debug.xml";
if (_debug_file_printer == NULL) {
_debug_file_printer = new IdealGraphPrinter(C, file_name, append);
} else {
_debug_file_printer->update_compiled_method(C->method());
}
tty->print_cr("Method %s to %s", append ? "appended" : "printed", file_name);
_debug_file_printer->print_method(phase_name, 0);

Compile:: should_print(called by print_method()) is not aware of _debug_file_printer, it would create a new one, and entering into unexpected network_stream initialization(See hs_err on JBS):

bool should_print(int level = 1) {
#ifndef PRODUCT
if (PrintIdealGraphLevel < 0) { // disabled by the user
return false;
}
bool need = directive()->IGVPrintLevelOption >= level;
if (need && !_printer) {
_printer = IdealGraphPrinter::printer();
assert(_printer != NULL, "_printer is NULL when we need it!");
_printer->set_compile(this);

For debugging purposes, we can skip checking should_print(), printing graph directly. (But even if applying this patch, igv_print can not work well since it seems produces an ill-formed ideal graph whose tags are not enclosed, thus can not be recognized by idealgraphvisualizer, this looks like a related by different fix, so I might create another PR to address this?)

Thanks!
Yang


Progress

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

Issue

  • JDK-8263775: C2: igv_print() crash unexpectedly when called from debugger

Reviewers

Download

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

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 18, 2021

👋 Welcome back yyang! 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 label Mar 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

@kelthuzadx 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
Copy link

@mlbridge mlbridge bot commented Mar 18, 2021

Webrevs

@kelthuzadx kelthuzadx force-pushed the fix_igv_print_crash branch from 09cd043 to 056c655 Mar 18, 2021
Copy link
Member

@chhagedorn chhagedorn left a comment

Looks good to me. It makes sense to skip should_print. I think I only tested it back there with an open IGV. But the print to file version should also work with a closed IGV. For the network version, you have to have an open IGV anyways but it's better to directly have a call to print, too.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

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

8263775: C2: igv_print() crash unexpectedly when called from debugger

Reviewed-by: chagedorn

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

  • ff52f29: 8260716: Assert in MacroAssembler::clear_mem with -XX:-IdealizeClearArrayNode
  • 72b82fd: 8263725: JFR oldobject tests are not run when GCs are specified explicitly
  • 444a80b: 8263455: NMT: assert on registering a region which completely engulfs an existing region
  • 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
  • ... and 19 more: https://git.openjdk.java.net/jdk/compare/dd6c91141aa0f294bc9755e081ee2068e1f5f0f3...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 (@chhagedorn) 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).

@openjdk openjdk bot added the ready label Mar 18, 2021
@chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Mar 18, 2021

(But even if applying this patch, igv_print can not work well since it seems produces an ill-formed ideal graph whose tags are not enclosed, thus can not be recognized by idealgraphvisualizer, this looks like a related by different fix, so I might create another PR to address this?)

When does this happen? Can you reliably reproduce this? I also noticed in the past that sometimes when you want to print the graph during some graph modification then IGV could have problems showing the graph. Yes, please open a new bug for that.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Mar 18, 2021

Hi @chhagedorn , thanks for your review! It happens when VM crashes before VM exits normally, the ideal graph xml will be ill-formed, this is fairly a common case when debugging another crash/bug.

/integrate
(This PR looks trivial, so one reviewer might enough)

@openjdk openjdk bot added the sponsor label Mar 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

@kelthuzadx
Your change (at version 056c655) is now ready to be sponsored by a Committer.

@chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Mar 18, 2021

Yes, the fix is trivial. I'll sponsor.

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

@chhagedorn @kelthuzadx Since your change was applied there have been 31 commits pushed to the master branch:

  • 63eae8f: 8260605: Various java.lang.invoke cleanups
  • 9cd21b6: 8263590: Rawtypes warnings should be produced for pattern matching in instanceof
  • ff52f29: 8260716: Assert in MacroAssembler::clear_mem with -XX:-IdealizeClearArrayNode
  • 72b82fd: 8263725: JFR oldobject tests are not run when GCs are specified explicitly
  • 444a80b: 8263455: NMT: assert on registering a region which completely engulfs an existing region
  • 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
  • ... and 21 more: https://git.openjdk.java.net/jdk/compare/dd6c91141aa0f294bc9755e081ee2068e1f5f0f3...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3f31a6b.

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

@kelthuzadx kelthuzadx deleted the fix_igv_print_crash branch Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants