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

[Eugene Leviant via llvm-commits]

evgeny777 added inline comments.

================
Comment at: ELF/LinkerScript.cpp:111
@@ +110,3 @@
+
+  if (llvm::find(Patterns, "COMMON") && Common->getSize())
+    Ret.push_back(Common);
----------------
grimar wrote:
> llvm::find(Patterns, "COMMON") returns iterator. Condition is always true,
> for cases when Patterns does not contain COMMON it is equal to
> 
> ```
> if (Patterns.end() && ... )
> ```
> You probably want to update testcase also.
I think you're not right. This is ArrayRef<T>, which iterator is a pointer to element. This means that if pointer is nullptr then there is no such element in the array. If you try to do this with STL container, for example std::vector you'll get compilation error, because STL iterator does not define operator bool().

================
Comment at: ELF/Symbols.h:188
@@ -187,1 +187,3 @@
+  // Output section for this symbol (default is .bss)
+  OutputSectionBase<ELFT> *Section = nullptr;
 };
----------------
grimar wrote:
> Since you always call fixSymbols() I think you can skip initialization.
If someone forgets to set Section pointer there will be a crash instead of undefined behavior, which is always good

================
Comment at: ELF/Writer.cpp:225
@@ -225,4 +224,3 @@
 
-  OutputSections = ScriptConfig->DoLayout
-                       ? Script<ELFT>::X->createSections(Factory)
-                       : createSections();
+  CommonSection = llvm::make_unique<CommonInputSection<ELFT>>();
+  OutputSections =
----------------
grimar wrote:
> May be factory should own and create this ?
> That way it would be consistent with OwningSections it has and also
> you'll dont need to add argument in call below, you will be able to take it directly from factory.
This is *input* section and I guess you're talking about OutputSectionFactory, right?


https://reviews.llvm.org/D22811