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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4326
+        Rela.r_addend = 0;
+        printDynamicRelocation(Obj, Rela, ELF::SHT_RELR);
+      }
----------------
rahulchaudhry wrote:
> jakehehrlich wrote:
> > rahulchaudhry wrote:
> > > jakehehrlich wrote:
> > > > same thing here.
> > > I'd argue that both type and addend should be printed here for consistency.
> > > 
> > > - The case for addend is easy. LLVMStyle prints it for all relocations, even SHT_REL, which don't have addends. The case for SHT_RELR is similar. A 0x0 is printed as addend in both cases.
> > > 
> > > - For printing a type (even a dummy one, like R_RELR_RELATIVE), the rationale is wrt. two readobj flags: '-dyn-relocations' and '-expand-relocs'.
> > > 
> > >   -dyn-relocations prints all dynamic relocations in a single Dynamic Relocations { } block. It does not separate them by sections as -relocations does. It would be disorienting to have some relocations from .rela.dyn in the block (with full type/symbol/addend info), followed by a bunch of hex numbers (with no type or description), followed by some more relocations from .rela.plt section (with full type/symbol/addend info).
> > > 
> > >   LLVMStyle::printDynamicRelocation also handles -expand-relocs flag and prints each entry on multiple lines. This is another reason to use this function to print relocations here instead of inlining a special case.
> > > 
> > My claim is that these are not actually relocation entries. Each entire corresponds to potentially many relocations and some correspond to none! More over the data seems like an address even though it isn't. I fully agree that in the decode case we should print the full information with the addend. My point is just that the information you're adding dosn't make sense as a relocation. I hadn't considered the behavior of -dyn-relcoations though. You're 100% right that we shouldn't print out raw hex addresses with no other information in that case.  That clearly presents an issue with the specific change I requested here.
> > 
> > What do you think about this proposal:
> > 1) When dynamic relocations are printed out, you decode the relocations so that they make sense.
> > 2) Add an extra switch -dyn-relr for printing out the list of hex entries in all SHT_RELR  sections.
> > 3) If -dyn-relr and -decode-relr are both set then decode those relocations and print them out as you've been doing.
> > 
> > This should resolve the dynamic relocation issue and resolve my concern. The matra here is then "you should cal print*Relocation when you can but you should only call it if you're actually using a real relocation.
> I'm not sure I fully understand the proposal. Here's what I think you're asking for:
> 
> 1. When printing -dyn-relocations, always decode the relr relocations. SGTM.
> 
> 2. With -relocations, print only the SHT_RELR hex entries by default. They're in a separate ".relr.dyn {}" block (for LLVMStyle) or under separate "Relocation section '.relr.dyn'" table (for GNUStyle), so there's no confusion. SGTM.
> 
> 3. With -relocations -decode-relr, decode the SHT_RELR section and print relocation entries in the corresponding .relr.dyn block or table. SGTM.
> 
> Please let me know if I'm missing something.
> 
> a. I don't see why an extra flag -dyn-relr  is needed. What would it be useful for? Simple -relocations without -decode-relr works fine for testing lld output etc.
> b. What happens with -relocations -expand-relocs (without -decode-relr)? What is the expanded form for SHT_RELR hex entries? Is it ok to ignore -expand-relocs for the .relr.dyn block?
> 
Answer to a: I don't think -relocations is the flag that should trigger printing these things but both formats are needed.

Answer to b: Not important because there should be no interaction with -relocations.

Stated in the more clear format you used:

1. Perfect. Exactly what I think should happen.

2. -relocations shouldn't cause SHT_RELR to print at all. It's orthogonal. This is to be consistent with the fact that -relocations doesn't print out dynamic relocations.

3. if -dyn-relr is set, print out the SHT_RELR sections using just hex entries. 

4. if -dyn-relr and -decode-relr are both set, print out the ".relr.dyn {}" format but with decoded relocation entries.


Repository:
  rL LLVM

https://reviews.llvm.org/D47919