[ELF] - Do not set output section flags except SHF_{ALLOC,WRITE,EXECINSTR}.

[grimar]

Summary

This is PR34546.

Currently LLD creates output sections even if it has no input sections,
but its command contains an assignment.
Committed code just assigns the same flag that was used in previous
live section.
That does not work sometimes. For example if we have following script:

.ARM.exidx : { *(.ARM.exidx*) }
.foo : { _foo = 0; } }

Then first section has SHF_LINK_ORDER flag. But section foo should not.
That was a reason of crash in OutputSection::finalize(). LLD tried to calculate
Link value, calling front() on empty input sections list.
I think we should only keep access flags and omit all others when creating such sections.

Patch fixes the crash observed.

[ruiu]

This is a very obscure way of setting flags to sections. I shouldn't be buried deep inside code like this.

Instead of doing something like this, drop the flag you don't want to propagate to the output when you parse input sections. I believe you can still recall we did something like this for SHF_COMPRESSED until I changed the way of handling it in some patch I submitted a few months ago.

[grimar]

This comment was removed by grimar.

[grimar]

Ah, I understand my mistake now. I assumed we should keep SHF_LINK_ORDER in output, but probably there is no reasons for that,
as it should only be used by static linker, so we can drop it, just like you suggested. Please ignore my previous comment then.

[grimar]

Problem here that we have to call adjustSectionsBeforeSorting before sorting the output sections,
it makes sections that has only assignment inside to be live and sets the flags
(including incorrect SHF_LINK_ORDER flag) for them.

Then we do actual sorting and assign section indexes after that.
And problem is that we can not sort SHF_LINK_ORDER sections before we know the final indexes.
Seems we can not just drop this flag early like we do for SHF_COMPRESSED for example.
I have no good solution for that atm.

[ruiu]

This is not in a right direction because of the same reason as I wrote before. There are basic rules here:

• output section flags are essentially the same as input sections' flags, and
• to make it possible, drop an unwanted flag after we read it.

I do not have time to point out all the location you need to update to satisfy the above rules, but can you please try to drop SHF_LINK_ORDER earlier? I'd take more time for each patch.

[grimar]

In D37736#872819, @ruiu wrote:

This is not in a right direction because of the same reason as I wrote before. There are basic rules here:

• output section flags are essentially the same as input sections' flags, and
• to make it possible, drop an unwanted flag after we read it.

That was clear from your first comment and I did not update the patch after that yet. My comment above actually describing the
problem I faced when tried to drop the flag. Patch is still in "update planned" status.

I do not have time to point out all the location you need to update to satisfy the above rules, but can you please try to drop SHF_LINK_ORDER earlier? I'd take more time for each patch.

[grimar]

What about SHF_TLS flag ?
I checked that for script below we also spread it to my:

SECTIONS {
 . = 0x10000;
 .tbss : { *(.tbss) }

 .my : { 
   mybegin = .; 
   . += 100;
   myend = .;
   }
}

And it does not seem correct, bfd does not do that either. And this place
looks appropriate for dropping it, isn't ?

[grimar]

In D37736#872819, @ruiu wrote:

This is not in a right direction because of the same reason as I wrote before. There are basic rules here:

• output section flags are essentially the same as input sections' flags, and
• to make it possible, drop an unwanted flag after we read it.

I do not have time to point out all the location you need to update to satisfy the above rules, but can you please try to drop SHF_LINK_ORDER earlier? I'd take more time for each patch.

I prepared a patch that drops SHF_LINK_ORDER flag, it is D38170.

[grimar]

Abandoning in favor of D38170.

[ruiu]

I still don't think I understand the point of this patch. This patch in the original form says that the patch is to fix a crash bug, but it does not crash anymore (is this correct?)

Then what's the point to set a "correct" flag to an empty section? Since a section is empty, it doesn't have any visible effect, no?

[grimar]

In D37736#890129, @ruiu wrote:

I still don't think I understand the point of this patch. This patch in the original form says that the patch is to fix a crash bug, but it does not crash anymore (is this correct?)

No, we did not land anything to fix this crash yet.

[grimar]

• Addressed review comment: added testcase for testing SHF_TLS flag spreading.

[ruiu]

Then, what's the point of this change?

[ruiu]

Thank you for explaining it. Can you add a summary to this code as a comment? Otherwise, it is still a mystery why we apply a bitmask there.

[grimar]

• Included summary comment from discussion thread.

[grimar]

• Updated comment.

[ruiu]

I will probably edit the comment later.

I don't feel "spread" is a right word, and what "specific" means is not clear. I'd make the subject "Do not set output section flags except SHF_{ALLOC,WRITE,EXECINSTR}".

With that, LGTM.

lld/trunk/ELF/LinkerScript.cpp

static bool isAllSectionDescription(const OutputSection &Cmd) {
   for (BaseCommand *Base : Cmd.SectionCommands)
     if (!isa<InputSectionDescription>(*Base))
       return false;
   return true;
 }
 
 void LinkerScript::adjustSectionsBeforeSorting() {
   // If the output section contains only symbol assignments, create a
-  // corresponding output section. The bfd linker seems to only create them if
-  // '.' is assigned to, but creating these section should not have any bad
-  // consequeces and gives us a section to put the symbol in.
+  // 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.
+  // Given that we want to create the section, we have to worry what impact
+  // it will have on the link. For example, if we just create a section with
+  // 0 for flags, it would change which PT_LOADs are created.
+  // We could remember that that particular section is dummy and ignore it in
+  // other parts of the linker, but unfortunately there are quite a few places
+  // that would need to change:
+  //   * The program header creation.
+  //   * The orphan section placement.
+  //   * The address assignment.
+  // The other option is to pick flags that minimize the impact the section
+  // will have on the rest of the linker. That is why we copy the flags from
+  // the previous sections. Only a few flags are needed to keep the impact low.
   uint64_t Flags = SHF_ALLOC;
 
   for (BaseCommand *Cmd : SectionCommands) {
     auto *Sec = dyn_cast<OutputSection>(Cmd);
     if (!Sec)
       continue;
     if (Sec->Live) {
-      Flags = Sec->Flags;
+      Flags = Sec->Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR);
       continue;
     }
 
     if (isAllSectionDescription(*Sec))
       continue;
 
     Sec->Live = true;
     Sec->Flags = Flags;

lld/trunk/test/ELF/linkerscript/arm-exidx-order.s

+# REQUIRES: arm
+# RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o
+# RUN: echo "SECTIONS { . = SIZEOF_HEADERS;    \
+# RUN:         .ARM.exidx : { *(.ARM.exidx*) } \
+# RUN:         .foo : { _foo = 0; } }" > %t.script
+# RUN: ld.lld -T %t.script %t.o -shared -o %t.so
+# RUN: llvm-readobj -s %t.so | FileCheck %s
+
+# CHECK:      Section {
+# CHECK:        Index: 
+# CHECK:        Name: .foo
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:   ]
+
+.fnstart
+.cantunwind
+.fnend

lld/trunk/test/ELF/linkerscript/symbol-only-flags.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "SECTIONS { . = SIZEOF_HEADERS; \
+# RUN:         .tbss : { *(.tbss) }         \
+# RUN:         .foo : { bar = .; } }" > %t.script
+# RUN: ld.lld -o %t --script %t.script %t.o
+# RUN: llvm-readobj -s %t | FileCheck %s
+
+## Check .foo does not get SHF_TLS flag.
+# CHECK:     Section {
+# CHECK:       Index:
+# CHECK:       Name: .foo
+# CHECK-NEXT:  Type: SHT_PROGBITS
+# CHECK-NEXT:  Flags [
+# CHECK-NEXT:    SHF_ALLOC
+# CHECK-NEXT:    SHF_WRITE
+# CHECK-NEXT:  ]
+
+.section .tbss,"awT",@nobits
+.quad 0