8257709: C1: Double assignment in InstructionPrinter::print_stack #1519

sergey-platonov · 2020-11-30T14:26:23Z

There is currently a minor bug (just a typo, actually) in the hotspot, leading
to undefined behavior.

Progress

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

Issue

JDK-8257709: C1: Double assignment in InstructionPrinter::print_stack

Reviewers

Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to ca66b57
Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to ca66b57

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1519/head:pull/1519
$ git checkout pull/1519

bridgekeeper · 2020-11-30T14:26:48Z

Hi @sergey-platonov, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user sergey-platonov" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2020-11-30T14:27:49Z

@sergey-platonov 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.

sergey-platonov · 2020-12-01T16:27:28Z

/signed

bridgekeeper · 2020-12-01T16:28:02Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

shipilev · 2020-12-03T17:59:33Z

Submitted: https://bugs.openjdk.java.net/browse/JDK-8257709 -- please rename this PR to "8257709: C1: Double assignment in InstructionPrinter::print_stack" to get it hooked properly. Also, can you please go to https://github.com/sergey-platonov/jdk/actions and check that testing workflow is enabled? If not, can you trigger it manually on your branch?

shipilev · 2020-12-03T18:00:29Z

src/hotspot/share/c1/c1_InstructionPrinter.cpp

I would prefer to keep i++, as in old code, unless it fixes something I don't quite see.

Done. Can you, please, create an issue, so I can mention it here?

I did already, see #1519 (comment).

Oh, sorry! I guess, haven't completely got back from the vacation :-)

Pipeline is on the way.

bridgekeeper · 2020-12-31T18:01:03Z

@sergey-platonov This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

There is currently a minor bug (just a typo, actually) in the hotspot, leading to undefined behavior. Issue: JDK-8257709: C1: Double assignment in InstructionPrinter::print_stack

mlbridge · 2021-01-11T11:23:06Z

Webrevs

01: Full - Incremental (c75652c)
00: Full (ca66b57)

shipilev

Looks good.

openjdk · 2021-01-11T11:24:54Z

@sergey-platonov 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:

8257709: C1: Double assignment in InstructionPrinter::print_stack

Reviewed-by: shade, 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 30 new commits pushed to the master branch:

400dc76: 8252015: [macos11] java.awt.TrayIcon requires updates for template images
ac2dee5: 8259539: JDK-8255711 broke trap messages
4697cfa: 8259576: Misplaced curly brace in Matcher::find_shared_post_visit
a6ab9e4: 8258576: Try to get zerobased CCS if heap is above 32 and CDS is disabled
a3561ae: 8258243: C2: assert failed ("Bad derived pointer") with -XX:+VerifyRegisterAllocator
4663704: 8259583: Remove unused decode_env::_codeBuffer
9d4e84f: 8259565: Zero: compiler/runtime/criticalnatives fails because CriticalJNINatives is not supported
98ccfbf: 8255710: Opensource unit/regression tests for CMM
61c5b95: 7194219: java/awt/Component/UpdatingBootTime/UpdatingBootTime.html fails on Linux
77f6290: 8258254: Move PtrQueue flush to PtrQueueSet subclasses
... and 20 more: https://git.openjdk.java.net/jdk/compare/33fbc10cb8c869984159230b55ab9f3c9ca039ec...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 (@shipilev, @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).

shipilev · 2021-01-11T11:27:42Z

...ah, and maybe merge from master to get all the recent fixes. Current tests might fail on known issues. Not that it is necessary for a simple issue like this, but it helps to learn the contribution ropes in this project.

chhagedorn

Looks good.

sergey-platonov · 2021-01-11T14:08:40Z

...ah, and maybe merge from master to get all the recent fixes. Current tests might fail on known issues. Not that it is necessary for a simple issue like this, but it helps to learn the contribution ropes in this project.

Done! Will commits be squashed?

shipilev · 2021-01-11T14:10:50Z

...ah, and maybe merge from master to get all the recent fixes. Current tests might fail on known issues. Not that it is necessary for a simple issue like this, but it helps to learn the contribution ropes in this project.

Done! Will commits be squashed?

Yes, bots would squash the commits during integration. You don't need to rebase or do anything else Git-wise. Once testing is clean, you can say /integrate, as bots suggested.

sergey-platonov · 2021-01-11T14:28:17Z

...ah, and maybe merge from master to get all the recent fixes. Current tests might fail on known issues. Not that it is necessary for a simple issue like this, but it helps to learn the contribution ropes in this project.

Done! Will commits be squashed?

Yes, bots would squash the commits during integration. You don't need to rebase or do anything else Git-wise. Once testing is clean, you can say /integrate, as bots suggested.

Great! Thank you for your help, Aleksey!

shipilev · 2021-01-12T11:01:30Z

If you go an look at "Checks" tab, you will see it says that everything is green. You can say /integrate now.

sergey-platonov · 2021-01-12T11:56:01Z

/integrate

openjdk · 2021-01-12T11:56:36Z

@sergey-platonov
Your change (at version c75652c) is now ready to be sponsored by a Committer.

shipilev · 2021-01-12T11:57:07Z

/sponsor

openjdk · 2021-01-12T11:57:49Z

@shipilev @sergey-platonov Since your change was applied there have been 30 commits pushed to the master branch:

400dc76: 8252015: [macos11] java.awt.TrayIcon requires updates for template images
ac2dee5: 8259539: JDK-8255711 broke trap messages
4697cfa: 8259576: Misplaced curly brace in Matcher::find_shared_post_visit
a6ab9e4: 8258576: Try to get zerobased CCS if heap is above 32 and CDS is disabled
a3561ae: 8258243: C2: assert failed ("Bad derived pointer") with -XX:+VerifyRegisterAllocator
4663704: 8259583: Remove unused decode_env::_codeBuffer
9d4e84f: 8259565: Zero: compiler/runtime/criticalnatives fails because CriticalJNINatives is not supported
98ccfbf: 8255710: Opensource unit/regression tests for CMM
61c5b95: 7194219: java/awt/Component/UpdatingBootTime/UpdatingBootTime.html fails on Linux
77f6290: 8258254: Move PtrQueue flush to PtrQueueSet subclasses
... and 20 more: https://git.openjdk.java.net/jdk/compare/33fbc10cb8c869984159230b55ab9f3c9ca039ec...master

Your commit was automatically rebased without conflicts.

Pushed as commit 563b268.

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

bridgekeeper bot added the oca Needs verification of OCA signatory status label Nov 30, 2020

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 30, 2020

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Dec 1, 2020

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Dec 1, 2020

shipilev suggested changes Dec 3, 2020

View reviewed changes

sergey-platonov force-pushed the fix/hotspot-possible-ub branch from 796f87f to 12a57f7 Compare January 11, 2021 08:32

sergey-platonov changed the title ~~[PATCH] Possible undefined behavior: double modification~~ 8257709: C1: Double assignment in InstructionPrinter::print_stack Jan 11, 2021

openjdk bot added the rfr Pull request is ready for review label Jan 11, 2021

8257709: C1: Double assignment in InstructionPrinter::print_stack

ca66b57

There is currently a minor bug (just a typo, actually) in the hotspot, leading to undefined behavior. Issue: JDK-8257709: C1: Double assignment in InstructionPrinter::print_stack

sergey-platonov force-pushed the fix/hotspot-possible-ub branch from 12a57f7 to ca66b57 Compare January 11, 2021 11:17

shipilev approved these changes Jan 11, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 11, 2021

chhagedorn approved these changes Jan 11, 2021

View reviewed changes

Merge branch 'master' into fix/hotspot-possible-ub

c75652c

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 12, 2021

openjdk bot closed this Jan 12, 2021

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 12, 2021

8257709: C1: Double assignment in InstructionPrinter::print_stack #1519

8257709: C1: Double assignment in InstructionPrinter::print_stack #1519

Uh oh!

Conversation

sergey-platonov commented Nov 30, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Nov 30, 2020

Uh oh!

openjdk bot commented Nov 30, 2020

Uh oh!

sergey-platonov commented Dec 1, 2020

Uh oh!

bridgekeeper bot commented Dec 1, 2020

Uh oh!

shipilev commented Dec 3, 2020

Uh oh!

shipilev Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

sergey-platonov Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

shipilev Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

sergey-platonov Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

sergey-platonov Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Dec 31, 2020

Uh oh!

mlbridge bot commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shipilev commented Jan 11, 2021

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

sergey-platonov commented Jan 11, 2021

Uh oh!

shipilev commented Jan 11, 2021

Uh oh!

sergey-platonov commented Jan 11, 2021

Uh oh!

shipilev commented Jan 12, 2021

Uh oh!

sergey-platonov commented Jan 12, 2021

Uh oh!

openjdk bot commented Jan 12, 2021

Uh oh!

shipilev commented Jan 12, 2021

Uh oh!

openjdk bot commented Jan 12, 2021

Uh oh!

Uh oh!

sergey-platonov commented Nov 30, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 11, 2021 •

edited

Loading

openjdk bot commented Jan 11, 2021 •

edited

Loading