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

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=40818

Removing a section that is used by relocation is an error
we did not report. The patch fixes that.

[jhenderson]

test/tools/llvm-objcopy/ELF/strip-section-err.test

7 ↗

(On Diff #188174)

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.

tools/llvm-objcopy/ELF/Object.cpp

1375 ↗

(On Diff #188174)

Nit: add a space between "e.g." and "remove".

1377 ↗

(On Diff #188174)

alive -> a live (here only, not in the first line).

1378 ↗

(On Diff #188174)

Same as above comment on e.g.

[grimar]

• Addressed review comments.

test/tools/llvm-objcopy/ELF/strip-section-err.test

7 ↗

(On Diff #188174)

What do you think about the updated version (I am printing the relocation name and index)?

[jhenderson]

test/tools/llvm-objcopy/ELF/strip-section-err.test

7 ↗

(On Diff #188174)

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.

tools/llvm-objcopy/ELF/Object.cpp

563 ↗

(On Diff #188198)

StringRef?

570 ↗

(On Diff #188198)

I + 1? Shouldn't this just be I? There's no null relocation to ignore...

Do you need the this->?

1385 ↗

(On Diff #188198)

One more nit: e.g. removed -> e.g. the removed

1377 ↗

(On Diff #188174)

You've lost the "a" here. Full text should be "if a live section critically"

[grimar]

• Addressed review comments.

test/tools/llvm-objcopy/ELF/strip-section-err.test

7 ↗

(On Diff #188174)

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.

tools/llvm-objcopy/ELF/Object.cpp

570 ↗

(On Diff #188198)

I + 1? Shouldn't this just be I? There's no null relocation to ignore...

I did that because in real-life people usually count things starting from 1, not from 0...
I thought that should work in this way here because I think I do not know any tool that would print
indexes of relocations. For example, objdump output is:

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE 
0000000000000001 R_X86_64_PLT32    .data+0xfffffffffffffffd

and readelf's is:

Relocation section '.rela.text' at offset 0x90 contains 1 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000001  000200000004 R_X86_64_PLT32    0000000000000000 .data - 3

Maybe we should print the Offset then, not an index? It seems to be more natural for relocations.
I did that change, what do you think?

Do you need the this->?

No. but all other places in this file using this->Name, for example, see the first error message in this file.
I would keep it for consistency. (Maybe it worth to remove them all at once in a follow-up, though).

[grimar]

tools/llvm-objcopy/ELF/Object.cpp

570 ↗

(On Diff #188198)

first error message in this file -> first error message in this method

[jhenderson]

test/tools/llvm-objcopy/ELF/strip-section-err.test

1 ↗

(On Diff #188321)

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.

5 ↗

(On Diff #188321)

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.

7 ↗

(On Diff #188174)

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.

[grimar]

test/tools/llvm-objcopy/ELF/strip-section-err.test

7 ↗

(On Diff #188174)

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 :)

[grimar]

• Addressed comments.

test/tools/llvm-objcopy/ELF/strip-section-err.test

1 ↗

(On Diff #188321)

Done.

5 ↗

(On Diff #188321)

Yes. Added such test.

[jhenderson]

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

test/tools/llvm-objcopy/ELF/strip-section-err.test

8 ↗

(On Diff #188347)

"remove a section with relocation" -> "remove the relocation section"

I think that's a little clearer.

11 ↗

(On Diff #188347)

Maybe use a different file name for this test case, to make it easier to debug the test in the future.

7 ↗

(On Diff #188174)

As for the quoting, that should really be tidied up. I'll file a bug.

https://bugs.llvm.org/show_bug.cgi?id=40859

[rupprecht]

test/tools/llvm-objcopy/ELF/strip-section-err.test

7 ↗

(On Diff #188174)

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.

[grimar]

test/tools/llvm-objcopy/ELF/strip-section-err.test

7 ↗

(On Diff #188174)

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'll think about how to reduce it. Will suggest a separate follow-up patch.

llvm/trunk/test/tools/llvm-objcopy/ELF/strip-section-err.test

+## Check we cannot remove a section containing symbols
+## referenced by relocations contained in the object.
+
+# 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 patching offset 0x1 from section .rela.text.
+
+## Check the behavior when we also remove the relocation section.
+## We have no reference in this case and hence no error should be emitted.
+
+# RUN: yaml2obj %s > %t2
+# RUN: llvm-objcopy -R .data -R .rela.text %t2 %t3
+# RUN: llvm-objdump --section-headers %t3 | FileCheck %s --check-prefix=NOSEC
+# NOSEC-NOT: .data
+# NOSEC-NOT: .rela.text
+
+--- !ELF
+FileHeader:      
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:        
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000004
+    Content:         E800000000
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Link:            .symtab
+    AddressAlign:    0x0000000000000008
+    EntSize:         0x0000000000000018
+    Info:            .text
+    Relocations:     
+      - Offset:          0x0000000000000001
+        Symbol:          foo
+        Type:            R_X86_64_GOTPCREL
+        Addend:          -4
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x0000000000000001
+    Content:         '0102'
+Symbols:         
+  Local:           
+    - Name:            foo
+      Section:         .data
+      Value:           0x0000000000000001
+DynamicSymbols:  {}
+...

llvm/trunk/tools/llvm-objcopy/ELF/Object.h

static bool classof(const SectionBase *S) {
     return S->Type == ELF::SHT_REL || S->Type == ELF::SHT_RELA;
   }
 };
 
 // Takes the symbol table type to use as a parameter so that we can deduplicate
 // that code between the two symbol table types.
 template <class SymTabType>
 class RelocSectionWithSymtabBase : public RelocationSectionBase {
-  SymTabType *Symbols = nullptr;
   void setSymTab(SymTabType *SymTab) { Symbols = SymTab; }
 
 protected:
   RelocSectionWithSymtabBase() = default;
 
+  SymTabType *Symbols = nullptr;
+
 public:
-  Error removeSectionReferences(
-      function_ref<bool(const SectionBase *)> ToRemove) override;
   void initialize(SectionTableRef SecTable) override;
   void finalize() override;
 };
 
 class RelocationSection
     : public RelocSectionWithSymtabBase<SymbolTableSection> {
   MAKE_SEC_WRITER_FRIEND
 
   std::vector<Relocation> Relocations;
 
 public:
   void addRelocation(Relocation Rel) { Relocations.push_back(Rel); }
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
+  Error removeSectionReferences(
+      function_ref<bool(const SectionBase *)> ToRemove) override;
   Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
   void markSymbols() override;
 
   static bool classof(const SectionBase *S) {
     if (S->Flags & ELF::SHF_ALLOC)
       return false;
     return S->Type == ELF::SHT_REL || S->Type == ELF::SHT_RELA;
   }

llvm/trunk/tools/llvm-objcopy/ELF/Object.cpp

 void SymbolTableSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }
 
 void SymbolTableSection::accept(MutableSectionVisitor &Visitor) {
   Visitor.visit(*this);
 }
 
-template <class SymTabType>
-Error RelocSectionWithSymtabBase<SymTabType>::removeSectionReferences(
+Error RelocationSection::removeSectionReferences(
     function_ref<bool(const SectionBase *)> ToRemove) {
   if (ToRemove(Symbols))
     return createStringError(llvm::errc::invalid_argument,
                              "Symbol table %s cannot be removed because it is "
                              "referenced by the relocation section %s.",
                              Symbols->Name.data(), this->Name.data());
+
+  for (const Relocation &R : Relocations) {
+    if (!R.RelocSymbol->DefinedIn || !ToRemove(R.RelocSymbol->DefinedIn))
+      continue;
+    return createStringError(
+        llvm::errc::invalid_argument,
+        "Section %s cannot be removed because of symbol '%s' "
+        "used by the relocation patching offset 0x%" PRIx64 " from section %s.",
+        R.RelocSymbol->DefinedIn->Name.data(), R.RelocSymbol->Name.c_str(),
+        R.Offset, this->Name.data());
+  }
+
   return Error::success();
 }
 
 template <class SymTabType>
 void RelocSectionWithSymtabBase<SymTabType>::initialize(
     SectionTableRef SecTable) {
   if (Link != SHN_UNDEF)
     setSymTab(SecTable.getSectionOfType<SymTabType>(
Error Object::removeSections(
   // an error here instead.
   std::unordered_set<const SectionBase *> RemoveSections;
   RemoveSections.reserve(std::distance(Iter, std::end(Sections)));
   for (auto &RemoveSec : make_range(Iter, std::end(Sections))) {
     for (auto &Segment : Segments)
       Segment->removeSection(RemoveSec.get());
     RemoveSections.insert(RemoveSec.get());
   }
-  for (auto &KeepSec : make_range(std::begin(Sections), Iter))
+
+  // For each section that remains alive, we want to remove the dead references.
+  // This either might update the content of the section (e.g. remove symbols
+  // from symbol table that belongs to removed section) or trigger an error if
+  // a live section critically depends on a section being removed somehow
+  // (e.g. the removed section is referenced by a relocation).
+  for (auto &KeepSec : make_range(std::begin(Sections), Iter)) {
     if (Error E = KeepSec->removeSectionReferences(
             [&RemoveSections](const SectionBase *Sec) {
               return RemoveSections.find(Sec) != RemoveSections.end();
             }))
       return E;
+  }
+
   // Now finally get rid of them all togethor.
   Sections.erase(Iter, std::end(Sections));
   return Error::success();
 }
 
 Error Object::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
   if (SymbolTable)
     for (const SecPtr &Sec : Sections)