[PATCH] D73254: [ELF] Rename relocateOne() to relocate() and pass `Relocation` to it
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 23 09:15:44 PST 2020
MaskRay marked 5 inline comments as done.
MaskRay added inline comments.
================
Comment at: lld/ELF/Arch/PPC64.cpp:789
case R_PPC64_ADDR14: {
- checkAlignment(loc, val, 4, type);
+ checkAlignment(loc, val, 4, rel);
// Preserve the AA/LK bits in the branch instruction
----------------
grimar wrote:
> In terms of the original code, seems previously this line used a new `type`, but now it uses an `originalType`.
> Is it intentional and correct change?
It is intentional.
`type` is only used in a diagnostic: `Target.h:reportRangeError`.
The code handle several similar relocation types together, but the original code made a mistake (`type` can be modified but the diagnostic should report the original relocation type). So this change actually also fixes a diagnostic bug (not serious, and I don't expect such alignment errors to happen in practice)
================
Comment at: lld/ELF/Arch/PPC64.cpp:884
case R_PPC64_REL32:
- checkInt(loc, val, 32, type);
+ checkInt(loc, val, 32, rel);
write32(loc, val);
----------------
grimar wrote:
> The same here and below.
They fixed the bug.
================
Comment at: lld/ELF/Target.h:90
+ rel.type = type;
+ relocate(loc, rel, val);
+ }
----------------
grimar wrote:
> It is probably looks a bit strange to have such piece here.
> It does not look to be generic code because initializes only single field.
> And it seems used only once.
`relocateOne` is very common.
```
% rg relocateOne lld | wc -l
72
```
The intention is to thread `Symbol` through relocation handling (context: https://github.com/ClangBuiltLinux/linux/issues/773)
For many `relocateOne`, there is probably not a good `Symbol *` to thread through. We can use `nullptr`, but that will requires changes in the call sites. I think it is just simpler to keep the utility here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73254/new/
https://reviews.llvm.org/D73254
More information about the llvm-commits
mailing list