[llvm-objcopy][ELF] Fix removing ".group" sections.

[ikudrin]

Summary

When a SHT_GROUP section is removed, but other sections of the group are kept, the SHF_GROUP flag of these sections should be dropped, otherwise the resulting ELF file will be malformed.

[MaskRay]

The code looks good. We have another issue: https://bugs.llvm.org/show_bug.cgi?id=46064 (if a member is deleted, SHT_GROUP content needs a change)

GNU objcopy was fixed by https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=30288845d67c64d07905c1e4ca9de4768d3b2dd8

llvm/test/tools/llvm-objcopy/ELF/remove-group-section.test

1 ↗

(On Diff #265983)

I'd prefer remove-section-group.test because the operation is remove-section.

(SHT_GROUP is referred to as "section group")

29 ↗

(On Diff #265983)

Sections [ this line can be deleted

31 ↗

(On Diff #265983)

Indent keys. See other files for examples

[ikudrin]

• Updated the test according to @MaskRay's comments. Thanks!

[jhenderson]

Sounds reasonable to me. I don't think this behaviour (removing .group sections) has been considered before. Some testing related to that behaviour that we should probably have, but which is outside the scope of this change is to show that you can remove a signature symbol, if the .group section has been removed (assuming that it works of course). Something else that I'm wondering perhaps DOES belong with this change - should we test that if you remove a group section in the same run as removing a member of that group no problems occur? I could imagine an issue with dangling pointers if we didn't do things properly.

llvm/test/tools/llvm-objcopy/ELF/remove-section-group.test

7–8

Nit: use '##' for comments for easy distinction from test commands.

30

Up to you, but you can probably also delete this line too, as you are only dumping sections.

[ikudrin]

• Updated the test. Thanks, @jhenderson!

[ikudrin]

In D80511#2054128, @jhenderson wrote:

Sounds reasonable to me. I don't think this behaviour (removing .group sections) has been considered before. Some testing related to that behaviour that we should probably have, but which is outside the scope of this change is to show that you can remove a signature symbol, if the .group section has been removed (assuming that it works of course).

As far as I can see, the symbol is removed.

Something else that I'm wondering perhaps DOES belong with this change - should we test that if you remove a group section in the same run as removing a member of that group no problems occur? I could imagine an issue with dangling pointers if we didn't do things properly.

The objects which represent the sections are not actually deleted, just moved to Object::RemovedSections to be used later in ELFWriter<ELFT>::writeSegmentData(). Thus, there are no dangling pointers.

[jhenderson]

In D80511#2054966, @ikudrin wrote:

Something else that I'm wondering perhaps DOES belong with this change - should we test that if you remove a group section in the same run as removing a member of that group no problems occur? I could imagine an issue with dangling pointers if we didn't do things properly.

The objects which represent the sections are not actually deleted, just moved to Object::RemovedSections to be used later in ELFWriter<ELFT>::writeSegmentData(). Thus, there are no dangling pointers.

Indeed, that's the current state of the code, so it should work, but it won't necessarily apply in the event of a future refactor. Anyway, LGTM, since this is not really part of this change.

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/remove-section-group.test

30

Move CHECK: before the YAML text as people seem to think the following order

RUN:
CHECK:
source

is easier to read.

llvm/test/tools/llvm-objcopy/ELF/remove-section-group.test

+## This checks that when the header section of a group is removed, the tool
+## drops the flag SHF_GROUP for preserved members of that group.
+
+# RUN: yaml2obj %s -o - \
+# RUN:   | llvm-objcopy -R .group - - \
+# RUN:   | llvm-readobj --sections - \
+# RUN:   | FileCheck %s
+
+# CHECK:      Name: .foo
+# CHECK-NEXT: Type: SHT_PROGBITS
+# CHECK-NEXT: Flags [
+# CHECK-NEXT:   SHF_ALLOC
+# CHECK-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:      ELFCLASS64
+  Data:       ELFDATA2LSB
+  Type:       ET_REL
+  Machine:    EM_X86_64
+Sections:
+  - Name:     .group
+    Type:     SHT_GROUP
+    Info:     foo_grp
+    Members:
+      - SectionOrType:  GRP_COMDAT
+      - SectionOrType:  .foo
+  - Name:     .foo
+    Type:     SHT_PROGBITS
+    Flags:    [ SHF_ALLOC, SHF_GROUP ]
+Symbols:
+  - Name:     foo_grp
+    Section:  .group

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

public:
   removeSectionReferences(bool AllowBrokenLinks,
                           function_ref<bool(const SectionBase *)> ToRemove);
   virtual Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
   virtual void accept(SectionVisitor &Visitor) const = 0;
   virtual void accept(MutableSectionVisitor &Visitor) = 0;
   virtual void markSymbols();
   virtual void
   replaceSectionReferences(const DenseMap<SectionBase *, SectionBase *> &);
+  // Notify the section that it is subject to removal.
+  virtual void onRemove();
 };
 
 class Segment {
 private:
   struct SectionCompare {
     bool operator()(const SectionBase *Lhs, const SectionBase *Rhs) const {
       // Some sections might have the same address if one of them is empty. To
       // fix this we can use the lexicographic ordering on ->Addr and the
public:
 
   void accept(SectionVisitor &) const override;
   void accept(MutableSectionVisitor &Visitor) override;
   void finalize() override;
   Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
   void markSymbols() override;
   void replaceSectionReferences(
       const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
+  void onRemove() override;
 
   static bool classof(const SectionBase *S) {
     return S->OriginalType == ELF::SHT_GROUP;
   }
 };
 
 class DynamicSymbolTableSection : public Section {
 public:

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

Error SectionBase::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
   return Error::success();
 }
 
 void SectionBase::initialize(SectionTableRef SecTable) {}
 void SectionBase::finalize() {}
 void SectionBase::markSymbols() {}
 void SectionBase::replaceSectionReferences(
     const DenseMap<SectionBase *, SectionBase *> &) {}
+void SectionBase::onRemove() {}
 
 template <class ELFT> void ELFWriter<ELFT>::writeShdr(const SectionBase &Sec) {
   uint8_t *B = Buf.getBufferStart() + Sec.HeaderOffset;
   Elf_Shdr &Shdr = *reinterpret_cast<Elf_Shdr *>(B);
   Shdr.sh_name = Sec.NameIndex;
   Shdr.sh_type = Sec.Type;
   Shdr.sh_flags = Sec.Flags;
   Shdr.sh_addr = Sec.Addr;

 
 void GroupSection::replaceSectionReferences(
     const DenseMap<SectionBase *, SectionBase *> &FromTo) {
   for (SectionBase *&Sec : GroupMembers)
     if (SectionBase *To = FromTo.lookup(Sec))
       Sec = To;
 }
 
+void GroupSection::onRemove() {
+  // As the header section of the group is removed, drop the Group flag in its
+  // former members.
+  for (SectionBase *Sec : GroupMembers)
+    Sec->Flags &= ~SHF_GROUP;
+}
+
 void Section::initialize(SectionTableRef SecTable) {
   if (Link == ELF::SHN_UNDEF)
     return;
   LinkSection =
       SecTable.getSection(Link, "Link field value " + Twine(Link) +
                                     " in section " + Name + " is invalid");
   if (LinkSection->Type == ELF::SHT_SYMTAB)
     LinkSection = nullptr;
Error Object::removeSections(bool AllowBrokenLinks,
   // Now make sure there are no remaining references to the sections that will
   // be removed. Sometimes it is impossible to remove a reference so we emit
   // 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());
+    RemoveSec->onRemove();
     RemoveSections.insert(RemoveSec.get());
   }
 
   // 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).