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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar 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.
+ 
----------------
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.
> 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 :)


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

https://reviews.llvm.org/D58625