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

Wed Jun 27 17:15:14 PDT 2018

rahulchaudhry marked an inline comment as done.
rahulchaudhry added inline comments.

================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4326
+        Rela.r_addend = 0;
+        printDynamicRelocation(Obj, Rela, ELF::SHT_RELR);
+      }
----------------
jakehehrlich wrote:
> rahulchaudhry wrote:
> > jakehehrlich wrote:
> > > 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.
> > Regarding 2: -relocations does print SHT_RELA sections for executables and shared objects. That's dynamic relocations, in much the same way as those in SHT_RELR.
> > 
> > Let's say a PIE binary has 10,000 relocations in SHT_RELA, 9,900 of which are relative relocations.
> > llvm-readobj -relocations will print all 10,000 relocations. So far so good.
> > 
> > Now let's say we re-link that binary with -pack-dyn-relocs=relr.
> > llvm-readobj -relocations will print only the remaining 100 relocations in SHT_RELA, and completely omit the rest, that are now encoded in SHT_RELR.
> > This is confusing, to say the least. It makes -relocations output pretty incomplete and useless for binaries containing SHT_RELR sections.
> > 
> > I think -relocations should print the entries in SHT_RELR as well. Whether it prints the encoded hex strings or the decoded relocations by default is up for discussion, and I'm fine either way, but completely skipping SHT_RELR in -relocations output does not sound like a good idea to me.  When someone passes -relocations, they want to list the relocations. Printing only 1% of the relocations and requiring another flag to see the remaining 99% does not look like a reasonable interface.
> Ah sorry, I assumed the wrong cause of some behavior I was seeing. Try this:
> 
> 1. link your executable
> 2. full strip your executable using llvm-objcopy --strip-sections (the binaries I had on hand to play with this happened to be full stripped)
> 3. run llvm-readobj -relocations. You'll see that no relocations are printed out
> 4. run llvm-readobj -dyn-relocations. You'll see that all expected relocations are output.
> 
> I assumed that -relocations checked weather a section was allocated or not before printing. Instead -relocations prints relocations for sections (allocated or not) where as -dyn-relocations looks for PT_DYNAMIC (not for allocated sections) and that explains the difference. (we should probably add a test case for -dyn-relocation).
> 
> So you're right. We should print out on -relocations. I believe it should always be decoded in that case but I'm ok with it printing out raw hex entries with no fake relocation type or addend. There should also be a way of getting the raw hex entries regardless. An additional -dyn-relr flag (you can pick a different name if you'd like) should accomplish that. Also it's not clear a specific -decode-relr flag is needed after that as 90% of the time you'll just want to see what the relocations are and when debugging SHT_RELR related issues you might want to look at the raw entries. 
> 
> I'd accept the functionality you have now with the additional constraint that relocations are always decoded for -dyn-relocations. I'd prefer if the same were true for -relocations and a separate flag allowed printing out the raw entries.
I've changed the default for both -relocations and -dyn-relocations to decode and print relative relocations in SHT_RELR section.
Added a new flag '-raw-relr' to disable this decoding (only for -relocations).
With '-raw-relr', just the raw hex entries are printed. There is no fake relocation type or addend.
Removed the '-decode-relr' flag, since that behavior is default now.
I think this addresses all of your concerns.
ptal.

Repository:
  rL LLVM

https://reviews.llvm.org/D47919