[PATCH] D33485: [ELF] - Do not allow -r to eat comdats.

[Rafael Avila de Espindola via llvm-commits]

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> Index: test/ELF/relocatable-comdat.s
> ===================================================================
> --- test/ELF/relocatable-comdat.s
> +++ test/ELF/relocatable-comdat.s
> @@ -0,0 +1,42 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
> +# RUN: ld.lld -r %t.o -o %t

Pass %t.o twice to the link to make the test more interesting.

> +# RUN: llvm-readobj -elf-section-groups -sections %t | FileCheck %s
> +
> +# CHECK:      Section {
> +# CHECK:        Index: 3
> +# CHECK:        Name: .text.bar

If you are checking the index number you can use CHECK-NEXT: for the
name, but why do you need to check the number?

> +# CHECK-NEXT:   Type: SHT_PROGBITS
> +# CHECK-NEXT:   Flags [
> +# CHECK-NEXT:     SHF_ALLOC
> +# CHECK-NEXT:     SHF_EXECINSTR
> +# CHECK-NEXT:     SHF_GROUP
> +# CHECK-NEXT:   ]

Please check the size of the sections.

> +# CHECK:      Section {
> +# CHECK-NEXT:   Index: 4
> +# CHECK-NEXT:   Name: .text.foo
> +# CHECK-NEXT:   Type: SHT_PROGBITS
> +# CHECK-NEXT:   Flags [
> +# CHECK-NEXT:     SHF_ALLOC
> +# CHECK-NEXT:     SHF_EXECINSTR
> +# CHECK-NEXT:     SHF_GROUP
> +# CHECK-NEXT:   ]
> +# CHECK:      Section {
> +# CHECK-NEXT:   Index: 5
> +# CHECK-NEXT:   Name: .symtab
> +
> +# CHECK:       Groups {
> +# CHECK-NEXT:    Group {
> +# CHECK-NEXT:      Name: .group
> +# CHECK-NEXT:      Index: 2
> +# CHECK-NEXT:      Type: COMDAT
> +# CHECK-NEXT:      Signature: abc
> +# CHECK-NEXT:      Section(s) in group [
> +# CHECK-NEXT:        .text.bar
> +# CHECK-NEXT:        .text.foo
> +# CHECK-NEXT:      ]
> +# CHECK-NEXT:    }
> +# CHECK-NEXT:  }
> +
> +.section .text.bar,"axG", at progbits,abc,comdat
> +.section .text.foo,"axG", at progbits,abc,comdat

Please add some simple content to the sections so that they are not
empty and have different sizes (.quad 42 and .long 42 for example).

>  static uint64_t getOutFlags(InputSectionBase *S) {
> +  if (Config->Relocatable)
> +    return S->Flags & ~SHF_COMPRESSED;

Not really related to the patch, but do we decompress in -r? Should we?
Would you mind checking and opening a bug if needed?

LGTM with a stronger test.

Thanks,
Rafael