[PATCH] D47919: llvm-readobj: add experimental support for SHT_RELR sections.

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

I'll have more comments tomorrow.  I'm still grokking my way though llvm-readobj as I've never looked at that code before.

I have a general question about GNUStyle though. Why should GNUStyle with Relr be so different from GNUStyle for Rel(a)? Shouldn't (ideally) relr be transparently usable without a difference in how it is printed out to the user? e.g. relr should print out something indistinguishable from if the linker had used rela/rel relocation instead.

If r_info can't be sensibly set then I think the only real option is to have it be output differently for both IMO. We shouldn't be outputting a relocation type that's incorrect. I'm not sure it makes sense to decode things quite this way when you don't have an operating system or architecture specific solution (as was the case with android). I think LLD defines some common relocation types across architectures but that isn't super useful here. I wouldn't mind that being duplicated here (maybe pcc disagrees though).



================
Comment at: include/llvm/BinaryFormat/ELF.h:1077
+// Relocation entry without explicit addend or info (relative relocations only).
+struct Elf32_Relr {
+  Elf32_Word r_data; // offset/bitmap for relative relocations
----------------
I think in the spec you propose Elf32_Relr and Elf64_Relr as typedefs instead of structs. I think this file (and the higher level ELFType file as well) should match the spec in that regard.


================
Comment at: lib/Object/ELF.cpp:229
+  Elf_Rela Rela;
+  Rela.r_info = 0;
+  Rela.r_addend = 0;
----------------
Is there a more sensible info we can set this to? It would have to be processor specific which might be super annoying. If there isn't a reasonable way to set the relocation type then I think it should be dropped llvm-readobj output in the llvm case.


================
Comment at: lib/Object/ELF.cpp:232
+  std::vector<Elf_Rela> Relocs;
+  size_t WordSize = sizeof(Elf_Addr);
+
----------------
can this be made constexpr? or at least static const?


================
Comment at: lib/Object/ELF.cpp:234
+
+  typename ELFT::uint Base = 0;
+  for (const Elf_Relr &R : relrs) {
----------------
nit: I think I'd prefer this use Elf_Addr since it represents an offset for a dynamic relocation (e.g. and address)


================
Comment at: lib/Object/ELF.cpp:247
+    // Odd entry: encodes bitmap for relocations starting at base.
+    typename ELFT::uint Offset = Base;
+    while (Entry != 0) {
----------------
nit I think I'd prefer this use Elf_Addr for the same reason.


================
Comment at: test/tools/llvm-readobj/elf-relr-relocs.test:6
+# LLVM1:      Section (1) .relr.dyn {
+# LLVM1-NEXT:   0x10D60 R_X86_64_NONE - 0x0
+# LLVM1-NEXT:   0x10D68 R_X86_64_NONE - 0x0
----------------
R_X86_64_NONE is not a valid relocation type. See my r_info comment above about this issue.


Repository:
  rL LLVM

https://reviews.llvm.org/D47919