[llvm-objcopy] Fix SHT_GROUP ordering.

[rupprecht]

Summary

When llvm-objcopy sorts sections during finalization, it only sorts based on the offset, which can cause the group section to come after the sections it contains. This causes link failures when using gold to link objects created by llvm-objcopy.

Fix this for now by copying GNU objcopy's behavior of placing SHT_GROUP sections first. In the future, we may want to remove this sorting entirely to more closely preserve the input file layout.

This fixes https://bugs.llvm.org/show_bug.cgi?id=42052.

[MaskRay]

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

1640 ↗

(On Diff #202020)

@jakehehrlich I can't find a use site of objcopy::elf::Object::Sections that requires it to be sorted.

I guess we can probably just delete this function and fix the 5 or 6 tests that check the section order.

[MaskRay]

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

1643 ↗

(On Diff #202020)

This also matches what GNU objcopy does.

This is not accurate.. I suspect it just doesn't sort sections, unless in --pad-to= or --gap-fill= mode.

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/group-reorder.test

1 ↗

(On Diff #202020)

Regarding this test, I'm not sure if llvm-objcopy should repair the object file. It seems currently the only broken tool is us :( After this issue is fixed, the repair functionality may be use useful any more...

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

1643 ↗

(On Diff #202020)

Sorry, GNU objcopy does move SHT_GROUP sections before.

bfd/elf.c:assign_section_numbers does this. It has a comment:

/* Put SHT_GROUP sections first. */

[jakehehrlich]

Can we just sort in the layout code instead of sorting everything? Then we don't have to sort at all. I think we've largely been good about making operations stable so order should be maintained if just don't sort.

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

1640 ↗

(On Diff #202020)

The layout algorithm assumes that things are sorted by offset so we can't just delete this but we can sort a copy there instead.

[jakehehrlich]

Alternatively we can sort by Index to allow ourselves to make mistakes in maintaining the order along the way but I think I'd rather not do that since that's just a hack.

[jakehehrlich]

Alternatively we could only sort program headers in the layout algorithm and then just layout sections in whatever order we please. It will just mean that our layout won't mirror the input but it will be valid.

[rupprecht]

llvm/test/tools/llvm-objcopy/ELF/group-reorder.test

1 ↗

(On Diff #202020)

Mind clarify what's broken in the object file? The input here is valid -- .group contains .bar, and comes before it -- but the test shows that .group moves to index 1.

It may also repair a broken object file, but I didn't test if it does that. But this should fix a bug by not *creating* a broken object file.

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

1640 ↗

(On Diff #202020)

I don't understand this either. I need to study it in more detail, but Object::layoutSections iterates over sections (in the order as sorted here) and assigns offsets based on what the aligned value of (previous offset + previous size) is... that doesn't require them to be sorted, AFAICT.

1643 ↗

(On Diff #202020)

Correct; sections are rearranged (for SHT_GROUP) but not sorted.

[MaskRay]

I don't have more comments. This does make llvm-objcopy behave more like GNU objcopy and fixes the SHT_GROUP bug. Such object files are accepted by ld.bfd and lld as a natural extension, but gold takes another approach to process SHT_GROUP and would give an error.

I slightly prefer deleting Object::sortSections() (I can't find anywhere the sorted behavior is required by the layout algorithm) but the current change is also good.

llvm/test/tools/llvm-objcopy/ELF/group-reorder.test

1 ↗

(On Diff #202020)

Sorry you can ignore my previous comment. I made the comment before realizing sorting is the behavior of GNU objcopy.

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

 # REQUIRES: zlib
 
 # RUN: yaml2obj %p/Inputs/compress-debug-sections.yaml -o %t.o
 # RUN: llvm-objcopy --compress-debug-sections=zlib-gnu %t.o %t-compressed.o
 # RUN: llvm-objcopy --decompress-debug-sections %t-compressed.o %t-decompressed.o
 
 # RUN: llvm-objdump -s %t.o --section=.debug_foo | FileCheck %s
 # RUN: llvm-objdump -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-COMPRESSED
 # RUN: llvm-readobj --relocations -S %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
 # RUN: llvm-readobj --relocations -S %t-decompressed.o | FileCheck %s --check-prefix=CHECK-HEADER
 # RUN: llvm-readobj --relocations -S %t.o | FileCheck %s --check-prefix=CHECK-HEADER
 # RUN: llvm-objdump -s %t-decompressed.o --section=.debug_foo | FileCheck %s
 
 # CHECK: .debug_foo:
 # CHECK-NEXT: 0000 00000000 00000000
 
-# CHECK-HEADER: Index: 1
-# CHECK-HEADER-NEXT: Name: .debug_foo
+# CHECK-HEADER:      Name: .debug_foo
 # CHECK-HEADER-NEXT: Type: SHT_PROGBITS
 # CHECK-HEADER-NEXT: Flags [
 # CHECK-HEADER-NEXT: ]
 # CHECK-HEADER-NEXT: Address:
 # CHECK-HEADER-NEXT: Offset:
 # CHECK-HEADER-NEXT: Size: 8
 
 # CHECK-COMPRESSED: .zdebug_foo:
 # CHECK-COMPRESSED: ZLIB
 # CHECK-COMPRESSED: .notdebug_foo:
 
-# CHECK-FLAGS-NOT: Name: .debug_foo
-# CHECK-FLAGS: Index: 1
-# CHECK-FLAGS-NEXT: Name: .zdebug_foo
+# CHECK-FLAGS-NOT:  Name: .debug_foo
+# CHECK-FLAGS:      Name: .zdebug_foo
 # CHECK-FLAGS-NEXT: Type: SHT_PROGBITS
 # CHECK-FLAGS-NEXT: Flags [
 # CHECK-FLAGS-NEXT: ]
 # CHECK-FLAGS-NEXT: Address:
 # CHECK-FLAGS-NEXT: Offset:
 # CHECK-FLAGS-NEXT: Size: 23
 
 # CHECK-FLAGS: Name: .notdebug_foo
 # CHECK-FLAGS-NEXT: Type: SHT_PROGBITS
 # CHECK-FLAGS-NEXT: Flags [
 # CHECK-FLAGS-NEXT: ]
 # CHECK-FLAGS-NEXT: Address:
 # CHECK-FLAGS-NEXT: Offset:
 # CHECK-FLAGS-NEXT: Size: 8
 
 # CHECK-FLAGS: Name: .rela.debug_foo
 # CHECK-FLAGS-NEXT: Type: SHT_RELA
 # CHECK-FLAGS-NEXT: Flags [
 # CHECK-FLAGS-NEXT: ]
-# CHECK-FLAGS-NEXT: Address:
-# CHECK-FLAGS-NEXT: Offset:
-# CHECK-FLAGS-NEXT: Size:
-# CHECK-FLAGS-NEXT: Link:
-# CHECK-FLAGS-NEXT: Info: 1
 
 # CHECK-FLAGS: Relocations [
 # CHECK-FLAGS-NEXT:   .rela.debug_foo {
 # CHECK-FLAGS-NEXT:     0x1 R_X86_64_32 .zdebug_foo 0x0
 # CHECK-FLAGS-NEXT:     0x2 R_X86_64_32 .notdebug_foo 0x0
 # CHECK-FLAGS-NEXT:   }
 # CHECK-FLAGS-NEXT: ]
 

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

 # REQUIRES: zlib
 
 # RUN: yaml2obj %p/Inputs/compress-debug-sections.yaml -o %t.o
 # RUN: llvm-objcopy --compress-debug-sections=zlib %t.o %t-compressed.o
 # RUN: llvm-objcopy --decompress-debug-sections %t-compressed.o %t-decompressed.o
 
 # RUN: llvm-objdump -s %t.o --section=.debug_foo | FileCheck %s
 # RUN: llvm-objdump -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-COMPRESSED
 # RUN: llvm-readobj --relocations -S %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
 # RUN: llvm-readobj --relocations -S %t-decompressed.o | FileCheck %s --check-prefix=CHECK-HEADER
 # RUN: llvm-readobj --relocations -S %t.o | FileCheck %s --check-prefix=CHECK-HEADER
 # RUN: llvm-objdump -s %t-decompressed.o --section=.debug_foo | FileCheck %s
 
 # CHECK: .debug_foo:
 # CHECK-NEXT: 0000 00000000 00000000
 
-# CHECK-HEADER: Index: 1
-# CHECK-HEADER-NEXT: Name: .debug_foo
+# CHECK-HEADER:      Name: .debug_foo
 # CHECK-HEADER-NEXT: Type: SHT_PROGBITS
 # CHECK-HEADER-NEXT: Flags [
 # CHECK-HEADER-NEXT: ]
 # CHECK-HEADER-NEXT: Address:
 # CHECK-HEADER-NEXT: Offset:
 # CHECK-HEADER-NEXT: Size: 8
 
 # CHECK-COMPRESSED: .debug_foo:
 # CHECK-COMPRESSED: .notdebug_foo:
 
-# CHECK-FLAGS: Index: 1
-# CHECK-FLAGS-NEXT: Name: .debug_foo
+# CHECK-FLAGS:      Name: .debug_foo
 # CHECK-FLAGS-NEXT: Type: SHT_PROGBITS
 # CHECK-FLAGS-NEXT: Flags [
 # CHECK-FLAGS-NEXT: SHF_COMPRESSED
 # CHECK-FLAGS-NEXT: ]
 # CHECK-FLAGS-NEXT: Address:
 # CHECK-FLAGS-NEXT: Offset:
 # CHECK-FLAGS-NEXT: Size: 35
 # CHECK-FLAGS-NOT: Name: .debug_foo

 # CHECK-FLAGS: Name: .rela.debug_foo
 # CHECK-FLAGS-NEXT: Type: SHT_RELA
 # CHECK-FLAGS-NEXT: Flags [
 # CHECK-FLAGS-NEXT: ]
 # CHECK-FLAGS-NEXT: Address:
 # CHECK-FLAGS-NEXT: Offset:
 # CHECK-FLAGS-NEXT: Size:
 # CHECK-FLAGS-NEXT: Link:
-# CHECK-FLAGS-NEXT: Info: 1
 
 # CHECK-FLAGS: Relocations [
 # CHECK-FLAGS-NEXT:   .rela.debug_foo {
 # CHECK-FLAGS-NEXT:     0x1 R_X86_64_32 .debug_foo 0x0
 # CHECK-FLAGS-NEXT:     0x2 R_X86_64_32 .notdebug_foo 0x0
 # CHECK-FLAGS-NEXT:   }
 # CHECK-FLAGS-NEXT: ]

llvm/trunk/test/tools/llvm-objcopy/ELF/group-reorder.test

+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-objcopy %t.o %t.2.o
+# RUN: llvm-readelf --elf-section-groups --sections %t.o | FileCheck %s --check-prefix=IN
+# RUN: llvm-readelf --elf-section-groups --sections %t.2.o | FileCheck %s --check-prefix=OUT
+
+# In this test, .group gets moved to the beginning. Run readelf -gS on input as
+# well as output to make sure it really moved, as well as to verify that we
+# aren't purely sorting based on offsets (it gets moved to the beginning
+# despite having a larger offset).
+
+# IN:      There are 7 section headers, starting at offset 0x40:
+# IN:        [Nr] Name              Type            Address          Off    Size
+# IN-NEXT:   [ 0]                   NULL            0000000000000000 000000 000000
+# IN-NEXT:   [ 1] .foo              PROGBITS        0000000000000000 000200 000040
+# IN-NEXT:   [ 2] .group            GROUP           0000000000000000 000240 000008
+# IN-NEXT:   [ 3] .bar              PROGBITS        0000000000000000 000248 000040
+
+# IN:      COMDAT group section [    2] `.group' [bar] contains 1 sections:
+# IN-NEXT:    [Index]    Name
+# IN-NEXT:    [    3]   .bar
+
+# OUT:      There are 7 section headers, starting at offset 0x160:
+# OUT:        [Nr] Name              Type            Address          Off    Size
+# OUT-NEXT:   [ 0]                   NULL            0000000000000000 000000 000000
+# OUT-NEXT:   [ 1] .group            GROUP           0000000000000000 000040 000008
+# OUT-NEXT:   [ 2] .foo              PROGBITS        0000000000000000 000048 000040
+# OUT-NEXT:   [ 3] .bar              PROGBITS        0000000000000000 000088 000040
+
+# OUT:      COMDAT group section [    1] `.group' [bar] contains 1 sections:
+# OUT-NEXT:    [Index]    Name
+# OUT-NEXT:    [    3]   .bar
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .foo
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Size:            64
+  - Name:            .group
+    Type:            SHT_GROUP
+    Link:            .symtab
+    Info:            bar
+    Members:
+      - SectionOrType:   GRP_COMDAT
+      - SectionOrType:   .bar
+  - Name:            .bar
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR, SHF_GROUP ]
+    Size:            64
+Symbols:
+  - Name:            .foo
+    Type:            STT_SECTION
+    Section:         .foo
+  - Name:            .bar
+    Type:            STT_SECTION
+    Section:         .bar
+  - Name:            bar
+    Type:            STT_FUNC
+    Section:         .foo

llvm/trunk/test/tools/llvm-objcopy/ELF/strip-dwo-groups.test

 // the symbol table, the indices of the symbols and the indices of the sections 
 // which go after the removed ones will change. Consequently, the fields 
 // Link, Info and the content of .group need to be updated. In the past 
 // `llvm-objcopy --strip-dwo` used to produce invalid binaries with 
 // broken .group section, this test verifies the correctness of 
 // Link, Info and the content of this section.
 
 CHECK:          Name: .group (179)
-CHECK-NEXT:     Index: 17
+CHECK-NEXT:     Index: 1{{$}}
 CHECK-NEXT:     Link: 19
 CHECK-NEXT:     Info: 14
-CHECK:          .text._ZN1SIiE4getXEv (2)
+CHECK:          .text._ZN1SIiE4getXEv
 
 CHECK:          Name: .group (179)
-CHECK-NEXT:     Index: 18
+CHECK-NEXT:     Index: 2{{$}}
 CHECK-NEXT:     Link: 19
 CHECK-NEXT:     Info: 13
-CHECK:          .text._ZN1SIdE4getXEv (4)
-CHECK-NEXT:     .rela.text._ZN1SIdE4getXEv (21)
+CHECK:          .text._ZN1SIdE4getXEv
+CHECK-NEXT:     .rela.text._ZN1SIdE4getXEv

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

Error Object::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
   if (SymbolTable)
     for (const SecPtr &Sec : Sections)
       if (Error E = Sec->removeSymbols(ToRemove))
         return E;
   return Error::success();
 }
 
 void Object::sortSections() {
-  // Put all sections in offset order. Maintain the ordering as closely as
-  // possible while meeting that demand however.
+  // Use stable_sort to maintain the original ordering as closely as possible.
   llvm::stable_sort(Sections, [](const SecPtr &A, const SecPtr &B) {
+    // Put SHT_GROUP sections first, since group section headers must come
+    // before the sections they contain. This also matches what GNU objcopy
+    // does.
+    if (A->Type != B->Type &&
+        (A->Type == ELF::SHT_GROUP || B->Type == ELF::SHT_GROUP))
+      return A->Type == ELF::SHT_GROUP;
+    // For all other sections, sort by offset order.
     return A->OriginalOffset < B->OriginalOffset;
   });
 }
 
 static uint64_t alignToAddr(uint64_t Offset, uint64_t Addr, uint64_t Align) {
   // Calculate Diff such that (Offset + Diff) & -Align == Addr & -Align.
   if (Align == 0)
     Align = 1;