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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

So in considering the case you pointed out about -dyn-relocations I discovered that the current behavior for -relocations is to not print out dynamic relocations at all. This is hidden by the fact that dynamic relocations tend to be duplicated with non-dynamic relocations by default. So we should only print these out on -dyn-relocations unless they're duplicated.  As part of my proposal to a solution to this I'm proposing an additional flag so that the raw form that Roland requested can be printed out. This way debugging of the underlying format is possible but for most purposes the decoded form can be used.



================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3418
+        Rela.r_addend = 0;
+        printDynamicRelocation(Obj, Rela, ELF::SHT_RELR);
+      }
----------------
rahulchaudhry wrote:
> jakehehrlich wrote:
> > 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.
> The addend gets skipped in GNUStyle::printDynamicRelocation (it's only printed for RELA).
> This GNUStyle dump is purely speculative, as there is no implementation for this in GNU binutils yet.
> We'll need to come back and adjust the format here anyway once gnu-readelf supports printing SHT_RELR sections.
> 
> Also, see below.
> 
Ok sounds good. I'm aware we'll need to come back. I just want to make a best faith effort first.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4326
+        Rela.r_addend = 0;
+        printDynamicRelocation(Obj, Rela, ELF::SHT_RELR);
+      }
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D47919