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

[JITLink] fix i686 R_386_32 relocation value #111091

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Oct 4, 2024

My i686 builds were segfaulting when trying to enable JITLink, and I traced it back to an incorrect Addend computation here. Most relocations involve adding the value at the location with a value taken from elsewhere. This one was missing the addend taken from the location, but other relocations may be missing this too?

@lhames @llvm/issue-subscribers-julialang

@vtjnash vtjnash changed the title [JITLink] fix i668 R_386_32 relocation value [JITLink] fix i686 R_386_32 relocation value Oct 4, 2024
@lhames
Copy link
Contributor

lhames commented Oct 4, 2024

Thanks very much at @vtjnash! Do you think you could add a test with a non-zero addend in llvm/test/ExecutionEngine/JITLink/i386 to test this?

@vtjnash vtjnash force-pushed the jn/JITLink-R_386_32 branch from 202a0a1 to 4731ac2 Compare October 4, 2024 17:04
@vtjnash
Copy link
Member Author

vtjnash commented Oct 4, 2024

Yes, thanks for pointing out where the tests are. I fixed all of the other relocations too, since most of them were similarly wrong.

Copy link

github-actions bot commented Oct 4, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@vtjnash vtjnash force-pushed the jn/JITLink-R_386_32 branch from 4731ac2 to 0abd57d Compare October 4, 2024 17:08
Most relocations involve adding the value at the location with a value
taken from elsewhere. Most of them were hard coded instead. Assume
signed, since the math is the same either way after truncation, but this
seems more likely to give reasonable looking intermediate results.
@vtjnash vtjnash force-pushed the jn/JITLink-R_386_32 branch from 0abd57d to 0d56a9f Compare October 4, 2024 17:11
@vtjnash
Copy link
Member Author

vtjnash commented Oct 15, 2024

bump

Copy link
Contributor

@lhames lhames left a comment

Choose a reason for hiding this comment

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

This is great stuff. Thanks very much @vtjnash!

Thanks for taking the time to add those test cases too.

@vtjnash
Copy link
Member Author

vtjnash commented Oct 16, 2024

I don't actually have merge rights anymore, so you will need to land it for me

@lhames lhames merged commit 5716f83 into llvm:main Oct 16, 2024
9 checks passed
@lhames
Copy link
Contributor

lhames commented Oct 16, 2024

@vtjnash Thanks for the heads up. Merged. :)

bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 16, 2024
Fix R_386_32 and other relocations by correcting Addend computations.
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
Fix R_386_32 and other relocations by correcting Addend computations.
EricWF pushed a commit to efcs/llvm-project that referenced this pull request Oct 22, 2024
Fix R_386_32 and other relocations by correcting Addend computations.
@vtjnash vtjnash deleted the jn/JITLink-R_386_32 branch October 23, 2024 15:17
giordano pushed a commit to JuliaLang/llvm-project that referenced this pull request Nov 3, 2024
Fix R_386_32 and other relocations by correcting Addend computations.

(cherry picked from commit 5716f83)
giordano pushed a commit to JuliaLang/llvm-project that referenced this pull request Nov 21, 2024
Fix R_386_32 and other relocations by correcting Addend computations.

(cherry picked from commit 5716f83)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants