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.

Sign up for GitHub

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

Jump to bottom

8304389: [11u] Crash on Windows in C2 compiled code after 8248238 and 8218431 #70

Closed
wants to merge 3 commits into from
Closed

8304389: [11u] Crash on Windows in C2 compiled code after 8248238 and 8218431 #70

wants to merge 3 commits into from

Conversation

GoeLin
Copy link
Member

@GoeLin GoeLin commented Mar 17, 2023 •
edited by openjdk bot
Loading

… 8218431

A required fix to avoid regression in 11.0.19. See JBS issue.

Progress

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

Issue

  • JDK-8304389: [11u] Crash on Windows in C2 compiled code after 8248238 and 8218431

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 70

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u/pull/70.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 17, 2023

👋 Welcome back goetz! 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 changed the title 8304389: [11u] Crash on Windows in C2 compiled code after 8248238 and… 8304389: [11u] Crash on Windows in C2 compiled code after 8248238 and 8218431 Mar 17, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 17, 2023
@mlbridge
Copy link

mlbridge bot commented Mar 17, 2023 •
edited
Loading

Webrevs

dean-long
dean-long suggested changes Mar 17, 2023
View reviewed changes
Copy link
Member

@dean-long dean-long left a comment

Choose a reason for hiding this comment

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

It sounds like the real problem is the #ifdef _WIN64 in x86_64.ad. If that code is wrong for 64-bit Windows, shouldn't it be removed?

@GoeLin
Copy link
Member Author

GoeLin commented Mar 20, 2023 •
edited
Loading

Hi @dean-long
I am not yet completely sure what is really wrong. The point is that the test only crashes in 11 since we enabled the #ifdef by accident. It is enabled in 16+, too, but going back all the way to starting development of 16 we don't have a single crash in that code in our test database. In 11, it is 100% reproducible. So I assumed there is another part missing that should be backported, too.
As release of 11.0.19 is coming up, I first want to undo the change in 11 to avoid the regression.

@GoeLin
Copy link
Member Author

GoeLin commented Mar 20, 2023 •
edited
Loading

Actually, the configuration leading to errors exactly implements the calling conventions as speified here https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-170#callercallee-saved-registers .

The other configuration, doing more save-on-call, is always correct, but inefficient in case the registers are also saved-on-entry. In that case the registers are saved twice. This is the configuration established by this change again.

@dean-long
Copy link
Member

There is probably some x64 code that is assuming Linux ABI for RSI and RDI, so it breaks when trying to use the Windows ABI. And the _WIN64 ABI settings in x86_64.ad may be effectively dead code that was never tested because of the makefile issue that never defined _WIN64 for adlc before.

@dean-long
Copy link
Member

Nevermind, I think you're right, these _WIN64 ABI settings have been enabled in C2 since jdk16, so it's not clear why there is a problem only in 11u.

@dean-long
Copy link
Member

@GoeLin, after more consideration, your fix does seem safest. However, it might help to have a comment explaining why that change has been disabled. So it might be better to comment out that .gmk logic rather than delete it.

@GoeLin
Copy link
Member Author

GoeLin commented Mar 21, 2023

Hi @dean-long, if I comment it out, it is no more set on aarch64.
I can add a comment pointing to this bug though.
But maybe it is better to fiddle with the ad file as you say.
In case a later backport brings new #ifdef _WIN64 coding to the ad files.
Currently, the problematic switch is the only occurance.

By the way, as I said in the JBS issue, I think the change making the difference in 16 is "JDK-8256205: Simplify compiler calling convention handling"

@dean-long
Copy link
Member

Yes, you need _WIN64 set for windows-aarch64 even if you comment out or remove the isTargetCpuBits block.

My concern with just changing just the .ad file is there are other uses of #ifdef _WIN64 in x86 .hpp header files that adlc files might include directly or indirectly.

To me JDK-8256205 just looks like a refactoring. I don't see how it would change the calling convention.

@GoeLin
Copy link
Member Author

GoeLin commented Mar 21, 2023 •
edited
Loading

Hi
I pushed a new change. #if 0 is not supported by adlc, therefore #ifdef WRONG

My concern with just changing just the .ad file is there are other uses of #ifdef _WIN64 in x86 .hpp header files that adlc files might include directly or indirectly.

adlc reads the ad file, evaluates the _WIN64, swallows it and outputs C++ files without that define.
The C++ build compiling the generated files has _WIN64 set anyways, already before the trouble we look at here.

dean-long
dean-long approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@dean-long dean-long left a comment

Choose a reason for hiding this comment

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

OK, this looks reasonable. Instead of #ifdef WRONG, how about something like #ifdef DISABLED instead?
I suggest filing a separate bug/RFE to investigate the reason for the crash.
Finally, please get a 2nd review.

TheRealMDoerr
TheRealMDoerr approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Choose a reason for hiding this comment

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

Ok. There seems to be code which clobbers at least RSI in 11u. Having the caller preserve these registers over runtime calls looks like a feasible workaround. I think this is a good fix for the 11u release. I'd use #if 0 instead of checking for DISABLED.

@openjdk
Copy link

openjdk bot commented Mar 21, 2023

@GoeLin This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8304389: [11u] Crash on Windows in C2 compiled code after 8248238 and 8218431

Reviewed-by: dlong, mdoerr

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 21, 2023
@GoeLin
Copy link
Member Author

GoeLin commented Mar 21, 2023

Hi Martin,
adlc implements checking #ifdef. It can not parse #if. Therefore, as I stated above, #if 0 is not working.
Thanks for your review!

@GoeLin
Copy link
Member Author

GoeLin commented Mar 21, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Mar 21, 2023

Going to push as commit f1c92b4.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 21, 2023
@openjdk openjdk bot closed this Mar 21, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 21, 2023
@openjdk
Copy link

openjdk bot commented Mar 21, 2023

@GoeLin Pushed as commit f1c92b4.

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

@GoeLin GoeLin deleted the goetz_8304389 branch March 21, 2023 14:43
@TheRealMDoerr
Copy link
Contributor

TheRealMDoerr commented Mar 21, 2023 •
edited
Loading

This fix is also missing in 11u: https://bugs.openjdk.org/browse/JDK-8254252
Would probably be needed before enabling RSI and RDI as callee saved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dean-long dean-long dean-long approved these changes

@TheRealMDoerr TheRealMDoerr TheRealMDoerr approved these changes

Assignees
No one assigned
Labels
integrated Pull request has been integrated
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GoeLin @dean-long @TheRealMDoerr