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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 3 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/ELF/LinkerScript.cpp:264-274
+    bool found = false;
+    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;
----------------
pohlen wrote:
> I find this a bit difficult to read because of the nested statements. Maybe we can flatten it using llvm::find?
> 
> 
> ```
> auto insertPos = llvm::find_if(sectionCommands, [where](BaseCommand *command) {
>   auto *to = dyn_cast<OutputSection>(*it);
>   return to != nullptr && to->name == where;
> });
> 
> if (insertPos == sectionCommands.end())
>   error("unable to insert " + os->name +
>         Twine(isAfter ? " after " : " before ") + where);
> 
> if (isAfter)
>   ++insertPos;
> 
> sectionCommands.insert(insertPos, os);
> ```
> 
> Up to you. Feel free to leave as is.
Thanks. I agree find_if makes the code easier to read.


================
Comment at: lld/ELF/LinkerScript.cpp:274
+          break;
+        }
+    if (!found)
----------------
grimar wrote:
> 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?
I also prefer fewer braces. (Excess braces harm readability to me...). In Writer.cpp, we have such code:

```lang=cpp
// Writer.cpp
    for (Symbol *sym : symtab->symbols())
      if (sym->isUndefined() && !sym->isWeak())
        if (auto *f = dyn_cast_or_null<SharedFile>(sym->file))
          if (f->allNeededIsKnown)
            error(toString(f) + ": undefined reference to " + toString(*sym));
```

I know that in some places, we added braces just to make it harder to make misleading indentation errors. However, -Wmisleading-indentation (GCC 6, clang 10) makes such protection more or less unnecessary...


================
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.
+
----------------
grimar wrote:
> 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.
Address/offset assignments are different with a `SECTIONS` command. They can be seen from the CHECK/CHECK2 lines.

I added a sentence ` Address/offset assignments are different with a main linker script.` to emphasize this fact.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74375