[ELF] Rename adjustSectionsBeforeSorting to adjustOutputSections and make it affect INSERT commands

[MaskRay]

Summary

adjustSectionsBeforeSorting updates some output section attributes
(alignment/flags) and removes discardable empty sections. When it is called,
INSERT commands have not been processed. Therefore the flags propagation rule
may not affect output sections defined in an INSERT command properly.

Fix this by moving processInsertCommands before adjustSectionsBeforeSorting.

adjustSectionsBeforeSorting is somewhat misnamed. The order between it and
sortInputSections does not matter. With the pass shuffle, the name of
adjustSectionsBeforeSorting becomes wrong. Therefore rename it. The new
name is not set into stone. The function mixes several tasks and the
code may be refactored in a way that we may give them more meaningful
names.

With this patch, I think the behavior of attribute propagation becomes more
reasonable. In particular, in the absence of non-INSERT SECTIONS,
inserting a section after a SHF_ALLOC one will give us a SHF_ALLOC section,
not a non-SHF_ALLOC one (see linkerscript/insert-after.test).

[bluca]

Tested, this solves the problem with SHF_ALLOC in a SHT_NOTE section.

[peter.smith]

Would it be possible to go a bit further and move all the functionality from adjustSections that isn't important for placing Orphan Sections to adjustSectionsAfterSorting. We'd then be able to give adjustSections a more specific name. For example adjustOutputSectionFlags. As it stands adjustSections could mean almost anything.

No objections to the work done so far though.

lld/ELF/LinkerScript.cpp

1169

I'm not sure I understand the edit here. The function we are in is adjustOutputSections() and it is not called again. There is an adjustSectionsAfterSorting that is called after Orphans are placed.

[MaskRay]

drop an incorrect comment update

[MaskRay]

In D118529#3284109, @peter.smith wrote:

Would it be possible to go a bit further and move all the functionality from adjustSections that isn't important for placing Orphan Sections to adjustSectionsAfterSorting. We'd then be able to give adjustSections a more specific name. For example adjustOutputSectionFlags. As it stands adjustSections could mean almost anything.

No objections to the work done so far though.

It's difficult to split adjustSectionsBeforeSorting. The isDiscardable functionality is coupled with flag propagation.
So I'd like to the keep the core logic as is.

lld/ELF/LinkerScript.cpp

1169

It's my mistake. It's "*After*", not "*Before*". I'll drop this change.

[peter.smith]

Thanks for the update. The adjustOutputSections still makes me a bit nervous as the name implies "do stuff" without any real idea of what just from the name. One possibility would be to split it into smaller functions, for example: setAlignment, removeFlagsFromEmptySection removeDiscardedOutputSections. Anyhow I don't want to let that get in the way of the actual fix.

[MaskRay]

In D118529#3286751, @peter.smith wrote:

Thanks for the update. The adjustOutputSections still makes me a bit nervous as the name implies "do stuff" without any real idea of what just from the name. One possibility would be to split it into smaller functions, for example: setAlignment, removeFlagsFromEmptySection removeDiscardedOutputSections. Anyhow I don't want to let that get in the way of the actual fix.

Thanks for the review. I acknowledge this. Added the following to the description:

The new name is not set into stone. The function mixes several tasks and the code may be refactored in a way that we may give them more meaningful names.

lld/ELF/LinkerScript.h

public:
   bool hasPhdrsCommands() { return !phdrsCommands.empty(); }
   uint64_t getDot() { return dot; }
   void discard(InputSectionBase &s);
 
   ExprValue getSymbolValue(StringRef name, const Twine &loc);
 
   void addOrphanSections();
   void diagnoseOrphanHandling() const;
-  void adjustSectionsBeforeSorting();
+  void adjustOutputSections();
   void adjustSectionsAfterSorting();
 
   SmallVector<PhdrEntry *, 0> createPhdrs();
   bool needsInterpSection();
 
   bool shouldKeep(InputSectionBase *s);
   const Defined *assignAddresses();
   void allocateHeaders(SmallVector<PhdrEntry *, 0> &phdrs);

lld/ELF/LinkerScript.cpp

 
 // Process INSERT [AFTER|BEFORE] commands. For each command, we move the
 // specified output section to the designated place.
 void LinkerScript::processInsertCommands() {
   SmallVector<OutputSection *, 0> moves;
   for (const InsertCommand &cmd : insertCommands) {
     for (StringRef name : cmd.names) {
       // If base is empty, it may have been discarded by
-      // adjustSectionsBeforeSorting(). We do not handle such output sections.
+      // adjustOutputSections(). We do not handle such output sections.
       auto from = llvm::find_if(sectionCommands, [&](SectionCommand *subCmd) {
         return isa<OutputSection>(subCmd) &&
                cast<OutputSection>(subCmd)->name == name;
       });
       if (from == sectionCommands.end())
         continue;
       moves.push_back(cast<OutputSection>(*from));
       sectionCommands.erase(from);
if (sec.phdrs.empty()) {
     // headers to sections that are allocated.
     if (sec.flags & SHF_ALLOC)
       sec.phdrs = phdrs;
   } else {
     phdrs = sec.phdrs;
   }
 }
 
-void LinkerScript::adjustSectionsBeforeSorting() {
+void LinkerScript::adjustOutputSections() {
   // If the output section contains only symbol assignments, create a
   // corresponding output section. The issue is what to do with linker script
   // like ".foo : { symbol = 42; }". One option would be to convert it to
   // "symbol = 42;". That is, move the symbol out of the empty section
   // description. That seems to be what bfd does for this simple case. The
   // problem is that this is not completely general. bfd will give up and
   // create a dummy section too if there is a ". = . + 1" inside the section
   // for example.
if (isEmpty)
                             SHF_WRITE | SHF_EXECINSTR);
 
     // The code below may remove empty output sections. We should save the
     // specified program headers (if exist) and propagate them to subsequent
     // sections which do not specify program headers.
     // An example of such a linker script is:
     // SECTIONS { .empty : { *(.empty) } :rw
     //            .foo : { *(.foo) } }
     // Note: at this point the order of output sections has not been finalized,
     // because orphans have not been inserted into their expected positions. We
     // will handle them in adjustSectionsAfterSorting().
     if (sec->sectionIndex != UINT32_MAX)
       maybePropagatePhdrs(*sec, defPhdrs);
 
     if (isEmpty && isDiscardable(*sec)) {
       sec->markDead();
       cmd = nullptr;

lld/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::sortInputSections() {
   maybeShuffle(order);
   for (SectionCommand *cmd : script->sectionCommands)
     if (auto *sec = dyn_cast<OutputSection>(cmd))
       sortSection(sec, order);
 }
 
 template <class ELFT> void Writer<ELFT>::sortSections() {
   llvm::TimeTraceScope timeScope("Sort sections");
-  script->adjustSectionsBeforeSorting();
 
   // Don't sort if using -r. It is not necessary and we want to preserve the
   // relative order for SHF_LINK_ORDER sections.
-  if (config->relocatable)
+  if (config->relocatable) {
+    script->adjustOutputSections();
     return;
+  }
 
   sortInputSections();
 
-  for (SectionCommand *cmd : script->sectionCommands) {
-    auto *os = dyn_cast<OutputSection>(cmd);
-    if (!os)
-      continue;
-    os->sortRank = getSectionRank(os);
-  }
-
+  for (SectionCommand *cmd : script->sectionCommands)
+    if (auto *osec = dyn_cast_or_null<OutputSection>(cmd))
+      osec->sortRank = getSectionRank(osec);
   if (!script->hasSectionsCommand) {
     // We know that all the OutputSections are contiguous in this case.
     auto isSection = [](SectionCommand *cmd) {
       return isa<OutputSection>(cmd);
     };
     std::stable_sort(
         llvm::find_if(script->sectionCommands, isSection),
         llvm::find_if(llvm::reverse(script->sectionCommands), isSection).base(),
         compareSections);
-
-    // Process INSERT commands. From this point onwards the order of
-    // script->sectionCommands is fixed.
-    script->processInsertCommands();
-    return;
   }
 
+  // Process INSERT commands and update output section attributes. From this
+  // point onwards the order of script->sectionCommands is fixed.
   script->processInsertCommands();
+  script->adjustOutputSections();
+
+  if (!script->hasSectionsCommand)
+    return;
 
   // Orphan sections are sections present in the input files which are
   // not explicitly placed into the output file by the linker script.
   //
   // The sections in the linker script are already in the correct
   // order. We have to figuere out where to insert the orphan
   // sections.
   //

lld/test/ELF/linkerscript/insert-after.test

 # RUN: ld.lld --script %s %t1.o -o %t2
 # RUN: llvm-readelf -S -l %t2 | FileCheck --check-prefix=CHECK2 %s
 # CHECK2:      Name      Type     Address            Off      Size   ES Flg
 # CHECK2-NEXT:           NULL
 # CHECK2-NEXT: .text     PROGBITS 000000000020{{.*}} [[#%x,]] 000008 00  AX
 # CHECK2-NEXT: .foo.text PROGBITS [[#%x,]]           [[#%x,]] 000008 00  AX
 # CHECK2-NEXT: .data     PROGBITS [[#%x,]]           [[#%x,]] 000008 00  WA
 # CHECK2-NEXT: .foo.data PROGBITS [[#%x,]]           [[#%x,]] 000008 00  WA
-# CHECK2-NEXT: .byte     PROGBITS [[#%x,]]           [[#%x,]] 000001 00     0
+# CHECK2-NEXT: .byte     PROGBITS [[#%x,]]           [[#%x,]] 000001 00  WA
 # CHECK2:      Type      {{.*}} Flg Align
 # CHECK2-NEXT: PHDR      {{.*}} R   0x8
 # CHECK2-NEXT: LOAD      {{.*}} R   0x1000
 # CHECK2-NEXT: LOAD      {{.*}} R E 0x1000
 # CHECK2-NEXT: LOAD      {{.*}} RW  0x1000
 # CHECK2-NEXT: GNU_STACK {{.*}} RW  0
 
 SECTIONS { .byte : { BYTE(0) } } INSERT AFTER .data;
 
 SECTIONS { .foo.data : { *(.foo.data) } } INSERT AFTER .data;
 
 ## The input section .foo.text is an orphan. It will be placed in .foo.text
 SECTIONS { .foo.text : {} } INSERT AFTER .text;

lld/test/ELF/linkerscript/insert-before.test

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/insert-after.s -o %t1.o
 
 ## Main linker script contains .text and .data sections. Here
 ## we check that can use INSERT BEFORE to insert sections .foo.data
 ## and .foo.text at the right places.
 
 # RUN: ld.lld %t1.o -o %t1 --script %p/Inputs/insert-after.script --script %s
 # RUN: llvm-readelf -S -l %t1 | FileCheck %s
 # CHECK:      Name      Type     Address          Off    Size   ES Flg
 # CHECK-NEXT:           NULL
 # CHECK-NEXT: .foo.text PROGBITS 0000000000000000 001000 000008 00  AX
 # CHECK-NEXT: .text     PROGBITS 0000000000000008 001008 000008 00  AX
-# CHECK-NEXT: .byte     PROGBITS 0000000000000010 001010 000001 00  WA
+# CHECK-NEXT: .byte     PROGBITS 0000000000000010 001010 000001 00  AX
 # CHECK-NEXT: .foo.data PROGBITS 0000000000000011 001011 000008 00  WA
 # CHECK-NEXT: .data     PROGBITS 0000000000000019 001019 000008 00  WA
 # CHECK:      Type
 # CHECK-NEXT: LOAD {{.*}} R E
 # CHECK-NEXT: LOAD {{.*}} RW
 # CHECK-NEXT: GNU_STACK {{.*}} RW
 
 ## There is no main linker script. INSERT BEFORE just reorders output sections,
 ## without making more layout changes. Address/offset assignments are different
 ## with a main linker script.
 
 # RUN: ld.lld --script %s %t1.o -o %t2
 # RUN: llvm-readelf -S -l %t2 | FileCheck --check-prefix=CHECK2 %s
 # CHECK2:      Name      Type     Address            Off      Size   ES Flg
 # CHECK2-NEXT:           NULL
 # CHECK2-NEXT: .foo.text PROGBITS 000000000020{{.*}} [[#%x,]] 000008 00  AX
 # CHECK2-NEXT: .text     PROGBITS [[#%x,]]           [[#%x,]] 000008 00  AX
-# CHECK2-NEXT: .byte     PROGBITS [[#%x,]]           [[#%x,]] 000001 00     0
+# CHECK2-NEXT: .byte     PROGBITS [[#%x,]]           [[#%x,]] 000001 00  WA
 # CHECK2-NEXT: .foo.data PROGBITS [[#%x,]]           [[#%x,]] 000008 00  WA
 # CHECK2-NEXT: .data     PROGBITS [[#%x,]]           [[#%x,]] 000008 00  WA
 # CHECK2:      Type      {{.*}} Flg Align
 # CHECK2-NEXT: PHDR      {{.*}} R   0x8
 # CHECK2-NEXT: LOAD      {{.*}} R   0x1000
 # CHECK2-NEXT: LOAD      {{.*}} R E 0x1000
 # CHECK2-NEXT: LOAD      {{.*}} RW  0x1000
 # CHECK2-NEXT: GNU_STACK {{.*}} RW  0
 
 SECTIONS { .byte : { BYTE(0) } } INSERT BEFORE .data;
 
 SECTIONS { .foo.data : { *(.foo.data) } } INSERT BEFORE .data;
 
 ## The input section .foo.text is an orphan. It will be placed in .foo.text
 SECTIONS { .foo.text : {} } INSERT BEFORE .text;