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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Feb 26 05:41:22 PST 2019


jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, but may be worth get another llvm-objcopy developer to give a thumbs up in case I missed anything.



================
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.
> As for the quoting, that should really be tidied up. I'll file a bug.
https://bugs.llvm.org/show_bug.cgi?id=40859


================
Comment at: test/tools/llvm-objcopy/ELF/strip-section-err.test:8
+
+## Check the behavior when we also remove a section with relocation.
+## We have no reference in this case and hence no error should be emitted.
----------------
"remove a section with relocation" -> "remove the relocation section"

I think that's a little clearer.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-section-err.test:11
+
+# RUN: yaml2obj %s > %t1
+# RUN: llvm-objcopy -R .data -R .rela.text %t1 %t2
----------------
Maybe use a different file name for this test case, to make it easier to debug the test in the future.


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

https://reviews.llvm.org/D58625





More information about the llvm-commits mailing list