In the future, we should stop enforcing --no-rosegment for hasSectionsCommand so that features can be more orthogonal. (I've mentioned this many times to Android/FreeBSD folks...)

Seems the comment needs an update to mention "BEFORE".

Don't use auto then?

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 ...."

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?

What do you think about adding a named struct instead of using the tuple (see PhdrsCommand/AddressState above).

BEFORE -> AFTER

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.

Lets add a general description of the test case?

Could you expand this comment and below ones please.

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.

[nit]: Move declaration to initialization?

StringRef where = next();

FWIW I'd tend towards using a named struct here. We have to look down to the bit of code to find out what the fields are.

OutputSection *os;
bool isAfter;
StringRef where;
std::tie(os, isAfter, where) = insert;

Perhaps the intention can be added to the release notes for 10.0. For example in LLD 11.0 we intend to make the following changes:

• a linker script no longer implies --no-rosegment

This might be some way towards informing users of interface changes so that they don't get surprised. Given that few read the release notes, we might also consider a warning, maybe an optional one if someone uses a feature that will have its default changed.

Thanks. I agree find_if makes the code easier to read.

I also prefer fewer braces. (Excess braces harm readability to me...). In Writer.cpp, we have such code:

// 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...

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.

@srhines @nickdesaulniers I want to delete config->singleRoRx = true; in the future to be more consistant with other parts of lld. (It also matches ld.bfd -z separate-code)

Can you please test whether any project needs adaptation with this change?

Sorry, what is the ask? I don't see separate-code being used in the Linux kernel. Or are you asking about all of Android (userspace)?

I'm asking about all of Android userspace that may use the linker script SECTIONS command.

Currently, due to config->singleRoRx = true; (like enforced --no-rosegment), there is no R PT_LOAD segment with a SECTIONS command.

After deleting config->singleRoRx = true;, an executable with a SECTIONS command will get one R PT_LOAD segment and one RX PT_LOAD segment, like a regular executable without a linker script.

I don't expect it will affect Linux kernel. We can easily add --no-rosegment if it does.

We'll likely be able to check this out with our next update. If we hit problems, we can follow up. Thanks for letting me know.