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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Driver.cpp:1084-1086
+  // After this point there are no more common symbols
+  // unless this is a -r link and we are preserving
+  // common symbols.
----------------
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.


================
Comment at: ELF/Symbols.cpp:103
+  case SymbolBody::DefinedCommonKind:
+    llvm_unreachable("unreachable: common are converted to bss");
   case SymbolBody::SharedKind: {
----------------
Messages for `llvm_unreachable` don't usually start with "unreachable: ", so remove it.


================
Comment at: ELF/SyntheticSections.cpp:72
+
+    // Change the symbol to be a regular definition.
+    Sym->Section = Section;
----------------
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.


================
Comment at: ELF/SyntheticSections.h:744
 
-std::vector<InputSection *> createCommonSections();
+template <class ELFT> void commonToBss();
 InputSection *createInterpSection();
----------------
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.


https://reviews.llvm.org/D38137