8335536: Fix assertion failure in IdealGraphPrinter when append is true #19986

MaxXSoft · 2024-07-02T09:31:32Z

When IdealGraphPrinter is created with append set to true, it does not write the head element to the file in its constructor. But in the destructor, it always writes the tail element to the file no matter append is true or not, which causes an assertion failure in xmlStream::pop_tag.

This issue can be reproduced by calling igv_append directly, or by calling Compile::igv_print_method_to_file with append set to true.

This patch fixes the issue.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8335536: Fix assertion failure in IdealGraphPrinter when append is true (Bug - P5)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Christian Hagedorn (@chhagedorn - Reviewer)
Tobias Holenstein (@tobiasholenstein - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19986

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-02T09:32:36Z

👋 Welcome back qxing! 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 · 2024-07-02T09:32:49Z

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

8335536: Fix assertion failure in IdealGraphPrinter when append is true

Reviewed-by: thartmann, chagedorn, tholenstein

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

6c84e9c: 8335544: Serial: Remove unused _should_allocate_from_space
4d2f737: 8335357: Delete HotSpotJDKReflection.oopSizeOffset
77a7078: 8335593: Fix -Wzero-as-null-pointer-constant warning in Type_Array ctor
f3f90dc: 8335592: Fix -Wzero-as-null-pointer-constant warnings in RootNode ctor
7bc8f9c: 8335589: Fix -Wzero-as-null-pointer-constant warnings in IdealLoopTree ctor
0db9bc5: 8335290: Rename ClassFile::transform to ClassFile::transformClass
d51141e: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes
fac74b1: 8334229: Optimize InterpreterOopMap layout
f9b4ea1: 8334220: Optimize Klass layout after JDK-8180450
f7af450: 8335110: Fix instruction name and API spec inconsistencies in CodeBuilder
... and 10 more: https://git.openjdk.org/jdk/compare/4060b35b1d00fccbec4b20353063f77c43ecc686...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 (@TobiHartmann, @chhagedorn, @tobiasholenstein) 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 · 2024-07-02T09:33:12Z

@MaxXSoft 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 · 2024-07-02T09:38:25Z

Webrevs

01: Full - Incremental (269bb1c2)
00: Full (42de6316)

TobiHartmann · 2024-07-03T05:52:55Z

src/hotspot/share/opto/idealGraphPrinter.cpp

+    } else {
+      end_method();
+      begin_method();
+    }


Could you explain this change and maybe add a comment?

Reason for adding if (_append): in append mode, the constructor of IdealGraphPrinter will not call begin_method, so we should not call end_method here to prevent the assertion failure, just update _current_method.

Reason for removing _current_method = nullptr;: _current_method is maintained and cleared in end_method, we do not need to handle it manually here.

Added more comments in the latest commit.

TobiHartmann

That looks good to me but @chhagedorn and/or @tobiasholenstein should have a look as well.

/reviewers 2

openjdk · 2024-07-03T07:07:34Z

@TobiHartmann
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

chhagedorn

Looks good to me, too.

tobiasholenstein · 2024-07-03T07:58:46Z

Looks good!

MaxXSoft · 2024-07-03T08:49:21Z

@TobiHartmann @chhagedorn @tobiasholenstein Thanks for the review!

/integrate

openjdk · 2024-07-03T08:50:50Z

@MaxXSoft
Your change (at version 269bb1c) is now ready to be sponsored by a Committer.

TobiHartmann · 2024-07-03T12:10:29Z

/sponsor

openjdk · 2024-07-03T12:12:01Z

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

350f9c1: 8322812: Manpage for jcmd is missing JFR.view command
c06b75f: 8335591: Fix -Wzero-as-null-pointer-constant warnings in ConcurrentHashTable
6c84e9c: 8335544: Serial: Remove unused _should_allocate_from_space
4d2f737: 8335357: Delete HotSpotJDKReflection.oopSizeOffset
77a7078: 8335593: Fix -Wzero-as-null-pointer-constant warning in Type_Array ctor
f3f90dc: 8335592: Fix -Wzero-as-null-pointer-constant warnings in RootNode ctor
7bc8f9c: 8335589: Fix -Wzero-as-null-pointer-constant warnings in IdealLoopTree ctor
0db9bc5: 8335290: Rename ClassFile::transform to ClassFile::transformClass
d51141e: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes
fac74b1: 8334229: Optimize InterpreterOopMap layout
... and 12 more: https://git.openjdk.org/jdk/compare/4060b35b1d00fccbec4b20353063f77c43ecc686...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-03T12:12:08Z

@TobiHartmann @MaxXSoft Pushed as commit 6db4c6a.

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

Fix assertion fail in IdealGraphPrinter when append is true.

42de631

openjdk bot added the rfr Pull request is ready for review label Jul 2, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jul 2, 2024

MaxXSoft changed the title ~~8335536: Fix assertion failure in IdealGraphPrinter when append is true.~~ 8335536: Fix assertion failure in IdealGraphPrinter when append is true Jul 2, 2024

TobiHartmann reviewed Jul 3, 2024

View reviewed changes

Add comments for IdealGraphPrinter::update_compiled_method.

269bb1c

TobiHartmann approved these changes Jul 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 3, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Jul 3, 2024

chhagedorn approved these changes Jul 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 3, 2024

tobiasholenstein approved these changes Jul 3, 2024

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 3, 2024

openjdk bot added the integrated Pull request has been integrated label Jul 3, 2024

openjdk bot closed this Jul 3, 2024

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 Jul 3, 2024

MaxXSoft deleted the fix-graph-printer branch July 4, 2024 00:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8335536: Fix assertion failure in IdealGraphPrinter when append is true #19986

8335536: Fix assertion failure in IdealGraphPrinter when append is true #19986

MaxXSoft commented Jul 2, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 2, 2024

openjdk bot commented Jul 2, 2024 •

edited

Loading

openjdk bot commented Jul 2, 2024

mlbridge bot commented Jul 2, 2024 •

edited

Loading

TobiHartmann Jul 3, 2024

MaxXSoft Jul 3, 2024

TobiHartmann Jul 3, 2024

TobiHartmann left a comment

openjdk bot commented Jul 3, 2024

chhagedorn left a comment

tobiasholenstein commented Jul 3, 2024

MaxXSoft commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

TobiHartmann commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

8335536: Fix assertion failure in IdealGraphPrinter when append is true #19986

8335536: Fix assertion failure in IdealGraphPrinter when append is true #19986

Conversation

MaxXSoft commented Jul 2, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 2, 2024

openjdk bot commented Jul 2, 2024 • edited Loading

openjdk bot commented Jul 2, 2024

mlbridge bot commented Jul 2, 2024 • edited Loading

Webrevs

TobiHartmann Jul 3, 2024

Choose a reason for hiding this comment

MaxXSoft Jul 3, 2024

Choose a reason for hiding this comment

TobiHartmann Jul 3, 2024

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 3, 2024

chhagedorn left a comment

Choose a reason for hiding this comment

tobiasholenstein commented Jul 3, 2024

MaxXSoft commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

TobiHartmann commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

MaxXSoft commented Jul 2, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 2, 2024 •

edited

Loading

mlbridge bot commented Jul 2, 2024 •

edited

Loading