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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

I think I'm more or less happy with the llvm-readobj part of this.

For yaml2obj I think it would be ideal to support a new relocation type that reads in a list of addresses, sets SHF_ALLOC, and computes the section contents from the addresses. Alternatively I'd be ok with allowing for individual entries to manually added. I'm just not a huge fan of forcing yaml2obj users to put a big hex blob in the middle of their test.



================
Comment at: include/llvm/Object/ELFTypes.h:66
   using Rela = Elf_Rel_Impl<ELFType<E, Is64>, true>;
+  using Relr = Elf_Relr_Impl<ELFType<E, Is64>>;
   using Verdef = Elf_Verdef_Impl<ELFType<E, Is64>>;
----------------
This should also be a typedef the way Addr is below.


================
Comment at: include/llvm/Object/ELFTypes.h:455
+  LLVM_ELF_IMPORT_TYPES(TargetEndianness, false)
+  Elf_Word r_data; // offset/bitmap for relative relocations
+};
----------------
You shouldn't use r_data in ELFTypes either. It should also be a typedef, just for 'packed' instead.


================
Comment at: lib/Object/ELF.cpp:234
+
+  typename ELFT::uint Base = 0;
+  for (const Elf_Relr &R : relrs) {
----------------
rahulchaudhry wrote:
> jakehehrlich wrote:
> > nit: I think I'd prefer this use Elf_Addr since it represents an offset for a dynamic relocation (e.g. and address)
> Defined a 'Word' type locally, and using it for Base/Entry/Offset now.
> Elf_Addr is a packed version of this uint type for endianness conversion.
> At least Entry can't be Elf_Addr, since we need to do bit-shifts on it (operator >>= is undefined for packed types).
> And if Entry is Word, then it makes sense to keep Base/Offset as Word as well, otherwise we end up with several unnecessary conversions below when doing arithmetic and assignments.
Ah gotcha...hmm. I think that's an acceptable reason. If you run into anything like this in LLD and Rui or someone gives a better recommendation, please come back and change this. Otherwise consider this done.


================
Comment at: test/tools/llvm-readobj/elf-relr-relocs.test:12
+# LLVM1-NEXT:   0xF0501         R_RELR_RELATIVE - 0x0
+# LLVM1-NEXT:   0xA700550400009 R_RELR_RELATIVE - 0x0
+# LLVM1-NEXT: }
----------------
I think you can drop the R_RELR_RELATIVE and the addend in this case.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3418
+        Rela.r_addend = 0;
+        printDynamicRelocation(Obj, Rela, ELF::SHT_RELR);
+      }
----------------
As mentioned above, I think in this case you don't have to print out the relocation type and addend. You might just inline something here that prints out the relr entry instead.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4106
+        Rela.r_addend = 0;
+        printRelocation(Obj, Rela, SymTab, Sec->sh_type);
+      }
----------------
Same thing here.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4326
+        Rela.r_addend = 0;
+        printDynamicRelocation(Obj, Rela, ELF::SHT_RELR);
+      }
----------------
same thing here.


Repository:
  rL LLVM

https://reviews.llvm.org/D47919