[PATCH] D38137: [ELF] Simpler scheme for handling common symbols

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:57
 
-std::vector<InputSection *> elf::createCommonSections() {
-  if (!Config->DefineCommon)
-    return {};
-
-  std::vector<InputSection *> Ret;
+template <class ELFT> void elf::createCommonSections() {
   for (Symbol *S : Symtab->getSymbols()) {
----------------
Please add a comment:

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


================
Comment at: ELF/SyntheticSections.cpp:64-70
+    // Create a synthetic section for the common data.
+    auto *Section = make<BssSection>("COMMON");
+    InputSections.push_back(Section);
+    Section->reserveSpace(Sym->Size, Sym->Alignment);
+    InputFile *File = Sym->getFile();
+    Section->File = File;
+    Section->Live = !Config->GcSections;
----------------
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);



================
Comment at: ELF/SyntheticSections.cpp:74
+    // don't have to care about DefinedCommon symbols beyond this point.
+    Sym->Section = Section;
+    SymbolBody *B = S->body();
----------------
You are passing Section to `replaceBody`, but do you still need this?


================
Comment at: ELF/SyntheticSections.cpp:75
+    Sym->Section = Section;
+    SymbolBody *B = S->body();
+    replaceBody<DefinedRegular>(S, File, B->getName(), B->IsLocal == 1,
----------------
Do you need `B`? You already have `Sym` which is an instance of a subclass of SymbolBody.


================
Comment at: ELF/SyntheticSections.cpp:76
+    SymbolBody *B = S->body();
+    replaceBody<DefinedRegular>(S, File, B->getName(), B->IsLocal == 1,
+                                B->StOther, B->Type,
----------------
`== 1` is odd; just like we don't do `if (<some boolean expression> == 1`), I'd omit it.


================
Comment at: ELF/SyntheticSections.cpp:78
+                                B->StOther, B->Type,
+                                0u, // Value 0 - one symbol per section.
+                                B->getSize<ELFT>(), Section);
----------------
0u -> 0 (0 is 0 however it is sign-extended.)

Do you actually need this comment?


https://reviews.llvm.org/D38137