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

x86-interrupt calling convention leads to wrong error code in debug mode #57270

Closed
phil-opp opened this issue Jan 2, 2019 · 15 comments
Closed
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@phil-opp
Copy link
Contributor

phil-opp commented Jan 2, 2019

When using the x86-interrupt calling convention for an exception with error code, the error code is wrong in debug mode. I checked the assembly code and it seems like it loads the first field of the exception stack frame instead of the error code, i.e. it is off by 8 bytes, so this seems to be a bug in LLVM. The interesting thing is that the error code is correct in release mode.

It worked correctly with nightly (9389e23a8 2017-12-31), so it is a regression. I'm currently trying different Rust versions to find out when the problem was introduced:

Nightly Works?
(9389e23 2017-12-31) Yes
(8ccab7e 2018-01-31) Yes
(bd98fe0 2018-02-06) Yes
(3bcda48 2018-02-09) Yes
(45fba43 2018-02-10) No
(b8398d9 2018-02-11) No
(4d2d3fc 2018-02-13) No
(0ff9872 2018-02-28) No
(1ffb321 2018-05-31) No
(f4a421e 2018-12-13) No

Edit: The problem was introduced between the 2018-02-09 and the 2018-02-10 nightly. These are the relevant commits: 3bcda48...45fba43. I think it was most likely the upgrade to LLVM 6 in #47828, which was huge.

See phil-opp/blog_os#513 for more context.

@nagisa nagisa added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 2, 2019
@phil-opp
Copy link
Contributor Author

phil-opp commented Jan 3, 2019

I think I found the problem. I'm currently preparing a LLVM patch.

@phil-opp
Copy link
Contributor Author

phil-opp commented Jan 3, 2019

I submitted a patch upstream: https://reviews.llvm.org/D56275.

@phil-opp
Copy link
Contributor Author

phil-opp commented Mar 6, 2019

A patch for fixing this issue was merged to LLVM in rL354837. I'm not sure how closely Rust tracks current LLVM changes and whether it's worth a backport.

@toothbrush7777777
Copy link

@phil-opp Has this been corrected? Does it work in recent Rust nightlies?

@mati865
Copy link
Contributor

mati865 commented May 23, 2019

LLVM 8 (the one which Rust is using) was branched in January and it haven't been backported.
Therefore it's still not fixed.

@toothbrush7777777
Copy link

toothbrush7777777 commented May 23, 2019

@mati865 Thank you for letting me know. Do you know if there are any plans to backport the fix into Rust's copy of LLVM?

@mark-i-m
Copy link
Member

@toothbrush7777777 IIUC we avoid doing that because maintaining the patches on our copy is a pain.

@mark-i-m
Copy link
Member

But the fix is in the LLVM trunk branch, so it should end up in whatever the next version of LLVM is...

@phil-opp
Copy link
Contributor Author

I thought about doing a backport PR, but the diff is quite large because the patch in rL354837 depends on the refactoring in rL351616 (so we would need to include it too). So I think it's better to wait for the next regular update of Rust's LLVM fork.

@toothbrush7777777
Copy link

toothbrush7777777 commented May 23, 2019

So that will be LLVM 9, right? Do you have any idea when that will be finished/copied to Rust's LLVM?

@mati865
Copy link
Contributor

mati865 commented May 23, 2019

LLVM 9 is expected to be released around September 2019, Rust may (or may not) migrate earlier.

@phil-opp
Copy link
Contributor Author

phil-opp commented Jul 8, 2019

There is now a (work in progress) pull request to update Rust to LLVM 9 at rust-lang/llvm-project#19. GitHub doesn't display all commits, so I'm not 100% sure that it contains llvm/llvm-project@2f055f0, but I see no reason why it shouldn't.

@kyrias
Copy link
Contributor

kyrias commented Jul 16, 2019

(I just double checked, and the 9.0-2019-07-12 branch does indeed contain the fix.)

@mati865
Copy link
Contributor

mati865 commented Jul 19, 2019

@phil-opp LLVM 9 has made it's way to nightly, could you retest and close the issue?

@phil-opp
Copy link
Contributor Author

@mati865 Sure. I tried it with both rustc 1.38.0-nightly (bc2e84ca0 2019-07-17) and rustc 1.38.0-nightly (95b1fe560 2019-07-20). Both nightlies reported a correct error code, so the issue seems to be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants