[PATCH] D39045: [ELF] - Simplify output section creation.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.cpp:464-468
+    // We should report discarded sections for case when no SECTIONS command
+    // is present. For other case we already reported it during processing
+    // layout commands when computed input sections lists.
+    if (!S->Live && !HasSectionsCommand)
+      reportDiscarded(S);
----------------
I do not understand this particular change, but I guess this will be removed by your other change.


================
Comment at: ELF/LinkerScript.cpp:487
+  // If no SECTIONS command was given, we should insert sections commands
+  // before any others, so that we can handle scripts which refers them,
+  // for example: "foo = ABSOLUTE(ADDR(.text)));".
----------------
any others -> otheers


================
Comment at: ELF/LinkerScript.cpp:490-492
+  auto InsertPoint =
+      HasSectionsCommand ? SectionCommands.end() : SectionCommands.begin();
+  SectionCommands.insert(InsertPoint, V.begin(), V.end());
----------------
I think 

  if (HasSectionsCommand)
    SectionCommands.insert(SectionsCommand.begin(), V.begin(), V.end());
  else
    SectionCommands.insert(SectionsCommand.end(), V.begin(), V.end());

is more straightforward.


================
Comment at: ELF/Writer.cpp:162
 
-  // Create output sections.
-  if (Script->HasSectionsCommand) {
-    // If linker script contains SECTIONS commands, let it create sections.
-    Script->processSectionCommands(Factory);
-
-    // Linker scripts may have left some input sections unassigned.
-    // Assign such sections using the default rule.
-    Script->addOrphanSections(Factory);
-  } else {
-    // If linker script does not contain SECTIONS commands, create
-    // output sections by default rules. We still need to give the
-    // linker script a chance to run, because it might contain
-    // non-SECTIONS commands such as ASSERT.
-    Script->processSectionCommands(Factory);
-    createSections();
+  // We want to process linker script commands. When SECTIONS command given
+  // we let it create sections, otherwise we still need to give the linker
----------------
is given


================
Comment at: ELF/Writer.cpp:163
+  // We want to process linker script commands. When SECTIONS command given
+  // we let it create sections, otherwise we still need to give the linker
+  // script a chance to run, because it might contain non-SECTIONS commands
----------------
, -> . 

"Otherwise ..." part of the comment doesn't seem to describe this code. Please move it to the right place.


================
Comment at: ELF/Writer.cpp:168-170
+  // When script does not have SECTIONS command or have sections not listed in
+  // layout, such sections called "orphans". Here we create output sections and
+  // assign orphan sections using the default rule.
----------------
  // Linker scripts controls how input sections are assigned to output sections. Input sections that were not handled by scripts are called "orphans", and they are assigned to output sections by the default rule. Process that.


================
Comment at: ELF/Writer.cpp:173-176
+  // When no SEECTIONS command is present, we want to apply default sorting and
+  // fabricate minimum default commands, like if trivial SECTIONS command would
+  // be given.
+  if (!Script->HasSectionsCommand) {
----------------
Hmm, at this point, I started wondering if the changes to this function actually improves the function. Now, it isn't easy to point out which code is executed when linker script is given/not givne.


https://reviews.llvm.org/D39045