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

8285436: riscv: Fix broken MacroAssembler::debug64 #8357

Closed
wants to merge 1 commit into from

Conversation

zhengxiaolinX
Copy link
Contributor

@zhengxiaolinX zhengxiaolinX commented Apr 22, 2022

MacroAssembler::stop()[1] and StubGenerator::generate_verify_oop()[2] would first push all regs (calling MacroAssembler::pusha()[3]) onto the stack and then call MacroAssembler::debug64()[4] to print the pushed regs. But MacroAssembler::pusha()[3] won't push x0~x4 so the result of MacroAssembler::debug64() is broken.

Tested by manually adding a __ verify_oop(x1) and option -XX:+VerifyOops -XX:+ShowMessageBoxOnError to deliberately crash the VM to make sure the new result matches the fact. Also a hotspot tier1.

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp#L533
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/stubGenerator_riscv.cpp#L581
[3] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp#L1126-L1130
[4] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp#L473-L503

Thanks,
Xiaolin


Progress

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

Issue

  • JDK-8285436: riscv: Fix broken MacroAssembler::debug64

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8357

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 22, 2022

👋 Welcome back xlinzheng! 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 Apr 22, 2022
@openjdk
Copy link

openjdk bot commented Apr 22, 2022

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

  • hotspot

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 label Apr 22, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 22, 2022

Webrevs

@zhengxiaolinX
Copy link
Contributor Author

zhengxiaolinX commented Apr 22, 2022

/label hotspot-compiler
/label remove hotspot
/cc hotspot-compiler

@openjdk openjdk bot added the hotspot-compiler label Apr 22, 2022
@openjdk
Copy link

openjdk bot commented Apr 22, 2022

@zhengxiaolinX
The hotspot-compiler label was successfully added.

@openjdk openjdk bot removed the hotspot label Apr 22, 2022
@openjdk
Copy link

openjdk bot commented Apr 22, 2022

@zhengxiaolinX
The hotspot label was successfully removed.

@openjdk
Copy link

openjdk bot commented Apr 22, 2022

@zhengxiaolinX The hotspot-compiler label was already applied.

@@ -530,7 +530,7 @@ void MacroAssembler::resolve_jobject(Register value, Register thread, Register t

void MacroAssembler::stop(const char* msg) {
address ip = pc();
pusha();
push_reg(0xffffffff, sp);
Copy link
Contributor

@yadongw yadongw Apr 22, 2022

Choose a reason for hiding this comment

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

Good catch, Xiaolin. But I plan to follow JDK-8245986 to use a specific instruction which will raise a SIGILL according to riscv spec. The patch will remove all these push/pop(s).

Copy link
Contributor Author

@zhengxiaolinX zhengxiaolinX Apr 22, 2022

Choose a reason for hiding this comment

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

Thank you for the explanation, Yadong. That would be better if you have future plans for this part, and I feel fine to retract this one as well because it might be not so important.

@zhengxiaolinX zhengxiaolinX deleted the 3-broken-debug64 branch Apr 23, 2022
@TobiHartmann
Copy link
Member

TobiHartmann commented Apr 25, 2022

@zhengxiaolinX Should we close the JBS issue then?

@zhengxiaolinX
Copy link
Contributor Author

zhengxiaolinX commented Apr 25, 2022

@zhengxiaolinX Should we close the JBS issue then?

Thank you! Closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler rfr
3 participants