[PATCH] D22811: [ELF] Linkerscript: allow setting custom output section for common symbols, instead of .bss

[Rui Ueyama via llvm-commits]

ruiu added a comment.

Overall looking good. I think I like the idea of CommonInputSection.


================
Comment at: ELF/InputSection.cpp:673-677
@@ +672,7 @@
+    : InputSection<ELFT>(nullptr, &Hdr) {
+
+  build();
+}
+
+template <class ELFT> void CommonInputSection<ELFT>::build() {
+  Hdr.sh_addralign = 1;
----------------
`build` is called only from the constructor, so I don't see a reason to separate the two functions. You could merge build with the constructor.

================
Comment at: ELF/InputSection.cpp:684-687
@@ +683,6 @@
+  for (Symbol *S : Symtab<ELFT>::X->getSymbols()) {
+    SymbolBody *Body = S->body();
+    if (auto *C = dyn_cast<DefinedCommon<ELFT>>(Body))
+      Symbols.push_back(C);
+  }
+
----------------
  for (Symbol *S : Symtab<ELFT>::X->getSymbols())
    if (auto *C = dyn_cast<DefinedCommon<ELFT>>(S->body()))
      Symbols.push_back(C);

================
Comment at: ELF/InputSection.cpp:704
@@ +703,3 @@
+  }
+  this->Alignment = Hdr.sh_addralign;
+}
----------------
It's weird that we have two variables for one thing -- can you use only this->Alignment?

================
Comment at: ELF/InputSection.cpp:707-712
@@ +706,8 @@
+
+template <class ELFT> void CommonInputSection<ELFT>::fixSymbols() {
+  for (DefinedCommon<ELFT> *C : Symbols) {
+    C->Offset += this->OutSecOff;
+    C->Section = this->OutSec;
+  }
+}
+
----------------
If you add an pointer to the CommonInputSection to DefinedCommon symbol type and use it in Symbol::getVA, you dont' need to adjust offsets (i.e. you can remove this function.)

================
Comment at: ELF/InputSection.h:272
@@ +271,3 @@
+  std::vector<DefinedCommon<ELFT> *> Symbols;
+  typename ELFT::Shdr Hdr;
+};
----------------
` = {}` to initialize with zeros, otherwise this change will trigger asan, because you are passing a pointer to this member to InputSectionBase's ctor and the ctor uses it.

================
Comment at: ELF/Symbols.h:188
@@ -187,1 +187,3 @@
+  // Output section for this symbol (default is .bss)
+  OutputSectionBase<ELFT> *Section = nullptr;
 };
----------------
You can remove this variable, no?


https://reviews.llvm.org/D22811