[llvm-objcopy][ELF] Fix removing a group member section.

[ikudrin]

Summary

When a group member is removed, the corresponding record in the SHT_GROUP section has to be also deleted.

This fixes PR46064.

[MaskRay]

LG, but please wait for other opinions.

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

11

The excessive indentation should be deleted.

[jhenderson]

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

7

I'd probably move this comment above the RUN bit, since it describes the test as a whole.

18

I suspect (don't know for sure) that you can omit the Link line here. I think yaml2obj adds it automatically by default for these sort of sections.

34

I personally find it easier to follow a test if the CHECK is closer to where it is used by FileCheck. I'd layout the file (with optional new lines at appropriate spots):

## Comments
# RUN: ...
# CHECK:

<yaml>

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

974

What happens if you remove the symbol table itself? I'm slightly concerned that it could leave the sh_link field invalid as presumably Section::removeSectionReferences won't be called for the GroupSection.

[ikudrin]

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

974

That is another issue with group sections, but not something which is introduced by this patch. Note that the base class for GroupSection is SectionBase, not Section. We need another patch for that.

[jhenderson]

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

974

Thanks. I suspected something like that might have been the case. I guess it's something that needs fixing. Are you going to fix it shortly, or shall I file a bug?

[ikudrin]

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

974

Honestly, fixing that is a bit off my plans.

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

+## This checks that the group section is shrunk when its member is removed.
+
+# RUN: yaml2obj %s -o - \
+# RUN:   | llvm-objcopy -R .foo - - \
+# RUN:   | obj2yaml - \
+# RUN:   | FileCheck %s
+
+# CHECK:      - Name: .group
+# CHECK:        Members:
+# CHECK-NEXT:     - SectionOrType:  GRP_COMDAT
+# CHECK-NEXT:     - SectionOrType:  .bar
+# CHECK-NOT:      - SectionOrType:
+
+--- !ELF
+FileHeader:
+  Class:      ELFCLASS64
+  Data:       ELFDATA2LSB
+  Type:       ET_REL
+  Machine:    EM_X86_64
+Sections:
+  - Name:     .group
+    Type:     SHT_GROUP
+    Info:     foo_bar_grp
+    Members:
+      - SectionOrType:  GRP_COMDAT
+      - SectionOrType:  .foo
+      - SectionOrType:  .bar
+  - Name:     .foo
+    Type:     SHT_PROGBITS
+    Flags:    [ SHF_ALLOC, SHF_GROUP ]
+  - Name:     .bar
+    Type:     SHT_PROGBITS
+    Flags:    [ SHF_ALLOC, SHF_GROUP ]
+Symbols:
+  - Name:     foo_bar_grp
+    Section:  .group

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

public:
   void setSymTab(const SymbolTableSection *SymTabSec) { SymTab = SymTabSec; }
   void setSymbol(Symbol *S) { Sym = S; }
   void setFlagWord(ELF::Elf32_Word W) { FlagWord = W; }
   void addMember(SectionBase *Sec) { GroupMembers.push_back(Sec); }
 
   void accept(SectionVisitor &) const override;
   void accept(MutableSectionVisitor &Visitor) override;
   void finalize() override;
+  Error removeSectionReferences(
+      bool AllowBrokenLinks,
+      function_ref<bool(const SectionBase *)> ToRemove) 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;

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

void ELFSectionSizer<ELFT>::visit(RelocationSection &Sec) {
   Sec.Size = Sec.Relocations.size() * Sec.EntrySize;
   // Align to the largest field in Elf_Rel(a).
   Sec.Align = ELFT::Is64Bits ? sizeof(Elf_Xword) : sizeof(Elf_Word);
 }
 
 template <class ELFT>
 void ELFSectionSizer<ELFT>::visit(GnuDebugLinkSection &Sec) {}
 
-template <class ELFT> void ELFSectionSizer<ELFT>::visit(GroupSection &Sec) {}
+template <class ELFT> void ELFSectionSizer<ELFT>::visit(GroupSection &Sec) {
+  Sec.Size = sizeof(Elf_Word) + Sec.GroupMembers.size() * sizeof(Elf_Word);
+}
 
 template <class ELFT>
 void ELFSectionSizer<ELFT>::visit(SectionIndexSection &Sec) {}
 
 template <class ELFT>
 void ELFSectionSizer<ELFT>::visit(CompressedSection &Sec) {}
 
 template <class ELFT>
Error Section::removeSectionReferences(
   return Error::success();
 }
 
 void GroupSection::finalize() {
   this->Info = Sym->Index;
   this->Link = SymTab->Index;
 }
 
+Error GroupSection::removeSectionReferences(
+    bool AllowBrokenLinks, function_ref<bool(const SectionBase *)> ToRemove) {
+  llvm::erase_if(GroupMembers, ToRemove);
+  return Error::success();
+}
+
 Error GroupSection::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
   if (ToRemove(*Sym))
     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 Error::success();
 }