[ELF] Simpler scheme for handling common symbols

[bd1976llvm]

Summary

Convert all common symbols to regular symbols after scan.

This means that the downstream code does not to handle common symbols as a special case.

[ruiu]

First of all, could you read https://llvm.org/docs/CodingStandards.html and follow that style? One of the easiest way is to run clang-format-diff on your patch so that everything in your patch is formatted automatically.

ELF/Driver.cpp

1088

Remove lld::elf::. In general, please look around and make your new code looks like the same in style as other code.

ELF/MarkLive.cpp

66–67

Just remove the code.

225–226

Remove

ELF/SyntheticSections.cpp

63–64

Remove Pos and assert.

63–64

create -> Create

It's not a fake section. We call it a synthetic section.

Please add a full stop at end.

64

auto * -> auto *

64–75

We do not use auto unless its actual type is obvious from RHS.

66

change -> Change

Add a full stop.

[bd1976llvm]

In D38137#877803, @ruiu wrote:

First of all, could you read https://llvm.org/docs/CodingStandards.html and follow that style? One of the easiest way is to run clang-format-diff on your patch so that everything in your patch is formatted automatically.

What a mess! Thanks for reviewing the patch despite how unstylish it is.
I agree with all the points you raised - will update the patch.

[bd1976llvm]

Updated the diff to address review comments.

[ruiu]

ELF/Driver.cpp

1084–1086

Probably it is better to describe what this does, instead of explaining post-conditions.

// Create a .bss section for each common symbol and then replace the common symbol with a DefinedRegular symbol. As a result, all common symbols are "instantiated" as regular defined symbols, so that we don't need to care about common symbols beyond this point. Note that if -r is given, we just need to pass through common symbols as-is.

ELF/Symbols.cpp

103

Messages for llvm_unreachable don't usually start with "unreachable: ", so remove it.

ELF/SyntheticSections.cpp

72

I'd also describe the motivation of doing this.

// Replace all DefinedCommon symbols with DefinedRegular symbols so that we don't have to care about DefinedCommon symbols beyond this point.

ELF/SyntheticSections.h

771

I think I'd keep the original function name because the function still creates common sections, in addition to replace DefinedCommon symbols with DefinedRegular symbols. It looks that the function creates common sections is more important than replacing symbols.

[bd1976llvm]

Updated as per review comments.
Apologies that it took some time to update the patch.

[ruiu]

ELF/SyntheticSections.cpp

57

Please add a comment:

Create a .bss section for each common section and replace the common symbol with a DefinedRegular symbol.

64–76

It is a bit more readable create an instance and then once it's done add it to InputSections.

auto *Section = make<BssSection>("COMMON");
Section->File = Sym->getFile();
Section->Live = !Config->GcSections;
Section->reserveSpace(Sym->Size, Sym->Alignment);
InputSections.push_back(Section);

74

You are passing Section to replaceBody, but do you still need this?

75

Do you need B? You already have Sym which is an instance of a subclass of SymbolBody.

76

== 1 is odd; just like we don't do if (<some boolean expression> == 1), I'd omit it.

78

0u -> 0 (0 is 0 however it is sign-extended.)

Do you actually need this comment?

[bd1976llvm]

updated with review comments.

[ruiu]

ELF/Symbols.cpp

282

I don't think this is how clang-format formats.

ELF/SyntheticSections.cpp

73

80 col.

[ruiu]

ELF/MarkLive.cpp

224–225

Since you have removed the following code, you can remove this return.

auto MarkSymbol = [&](SymbolBody *Sym) {
  if (auto *D = dyn_cast_or_null<DefinedRegular>(Sym))
    if (auto *Sec = cast_or_null<InputSectionBase>(D->Section))
      Enqueue(Sec, D->Value);
};

[bd1976llvm]

Addressed review comments.

[bd1976llvm]

Missed a review comment. Now addressed.

[bd1976llvm]

ELF/Symbols.cpp

282

I tried both the Windows and the Linux versions of the latest clang format. In both cases I got the same formatting. Am I doing something wrong? What do you expect the formatting to be here?

[ruiu]

LGTM. Thanks!

ELF/MarkLive.cpp

221

nit: remove {}

ELF/Symbols.cpp

282

Oh sorry, then that's fine.

ELF/Driver.cpp

for (InputSectionBase *S : F->getSections())
       InputSections.push_back(cast<InputSection>(S));
 
   // This adds a .comment section containing a version string. We have to add it
   // before decompressAndMergeSections because the .comment section is a
   // mergeable section.
   if (!Config->Relocatable)
     InputSections.push_back(createCommentSection<ELFT>());
 
+  // Create a .bss section for each common symbol and then replace the common
+  // symbol with a DefinedRegular symbol. As a result, all common symbols are
+  // "instantiated" as regular defined symbols, so that we don't need to care
+  // about common symbols beyond this point. Note that if -r is given, we just
+  // need to pass through common symbols as-is.
+  if (Config->DefineCommon)
+    createCommonSections<ELFT>();
+
   // Do size optimizations: garbage collection, merging of SHF_MERGE sections
   // and identical code folding.
   if (Config->GcSections)
     markLive<ELFT>();
   decompressAndMergeSections();
   if (Config->ICF)
     doIcf<ELFT>();
 
   // Write the result to the file.
   writeResult<ELFT>();
 }

ELF/MarkLive.cpp

 // There are normally few input sections whose names are valid C
 // identifiers, so we just store a std::vector instead of a multimap.
 static DenseMap<StringRef, std::vector<InputSectionBase *>> CNamedSections;
 
 template <class ELFT, class RelT>
 static void resolveReloc(InputSectionBase &Sec, RelT &Rel,
                          std::function<void(InputSectionBase *, uint64_t)> Fn) {
   SymbolBody &B = Sec.getFile<ELFT>()->getRelocTargetSym(Rel);
 
-  if (auto *Sym = dyn_cast<DefinedCommon>(&B)) {
-    Sym->Live = true;
-    return;
-  }
-
   if (auto *D = dyn_cast<DefinedRegular>(&B)) {
     if (!D->Section)
       return;
     uint64_t Offset = D->Value;
     if (D->isSection())
       Offset += getAddend<ELFT>(Sec, Rel);
     Fn(cast<InputSectionBase>(D->Section), Offset);
     return;
   }
auto Enqueue = [&](InputSectionBase *Sec, uint64_t Offset) {
     Sec->Live = true;
 
     // Add input section to the queue.
     if (InputSection *S = dyn_cast<InputSection>(Sec))
       Q.push_back(S);
   };
 
   auto MarkSymbol = [&](SymbolBody *Sym) {
     if (auto *D = dyn_cast_or_null<DefinedRegular>(Sym)) {
       if (auto *IS = cast_or_null<InputSectionBase>(D->Section))
         Enqueue(IS, D->Value);
       return;
     }
-    if (auto *S = dyn_cast_or_null<DefinedCommon>(Sym))
-      S->Live = true;
   };
 
   // Add GC root symbols.
   MarkSymbol(Symtab->find(Config->Entry));
   MarkSymbol(Symtab->find(Config->Init));
   MarkSymbol(Symtab->find(Config->Fini));
   for (StringRef S : Config->Undefined)
     MarkSymbol(Symtab->find(S));
   for (StringRef S : Script->Opt.ReferencedSymbols)

ELF/Symbols.h

 public:
   DefinedCommon(StringRef N, uint64_t Size, uint32_t Alignment, uint8_t StOther,
                 uint8_t Type);
 
   static bool classof(const SymbolBody *S) {
     return S->kind() == SymbolBody::DefinedCommonKind;
   }
 
-  // True if this symbol is not GC'ed. Liveness is usually a notion of
-  // input sections and not of symbols, but since common symbols don't
-  // belong to any input section, their liveness is managed by this bit.
-  bool Live;
-
   // The maximum alignment we have seen for this symbol.
   uint32_t Alignment;
 
   // The output offset of this common symbol in the output bss.
   // Computed by the writer.
   uint64_t Size;
   BssSection *Section = nullptr;
 };

ELF/Symbols.cpp

case SymbolBody::DefinedRegularKind: {
     if (D.isTls() && !Config->Relocatable) {
       if (!Out::TlsPhdr)
         fatal(toString(D.getFile()) +
               " has an STT_TLS symbol but doesn't have an SHF_TLS section");
       return VA - Out::TlsPhdr->p_vaddr;
     }
     return VA;
   }
-  case SymbolBody::DefinedCommonKind: {
-    if (!Config->DefineCommon)
-      return 0;
-    auto DC = cast<DefinedCommon>(Body);
-    if (!DC.Live)
-      return 0;
-    return DC.Section->getParent()->Addr + DC.Section->OutSecOff;
-  }
+  case SymbolBody::DefinedCommonKind:
+    llvm_unreachable("common are converted to bss");
   case SymbolBody::SharedKind: {
     auto &SS = cast<SharedSymbol>(Body);
     if (SS.CopyRelSec)
       return SS.CopyRelSec->getParent()->Addr + SS.CopyRelSec->OutSecOff;
     if (SS.NeedsPltAddr)
       return Body.getPltVA();
     return 0;
   }

 
 Undefined::Undefined(StringRefZ Name, bool IsLocal, uint8_t StOther,
                      uint8_t Type)
     : SymbolBody(SymbolBody::UndefinedKind, Name, IsLocal, StOther, Type) {}
 
 DefinedCommon::DefinedCommon(StringRef Name, uint64_t Size, uint32_t Alignment,
                              uint8_t StOther, uint8_t Type)
     : Defined(SymbolBody::DefinedCommonKind, Name, /*IsLocal=*/false, StOther,
               Type),
-      Live(!Config->GcSections), Alignment(Alignment), Size(Size) {}
+      Alignment(Alignment), Size(Size) {}
 
 // If a shared symbol is referred via a copy relocation, its alignment
 // becomes part of the ABI. This function returns a symbol alignment.
 // Because symbols don't have alignment attributes, we need to infer that.
 template <class ELFT> uint32_t SharedSymbol::getAlignment() const {
   SharedFile<ELFT> *File = getFile<ELFT>();
   uint32_t SecAlign = File->getSection(getSym<ELFT>())->sh_addralign;
   uint64_t SymValue = getSym<ELFT>().st_value;

ELF/SyntheticSections.h

public:
   void writeTo(uint8_t *Buf) override;
   InputSection *getTargetInputSection() const;
 
 private:
   std::vector<const Thunk *> Thunks;
   size_t Size = 0;
 };
 
-std::vector<InputSection *> createCommonSections();
+template <class ELFT> void createCommonSections();
 InputSection *createInterpSection();
 template <class ELFT> MergeInputSection *createCommentSection();
 void decompressAndMergeSections();
 
 SymbolBody *addSyntheticLocal(StringRef Name, uint8_t Type, uint64_t Value,
                               uint64_t Size, InputSectionBase *Section);
 
 // Linker generated sections which can be used as inputs.

ELF/SyntheticSections.cpp

 using namespace lld::elf;
 
 uint64_t SyntheticSection::getVA() const {
   if (OutputSection *Sec = getParent())
     return Sec->Addr + OutSecOff;
   return 0;
 }
 
-std::vector<InputSection *> elf::createCommonSections() {
-  if (!Config->DefineCommon)
-    return {};
-
-  std::vector<InputSection *> Ret;
+template <class ELFT> void elf::createCommonSections() {
   for (Symbol *S : Symtab->getSymbols()) {
     auto *Sym = dyn_cast<DefinedCommon>(S->body());
-    if (!Sym || !Sym->Live)
+
+    if (!Sym)
       continue;
 
-    Sym->Section = make<BssSection>("COMMON");
-    size_t Pos = Sym->Section->reserveSpace(Sym->Size, Sym->Alignment);
-    assert(Pos == 0);
-    (void)Pos;
-    Sym->Section->File = Sym->getFile();
-    Ret.push_back(Sym->Section);
+    // Create a synthetic section for the common data.
+    auto *Section = make<BssSection>("COMMON");
+    Section->File = Sym->getFile();
+    Section->Live = !Config->GcSections;
+    Section->reserveSpace(Sym->Size, Sym->Alignment);
+    InputSections.push_back(Section);
+
+    // Replace all DefinedCommon symbols with DefinedRegular symbols so that we
+    // don't have to care about DefinedCommon symbols beyond this point.
+    replaceBody<DefinedRegular>(S, Sym->getFile(), Sym->getName(), (bool)Sym->IsLocal,
+                                Sym->StOther, Sym->Type, 0,
+                                Sym->getSize<ELFT>(), Section);
   }
-  return Ret;
 }
 
 // Returns an LLD version string.
 static ArrayRef<uint8_t> getVersion() {
   // Check LLD_VERSION first for ease of testing.
   // You can get consitent output by using the environment variable.
   // This is only for testing.
   StringRef S = getenv("LLD_VERSION");
   if (S.empty())
     S = Saver.save(Twine("Linker: ") + getLLDVersion());

 template GdbIndexSection *elf::createGdbIndex<ELF64LE>();
 template GdbIndexSection *elf::createGdbIndex<ELF64BE>();
 
 template void PltSection::addEntry<ELF32LE>(SymbolBody &Sym);
 template void PltSection::addEntry<ELF32BE>(SymbolBody &Sym);
 template void PltSection::addEntry<ELF64LE>(SymbolBody &Sym);
 template void PltSection::addEntry<ELF64BE>(SymbolBody &Sym);
 
+template void elf::createCommonSections<ELF32LE>();
+template void elf::createCommonSections<ELF32BE>();
+template void elf::createCommonSections<ELF64LE>();
+template void elf::createCommonSections<ELF64BE>();
+
 template MergeInputSection *elf::createCommentSection<ELF32LE>();
 template MergeInputSection *elf::createCommentSection<ELF32BE>();
 template MergeInputSection *elf::createCommentSection<ELF64LE>();
 template MergeInputSection *elf::createCommentSection<ELF64BE>();
 
 template class elf::MipsAbiFlagsSection<ELF32LE>;
 template class elf::MipsAbiFlagsSection<ELF32BE>;
 template class elf::MipsAbiFlagsSection<ELF64LE>;

ELF/Writer.cpp

if (Config->Strip != StripPolicy::All) {
     InX::SymTab = make<SymbolTableSection<ELFT>>(*InX::StrTab);
   }
 
   if (Config->BuildId != BuildIdKind::None) {
     InX::BuildId = make<BuildIdSection>();
     Add(InX::BuildId);
   }
 
-  for (InputSection *S : createCommonSections())
-    Add(S);
-
   InX::Bss = make<BssSection>(".bss");
   Add(InX::Bss);
   InX::BssRelRo = make<BssSection>(".bss.rel.ro");
   Add(InX::BssRelRo);
 
   // Add MIPS-specific sections.
   if (Config->EMachine == EM_MIPS) {
     if (!Config->Shared && Config->HasDynSymTab) {
if (auto *IS = dyn_cast<InputSectionBase>(Sec)) {
       if (!IS->Live)
         return false;
     }
     if (auto *S = dyn_cast<MergeInputSection>(Sec))
       if (!S->getSectionPiece(D->Value)->Live)
         return false;
     return true;
   }
-
-  if (auto *Sym = dyn_cast<DefinedCommon>(&B))
-    return Sym->Live;
   return true;
 }
 
 // Local symbols are not in the linker's symbol table. This function scans
 // each object file's symbol table to copy local symbols to the output.
 template <class ELFT> void Writer<ELFT>::copyLocalSymbols() {
   if (!InX::SymTab)
     return;