[PATCH] D142880: [RISCV][LLD] Support R_RISCV_SET_ULEB128 and R_RISCV_SUB_ULEB128

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 requested changes to this revision.
jrtc27 added a comment.
This revision now requires changes to proceed.

- What about relocations in non-SHF_ALLOC sections? Surely that can happen for debug info?

- Test has no coverage of errors



================
Comment at: lld/ELF/Arch/RISCV.cpp:111-113
+  error(errPlace.loc + " ULEB128 value has extended the length by " +
+        toString(type) + "relocation from " + Twine(oldLength) + " byte to " +
+        Twine(newLength) + " byte");
----------------
This doesn't make grammatical sense. Please rewrite this to be in the style of elf::reportRangeError; I'd like to see it print something like:

```
error: foo.o+0x1234: ULEB128 difference relocation pair overflow: 5 bytes needed but only 4 bytes allocated; references 'foo' - 'bar'
>>> 'foo' defined in foo.o
>>> 'bar' defined in bar.o
>>> referenced by bar.c:12
>>>               bar.o:(.baz+0x4)
```

and I guess also with the -fdebug-types-section message. reportRangeError does a lot of useful things, it would be a shame to deviate from that and lose the information.




================
Comment at: lld/ELF/Arch/RISCV.cpp:331
+void RISCV::relocateAlloc(InputSectionBase &sec, uint8_t *buf) const {
+  const unsigned bits = config->is64 ? 64 : 32;
+  uint64_t secAddr = sec.getOutputSection()->addr;
----------------
`config->wordsize * 8;`


================
Comment at: lld/ELF/Arch/RISCV.cpp:337
+  uint8_t *lastULEB128SetLoc = nullptr;
+  uint64_t lastULEB128SetVal = 0;
+  for (const Relocation &rel : sec.relocs()) {
----------------
Why not just keep a pointer to the relocation around instead?


================
Comment at: lld/ELF/Arch/RISCV.cpp:353
+        error(errPlace.loc +
+              " More than one R_RISCV_SET_ULEB128 in same location");
+      }
----------------
Why do you actually need to enforce this? So long as the SET/SUB pairs are adjacent it's fine


================
Comment at: lld/ELF/Arch/RISCV.cpp:363
+        error(errPlace.loc +
+              " R_RISCV_SUB_ULEB128 must come after R_RISCV_SET_ULEB128");
+        break;
----------------
There are two cases this handles:
1. a SUB with no preceding SET
2. a SUB with a different preceding SET

This error isn't really correct. What you want to say is that you've found a SUB which is not, when looking at solely ULEB128 relocations, directly preceded by its corresponding SET.


================
Comment at: lld/ELF/Arch/RISCV.cpp:369
+      uint64_t newVal = lastULEB128SetVal - val;
+      unsigned newLength = encodeULEB128(newVal, loc, /*PadTo*/ oldLength);
+      checkULEB128LengthNoExtend(loc, rel.type, oldLength, newLength);
----------------
What if this walks off the end of the buffer? It's an obscure edge case but could happen if you have a too-small buffer at the end of the file. Then we'll crash rather than catch the error early. Check getULEB128Size first.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142880/new/

https://reviews.llvm.org/D142880