[PATCH] D14140: [ELF2] SECTIONS command basic support

[Denis Protivensky via llvm-commits]

denis-protivensky added inline comments.

================
Comment at: ELF/LinkerScript.cpp:250
@@ +249,3 @@
+  InSec.InputFile = next();
+  mapBraces("(", ")", [this, &InSec]() {
+    StringRef Tok = next();
----------------
ruiu wrote:
> I wouldn't write this in this functional style. This can be written in a plain C style if you define skip(S) which returns true if the next token is S.
> 
>   expect("(");
>   while (!skip(")") {
>     StringRef Tok = next();
>     if (Tok == "EXCLUDE_FILE") {
>     ...
`mapBraces` actually does the same. I can change `peek` to `skip`, but why to produce code duplication where it can be avoided? Procedural style may be more cumbersome here than the current functional implementation.

================
Comment at: ELF/Writer.cpp:461-465
@@ +460,7 @@
+template <class ELFT>
+bool Writer<ELFT>::hasBoundingSymbols(OutputSectionBase<ELFT> *OS) const {
+  if (!hasCustomSections())
+    return true;
+  return OS && Config->OutputSections.count(OS->getName()) == 0;
+}
+
----------------
ruiu wrote:
> Why don't you create __start/__stop symbols if the section is defined using a linker script? Is that a documented behavior?
If one defines output section description, they may define all needed symbols manually.
That's why I think any default actions that can be achieved with linker script should be suppressed.

================
Comment at: ELF/Writer.cpp:614
@@ -574,3 +613,3 @@
 
-  std::stable_sort(OutputSections.begin(), OutputSections.end(),
-                   compareOutputSections<ELFT>);
+  std::stable_sort(
+      OutputSections.begin(), OutputSections.end(),
----------------
ruiu wrote:
> I don't like to add more code to createSections() which is already too long. Move this lambda out of this function.
I need `RegularSections`, which is local to the method. It's captured in the lambda, and I think it would be ugly to pass it to the comparison function along with two output sections to compare. Any ideas how to make it better?

================
Comment at: ELF/Writer.cpp:909
@@ +908,3 @@
+        InputToOutputSection[Name] = OutSec.second.Name;
+}
+
----------------
grimar wrote:
> I think you can do the same for auto Name.
I'll use real types instead.


http://reviews.llvm.org/D14140