[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