[PATCH] D58625: [llvm-objcopy] - Check for invalidated relocations when removing a section.

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht accepted this revision.
rupprecht added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-section-err.test:7
+
+# CHECK: error: Section .data cannot be removed because it is referenced by the relocation section .rela.text.
+ 
----------------
grimar wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > grimar wrote:
> > > > jhenderson wrote:
> > > > > grimar wrote:
> > > > > > jhenderson wrote:
> > > > > > > The relocations don't really reference the section, but rather a symbol in that section. I think the error message should reflect that somehow. It would also be great if the index of the relocation could be mentioned in the error message.
> > > > > > What do you think about the updated version (I am printing the relocation name and index)?
> > > > > Thanks, that looks much better, although I don't think there's much benefit in the type of relocation. I think we should probably be quoting section names too, but be consistent with what we do in other error messages.
> > > > Ok, I removed the type of relocation from the output.
> > > > That makes an interface to be slightly simpler.
> > > > 
> > > > Speaking about consistency of error messages, objdump is not very consistent.
> > > > It can quote/not quote the symbol names:
> > > > 
> > > > ```
> > > >     return createStringError(llvm::errc::invalid_argument,
> > > >                              "Symbol %s cannot be removed because it is "
> > > >                              "referenced by the section %s[%d].",
> > > >                              Sym->Name.data(), this->Name.data(), this->Index);
> > > > 
> > > > ```
> > > > 
> > > > ```
> > > >       return createStringError(
> > > >           llvm::errc::invalid_argument,
> > > >           "not stripping symbol '%s' because it is named in a relocation.",
> > > >           Reloc.RelocSymbol->Name.data());
> > > > ```
> > > > 
> > > > And seems never quotes the section names.
> > > Hmm... so the offset is where the relocation patches, and doesn't necessarily make it easy to locate the relocation either (plus technically it's not illegal in the ELF spec to have two relocations patching the same offset). Maybe it's worth some others chiming in? @rupprecht/@jakehehrlich?
> > > 
> > > By the way, I'd say "patching offset" not "at offset", since the relocation is sat at a completely unrelated offset in its own section so this is otherwise a bit ambiguous.
> > > 
> > > As for the quoting, that should really be tidied up. I'll file a bug.
> > > As for the quoting, that should really be tidied up. I'll file a bug.
> > https://bugs.llvm.org/show_bug.cgi?id=40859
> > Hmm... so the offset is where the relocation patches, and doesn't necessarily make it easy to locate the relocation either 
> 
> Two points:
> 1) In compare with GNU objcopy which reports "bar2.o: symbol `foobar' required but not present" it a lot easier.
> 2) We report a symbol name, section and relocation's offset together. This should be enough to find the place where things went wrong I think. For example, LLD might report a section name, offset and relocation name too:
> `(.nonalloc+0x1): has non-ABS relocation R_X86_64_PC32 against symbol '_start'`
> It works :)
> error: Section .data cannot be removed because of symbol 'foo' used by the relocation patching offset 0x1 from section .rela.text.

The amount of technical information is good, but I think the rest of it is too verbose... any way to make it a bit more compact would be nice. The LLD error looks a little better, maybe we should use something like that?

I don't want to bikeshed on the wording too much though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58625/new/

https://reviews.llvm.org/D58625