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

[Rafael Avila de Espindola via llvm-commits]

LGTM

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

> grimar updated this revision to Diff 100217.
> grimar added a comment.
>
> - Addressed review comments.
>
>
> https://reviews.llvm.org/D33485
>
> Files:
>   ELF/InputFiles.cpp
>   ELF/InputSection.cpp
>   ELF/InputSection.h
>   ELF/OutputSections.cpp
>   test/ELF/relocatable-comdat.s
>
> Index: test/ELF/relocatable-comdat.s
> ===================================================================
> --- test/ELF/relocatable-comdat.s
> +++ test/ELF/relocatable-comdat.s
> @@ -0,0 +1,45 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
> +# RUN: ld.lld -r %t.o %t.o -o %t
> +# RUN: llvm-readobj -elf-section-groups -sections %t | FileCheck %s
> +
> +# CHECK:        Name: .text.bar
> +# CHECK-NEXT:   Type: SHT_PROGBITS
> +# CHECK-NEXT:   Flags [
> +# CHECK-NEXT:     SHF_ALLOC
> +# CHECK-NEXT:     SHF_EXECINSTR
> +# CHECK-NEXT:     SHF_GROUP
> +# CHECK-NEXT:   ]
> +# CHECK-NEXT:   Address:
> +# CHECK-NEXT:   Offset:
> +# CHECK-NEXT:   Size: 8
> +# 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-NEXT:   Address:
> +# CHECK-NEXT:   Offset:
> +# CHECK-NEXT:   Size: 4
> +
> +# 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
> +.quad 42
> +.section .text.foo,"axG", at progbits,abc,comdat
> +.long 42
> Index: ELF/OutputSections.cpp
> ===================================================================
> --- ELF/OutputSections.cpp
> +++ ELF/OutputSections.cpp
> @@ -126,6 +126,20 @@
>    }
>  
>    uint32_t Type = this->Type;
> +  if (Type == SHT_GROUP) {
> +    // sh_link field for SHT_GROUP sections should contain the section index of
> +    // the symbol table.
> +    this->Link = InX::SymTab->OutSec->SectionIndex;
> +
> +    // sh_link then contain index of an entry in symbol table section which
> +    // provides signature of the section group.
> +    InputSection *First = Sections[0];
> +    elf::ObjectFile<ELFT> *Obj = First->getFile<ELFT>();
> +    ArrayRef<SymbolBody *> Symbols = Obj->getSymbols();
> +    this->Info = InX::SymTab->getSymbolIndex(Symbols[First->Info - 1]);
> +    return;
> +  }
> +
>    if (!Config->CopyRelocs || (Type != SHT_RELA && Type != SHT_REL))
>      return;
>  
> @@ -260,6 +274,8 @@
>  }
>  
>  static uint64_t getOutFlags(InputSectionBase *S) {
> +  if (Config->Relocatable)
> +    return S->Flags & ~SHF_COMPRESSED;
>    return S->Flags & ~SHF_GROUP & ~SHF_COMPRESSED;
>  }
>  
> Index: ELF/InputSection.h
> ===================================================================
> --- ELF/InputSection.h
> +++ ELF/InputSection.h
> @@ -319,6 +319,9 @@
>  private:
>    template <class ELFT, class RelTy>
>    void copyRelocations(uint8_t *Buf, llvm::ArrayRef<RelTy> Rels);
> +
> +  template <class ELFT, class EntTy>
> +  void copyGroup(uint8_t *Buf, llvm::ArrayRef<EntTy> Ents);
>  };
>  
>  // The list of all input sections.
> Index: ELF/InputSection.cpp
> ===================================================================
> --- ELF/InputSection.cpp
> +++ ELF/InputSection.cpp
> @@ -294,6 +294,21 @@
>    return S->kind() != Output;
>  }
>  
> +template <class ELFT, class EntTy>
> +void InputSection::copyGroup(uint8_t *Buf, llvm::ArrayRef<EntTy> Ents) {
> +  assert(this->Type == SHT_GROUP);
> +
> +  // First entry is a flag word, we leave it unchanged.
> +  auto *P = reinterpret_cast<typename ELFT::Word *>(Buf);
> +  *P++ = Ents[0];
> +
> +  // Here we adjust indices of sections that belong to group as it
> +  // might change during linking.
> +  ArrayRef<InputSectionBase *> Sections = this->File->getSections();
> +  for (const EntTy &Ent : Ents.slice(1))
> +    *P++ = Sections[Ent]->OutSec->SectionIndex;
> +}
> +
>  InputSectionBase *InputSection::getRelocatedSection() {
>    assert(this->Type == SHT_RELA || this->Type == SHT_REL);
>    ArrayRef<InputSectionBase *> Sections = this->File->getSections();
> @@ -679,6 +694,14 @@
>      return;
>    }
>  
> +  // If -r is given, linker should keep SHT_GROUP sections. We should fixup
> +  // them, see copyGroup().
> +  if (this->Type == SHT_GROUP) {
> +    copyGroup<ELFT>(Buf + OutSecOff,
> +                    this->template getDataAs<typename ELFT::Word>());
> +    return;
> +  }
> +
>    // Copy section contents from source object file to output file
>    // and then apply relocations.
>    memcpy(Buf + OutSecOff, Data.data(), Data.size());
> Index: ELF/InputFiles.cpp
> ===================================================================
> --- ELF/InputFiles.cpp
> +++ ELF/InputFiles.cpp
> @@ -303,17 +303,23 @@
>      }
>  
>      switch (Sec.sh_type) {
> -    case SHT_GROUP:
> -      this->Sections[I] = &InputSection::Discarded;
> -      if (ComdatGroups.insert(getShtGroupSignature(ObjSections, Sec)).second)
> +    case SHT_GROUP: {
> +      bool New =
> +          ComdatGroups.insert(getShtGroupSignature(ObjSections, Sec)).second;
> +      if (New && Config->Relocatable)
> +        this->Sections[I] = createInputSection(Sec, SectionStringTable);
> +      else
> +        this->Sections[I] = &InputSection::Discarded;
> +      if (New)
>          continue;
> +
>        for (uint32_t SecIndex : getShtGroupEntries(Sec)) {
>          if (SecIndex >= Size)
>            fatal(toString(this) +
>                  ": invalid section index in group: " + Twine(SecIndex));
>          this->Sections[SecIndex] = &InputSection::Discarded;
>        }
> -      break;
> +    } break;
>      case SHT_SYMTAB:
>        this->initSymtab(ObjSections, &Sec);
>        break;