[llvm-objcopy] - Implement replaceSectionReferences for GroupSection class.

[grimar]

Summary

Currently, llvm-objcopy incorrectly handles compression and decompression of the
sections from COMDAT groups, because we do not implement the
replaceSectionReferences for this type of the sections.

The patch does that.

[grimar]

• Minor cosmetic changes.

[jhenderson]

Looks good apart from a few comments.

test/tools/llvm-objcopy/ELF/compress-debug-sections-groups.test

5 ↗

(On Diff #191671)

tool -> llvm-objcopy

(let's be explicit)

6 ↗

(On Diff #191671)

section -> sections

8 ↗

(On Diff #191671)

I'd tweak this and the below comments to the following, as it sounds more natural:

"Check compression of debug sections." (or decompression)

[jhenderson]

Hmm, I suppose for strict correctness, you should also check if the associated symbol table needs updating too.

[grimar]

In D59638#1437787, @jhenderson wrote:

Hmm, I suppose for strict correctness, you should also check if the associated symbol table needs updating too.

Could you elaborate please what you have in mind? We currently already update all the symbols in the symbol table.

Spec says (https://docs.oracle.com/cd/E19683-01/816-1386/chapter7-26/index.html):
"The name of a symbol from one of the containing object's symbol tables provides a signature for the section group."
In the test, I check that the group signature is still groupname both after compression and decompression,
i.e. I think I already check that nothing is broken here. Am I missing something?

[grimar]

• Addressed review comments.

[jhenderson]

In D59638#1437838, @grimar wrote:

In D59638#1437787, @jhenderson wrote:

Hmm, I suppose for strict correctness, you should also check if the associated symbol table needs updating too.

Could you elaborate please what you have in mind? We currently already update all the symbols in the symbol table.

Spec says (https://docs.oracle.com/cd/E19683-01/816-1386/chapter7-26/index.html):
"The name of a symbol from one of the containing object's symbol tables provides a signature for the section group."
In the test, I check that the group signature is still groupname both after compression and decompression,
i.e. I think I already check that nothing is broken here. Am I missing something?

Yes, the symbols are correctly updated, and it's not possible to break the behaviour with the current switches available in llvm-objcopy, I think, but theoretically, there's nothing stopping us replacing the symbol table itself. The GroupSection class has a pointer to a SymbolTableSection, which will point at the wrong section if that symbol table ever gets replaced. In other words, I think you should probably add this to the replaceSectionReferences definition for strict correctness:

if (SectionBase *To = FromTo.lookup(SymTab))
  Symtab = To;

On the other hand, symbol table replacing will cause other issues, so if you don't want to do it, I guess that's okay for now?

[grimar]

In D59638#1437873, @jhenderson wrote:

In D59638#1437838, @grimar wrote:

In D59638#1437787, @jhenderson wrote:

Hmm, I suppose for strict correctness, you should also check if the associated symbol table needs updating too.

Could you elaborate please what you have in mind? We currently already update all the symbols in the symbol table.

Spec says (https://docs.oracle.com/cd/E19683-01/816-1386/chapter7-26/index.html):
"The name of a symbol from one of the containing object's symbol tables provides a signature for the section group."
In the test, I check that the group signature is still groupname both after compression and decompression,
i.e. I think I already check that nothing is broken here. Am I missing something?

Yes, the symbols are correctly updated, and it's not possible to break the behaviour with the current switches available in llvm-objcopy, I think, but theoretically, there's nothing stopping us replacing the symbol table itself. The GroupSection class has a pointer to a SymbolTableSection, which will point at the wrong section if that symbol table ever gets replaced. In other words, I think you should probably add this to the replaceSectionReferences definition for strict correctness:

if (SectionBase *To = FromTo.lookup(SymTab))
  Symtab = To;

On the other hand, symbol table replacing will cause other issues, so if you don't want to do it, I guess that's okay for now?

I see. Symbol table replacement would definitely require much more changes everywhere I think.
So, yes, I would prefer not to add a code for this a bit unrealistic (atm) case.

[jhenderson]

Okay, LGTM. Maybe worth waiting a day to let others comment.

[rupprecht]

lgtm

llvm/trunk/test/tools/llvm-objcopy/ELF/Inputs/compress-debug-sections.yaml

Relocations:
         Type:            R_X86_64_32
       - Offset:          0x2
         Symbol:          .notdebug_foo
         Type:            R_X86_64_32
 ## This section should not be involved in relocations.
   - Name:            .debug_bar
     Type:            SHT_PROGBITS
     Content:         0000000000000000
+  - Name:            .group
+    Type:            SHT_GROUP
+    Link:            .symtab
+    EntSize:         0x0000000000000004
+    Info:            groupname
+    Members:         
+      - SectionOrType:   GRP_COMDAT
+      - SectionOrType:   .text.in.group
+      - SectionOrType:   .debug_in_group
+  - Name:            .text.in.group
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR, SHF_GROUP ]
+    Content:         '00'
+  - Name:            .debug_in_group
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_GROUP ]
+    Content:         '00'
 Symbols:
   Global:
     - Name:    .debug_foo
       Type:    STT_SECTION
       Section: .debug_foo
     - Name:    .notdebug_foo
       Type:    STT_SECTION
       Section: .notdebug_foo
     - Name:    .Linfo_string0
       Section: .debug_bar
+    - Name:    groupname
+      Section: .group
 ...

llvm/trunk/test/tools/llvm-objcopy/ELF/compress-debug-sections-groups.test

+# REQUIRES: zlib
+
+## In this test, we check how llvm-objcopy handles compression/decompression
+## of debug sections included in a COMDAT group.
+
+# RUN: yaml2obj %p/Inputs/compress-debug-sections.yaml -o %t.o
+
+## Check compression of debug sections.
+# RUN: llvm-objcopy --compress-debug-sections %t.o %t-compressed.o
+# RUN: llvm-readobj -s -elf-section-groups %t-compressed.o | \
+# RUN:   FileCheck %s --check-prefixes=CHECK,COMPRESS
+
+## Check zlib-gnu compression of debug sections.
+# RUN: llvm-objcopy --compress-debug-sections=zlib-gnu %t.o %t-compressed-gnu.o
+# RUN: llvm-readobj -s -elf-section-groups %t-compressed-gnu.o | \
+# RUN:   FileCheck %s --check-prefixes=CHECK,COMPRESSZLIB
+
+## Check decompression of debug sections.
+# RUN: llvm-objcopy --decompress-debug-sections %t-compressed.o %t-decompressed.o
+# RUN: llvm-readobj -elf-section-groups %t-decompressed.o | \
+# RUN:   FileCheck %s --check-prefixes=CHECK,DECOMPRESS
+
+## Check decompression of zlib-gnu debug sections.
+# RUN: llvm-objcopy --decompress-debug-sections %t-compressed-gnu.o %t-decompressed-gnu.o
+# RUN: llvm-readobj -elf-section-groups %t-decompressed-gnu.o | \
+# RUN:   FileCheck %s --check-prefixes=CHECK,DECOMPRESS
+
+# COMPRESSZLIB:      Name: .zdebug_in_group
+# COMPRESSZLIB-NEXT: Type: SHT_PROGBITS
+# COMPRESSZLIB-NEXT: Flags [
+# COMPRESSZLIB-NEXT:   SHF_GROUP
+# COMPRESSZLIB-NEXT: ]
+
+# COMPRESS:      Name: .debug_in_group
+# COMPRESS-NEXT: Type: SHT_PROGBITS
+# COMPRESS-NEXT: Flags [
+# COMPRESS-NEXT:   SHF_COMPRESSED
+# COMPRESS-NEXT:   SHF_GROUP
+# COMPRESS-NEXT: ]
+
+# CHECK:           Group {
+# CHECK-NEXT:        Name: .group
+# CHECK-NEXT:        Index:
+# CHECK-NEXT:        Link:
+# CHECK-NEXT:        Info:
+# CHECK-NEXT:        Type: COMDAT
+# CHECK-NEXT:        Signature: groupname
+# CHECK-NEXT:        Section(s) in group [
+# CHECK-NEXT:          .text.in.group
+# COMPRESSZLIB-NEXT:   .zdebug_in_group
+# COMPRESS-NEXT:       .debug_in_group
+# DECOMPRESS-NEXT:     .debug_in_group
+# CHECK-NEXT:        ]

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

public:
   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 removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
   void markSymbols() override;
+  void replaceSectionReferences(
+      const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
 
   static bool classof(const SectionBase *S) {
     return S->Type == ELF::SHT_GROUP;
   }
 };
 
 class DynamicSymbolTableSection : public Section {
 public:

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

Error GroupSection::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
   return Error::success();
 }
 
 void GroupSection::markSymbols() {
   if (Sym)
     Sym->Referenced = true;
 }
 
+void GroupSection::replaceSectionReferences(
+    const DenseMap<SectionBase *, SectionBase *> &FromTo) {
+  for (SectionBase *&Sec : GroupMembers)
+    if (SectionBase *To = FromTo.lookup(Sec))
+      Sec = To;
+}
+
 void Section::initialize(SectionTableRef SecTable) {
   if (Link != ELF::SHN_UNDEF) {
     LinkSection =
         SecTable.getSection(Link, "Link field value " + Twine(Link) +
                                       " in section " + Name + " is invalid");
     if (LinkSection->Type == ELF::SHT_SYMTAB)
       LinkSection = nullptr;
   }