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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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:
> > 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.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-section-err.test:1
+## Check we cannot remove the section .data because it
+## is used by relocations contained in the object.
----------------
I'd make this comment a little more general for the behaviour, by removing references to the section name and describing what is the interesting characteristic of it. Something like "Check we cannot remove a section containing symbols referenced by relocations contained in the object." or something along those lines.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-section-err.test:5
+# RUN: yaml2obj %s > %t1
+# RUN: not llvm-objcopy -R .data %t1 2>&1 | FileCheck %s
+# CHECK: error: Section .data cannot be removed because of symbol 'foo' used by the relocation at offset 0x1 from section .rela.text.
----------------
In addition to this test case, I've thought of another one: what happens if you remove both .data and .rela.text? That shouldn't produce an error, since the reference has also been removed.


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

https://reviews.llvm.org/D58625