[PATCH] D74375: [ELF] Support INSERT [AFTER|BEFORE] for orphan sections

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/LinkerScript.cpp:249
 
 // This method is used to handle INSERT AFTER statement. Here we rebuild
 // the list of script commands to mix sections inserted into.
----------------
Seems the comment needs an update to mention "BEFORE".


================
Comment at: lld/ELF/LinkerScript.cpp:252
 void LinkerScript::processInsertCommands() {
-  std::vector<BaseCommand *> v;
-  auto insert = [&](std::vector<BaseCommand *> &from) {
-    v.insert(v.end(), from.begin(), from.end());
-    from.clear();
-  };
+  for (auto &insert : insertCommands) {
+    OutputSection *os;
----------------
Don't use `auto` then?


================
Comment at: lld/ELF/LinkerScript.cpp:258
 
-  for (BaseCommand *base : sectionCommands) {
-    if (auto *os = dyn_cast<OutputSection>(base)) {
-      insert(insertBeforeCommands[os->name]);
-      v.push_back(base);
-      insert(insertAfterCommands[os->name]);
+    // Skip if os was removed from sectionCommands (empty and discardable).
+    auto from = llvm::find(sectionCommands, os);
----------------
I am not sure this is a clear comment. I had to read the code to understand what you do.
Perhaps it should say something like "we do not handle an INSERT command for an output section that  ...."


================
Comment at: lld/ELF/LinkerScript.cpp:274
+          break;
+        }
+    if (!found)
----------------
The using of curly bracers violates the LLD style a bit. Across it's code we usually added an additional
brackets for each case where a branch contained more than a single line. I.e.:

```
  for (auto it = sectionCommands.begin(), e = sectionCommands.end(); it != e;
       ++it) {
    if (auto *to = dyn_cast<OutputSection>(*it)) {
      if (to->name == where) {
        if (isAfter)
          ++it;
        sectionCommands.insert(it, os);
        found = true;
        break;
      }
    }
  }
```

To clarify: here you have bracers in a internal branch, hence parent branches expected to use them too.
Actually I probably like this rule, but I observed many times that in LLVM people not always follow it.
And seems there is nothing like that in LLVM coding standart document. So seems it was something internal to LLD,
but perhaps it is better to follow for consistency with the rest od the code?


================
Comment at: lld/ELF/LinkerScript.h:316
+  // to be reordered.
+  std::vector<std::tuple<OutputSection *, bool, StringRef>> insertCommands;
 };
----------------
What do you think about adding a named struct instead of using the `tuple` (see `PhdrsCommand`/`AddressState` above).


================
Comment at: lld/test/ELF/linkerscript/insert-after.test:21
+
+## There is no main linker script. INSERT BEFORE just reorders output sections,
+## without making more layout changes.
----------------
BEFORE -> AFTER


================
Comment at: lld/test/ELF/linkerscript/insert-after.test:22
+## There is no main linker script. INSERT BEFORE just reorders output sections,
+## without making more layout changes.
+
----------------
Please clarify/expand what is "without making more layout changes". I think usually the word "layout" is commonly used
to describe the layout of sections. But here the order of them is the same, the difference is in segments created, though the first
comment says nothing about that, it is a bit hard to understand what is actually the difference I think.

Perhaps you could also combine the testing of section names to emphasise the difference (i.e. use something like --check-prefixes=COMMON,PART1 --check-prefixes=COMMON,PART2). Up to you though.


================
Comment at: lld/test/ELF/linkerscript/insert-duplicate.test:6
+
+# CHECK:      Name      Type     Address          Off
+# CHECK-NEXT:           NULL     0000000000000000 000000
----------------
Lets add a general description of the test case?


================
Comment at: lld/test/ELF/linkerscript/insert-duplicate.test:20
+
+## Reordered.
+SECTIONS { .foo.data : { *(.foo.data) } } INSERT AFTER .foo.text;
----------------
Could you expand this comment and below ones please.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74375/new/

https://reviews.llvm.org/D74375