llvm-jitlink: Fix bug in target address computation. #138794
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.6-beta.1
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
(#138795 is the followup change in question.) |
lhames
reviewed
May 7, 2025
| @@ -194,7 +194,8 @@ Error registerELFGraphInfo(Session &S, LinkGraph &G) { | |||
| else | |||
| FileInfo.SectionInfos[Sec.getName()] = { | |||
| ArrayRef<char>(FirstSym->getBlock().getContent().data(), SecSize), | |||
| SecAddr.getValue(), FirstSym->getTargetFlags()}; | |||
| (SecAddr - FirstSym->getOffset()).getValue(), | |||
There was a problem hiding this comment.
Instead of setting this here, it seems like it'd be tidier to define SecAddr like this on line 183:
auto SecAddr = FirstSym->getAddress() - FirstSym->getOffset();Unless you've already tried that and hit other issues I'm happy to go ahead and make that change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
llvm-jitlink has a bug revealed by my followup change where the
RuntimeDyldChecker::MemoryRegionInfo::TargetAddress field was
incorrectly being set to the address of the first symbol in the
section instead of the address of the section, which caused the
ExecutionEngine/JITLink/x86-64/ELF_small_pic_relocations.s test to
fail, so fix that for ELF. It looks like the same bug could exist for
the other object formats (but maybe not for Mach-O because of the atom
model?) but I'm not familiar with the JIT linker so I left them as is.
This change will be tested implicitly when my followup change lands.