[ELF] Change default output section type to SHT_PROGBITS

[andrewng]

Summary

This fixes an issue where a symbol only section at the start of a
PT_LOAD segment, causes incorrect alignment of the file offset for the
start of the segment. This results in the output of an invalid ELF.

This change breaks an existing test related to orphan sections. See test
orphan-phdrs.s for details.

[andrewng]

The default was SHT_PROGBITS in the past, and was changed to SHT_NOBITS to "reduce file size". However, it's not clear that it actually reduces output file size, but only reduces the value of file size in the output. In any case, it does cause the alignment problem described and shown in the symbol-only-align.test test.

I intend to fix the orphan behaviour in a separate change, which is why it has been marked as XFAIL for now.

[MaskRay]

https://reviews.llvm.org/D59986 fixes a similar issue.

[grimar]

In D60131#1451518, @andrewng wrote:

I intend to fix the orphan behavior in a separate change, which is why it has been marked as XFAIL for now.

I am not sure we have the practice to disable the test cases. What do you think about uploading the fix for orphan behavior
in a separate change rebased on this patch too, so that these 2 patches can be reviewed together?
(in that case, having XFAIL here is fine I think).

test/ELF/linkerscript/symbol-only-align.test

6

In LLD we avoid using multiple echo calls. Please do it with a single call.

11

Could you add a brief description of what this test does and what it checks here please?

[andrewng]

Updated to address review comments.

[andrewng]

test/ELF/linkerscript/symbol-only-align.test

6

Had to add ';' for a single echo to work.

[andrewng]

I am not sure we have the practice to disable the test cases. What do you think about uploading the fix for orphan behavior
in a separate change rebased on this patch too, so that these 2 patches can be reviewed together?
(in that case, having XFAIL here is fine I think).

I'm still not 100% sure of the best approach for the other patch for the orphan behaviour. However, this fix is more important.

[jakehehrlich]

This seems like the wrong fix to me. Shouldn't we be fixing the alignment bug with respect to segments not changing things like this?

If I recall correctly I made a change to make NOBITS the default because some sections nthat could have been NOBITS we're winding up as PROGBITS. I don't remember the exact details. I'm fairly sure this change regresses that point. You've changedna bunch of test from expecting NOBITS to expecting PROGBITS but that seems unjustified.

[andrewng]

In D60131#1453128, @jakehehrlich wrote:

This seems like the wrong fix to me. Shouldn't we be fixing the alignment bug with respect to segments not changing things like this?

If I recall correctly I made a change to make NOBITS the default because some sections nthat could have been NOBITS we're winding up as PROGBITS. I don't remember the exact details. I'm fairly sure this change regresses that point. You've changedna bunch of test from expecting NOBITS to expecting PROGBITS but that seems unjustified.

When I first looked into this issue, I did in fact look at the "layout" side for a potential fix. But as I progressed, it was not clear that it might be the most appropriate (or simplest) fix. So then I investigated why this situation was arising at all and it lead me back to this change of the default output section from SHT_PROGBITS to SHT_NOBITS. I tried to find more details for why this change was made, but couldn't find anything concrete other than the reduction in "file size" which does not appear to benefit the actual file output size in most cases.

If the default output section must be SHT_NOBITS, then an alternative solution will be required, but is this really a requirement?

[andrewng]

The follow up patch to fix the orphan PHDR test is D60273.

[andrewng]

Ping.

Is this patch acceptable or do we need to start looking for an alternative (and most probably more complex solution) to the issue described above?

[ruiu]

IIUC, you are fixing an issue that a segment whose size is 0 doesn't have a file offset that is congruent modulo page size. Is this correct?

The file offset for an empty segment is not actually significant; we can just set a dummy value. Have you considered that?

test/ELF/linkerscript/orphan-phdrs.s

1

We shouldn't keep a test that doesn't run at all. Please just remove.

test/ELF/linkerscript/symbol-only-align.test

2–5

nit: You can just write them in a single line.

[andrewng]

In D60131#1471202, @ruiu wrote:

IIUC, you are fixing an issue that a segment whose size is 0 doesn't have a file offset that is congruent modulo page size. Is this correct?

The file offset for an empty segment is not actually significant; we can just set a dummy value. Have you considered that?

No, the actual issue is that a non-empty PT_LOAD segment that starts with an empty symbol only section does not have a file offset that is congruent modulo the page size. As a result, the output is not a valid ELF.

test/ELF/linkerscript/orphan-phdrs.s

1

Just to be clear, you would like me to remove this valid test in this patch and then add it back again in the follow up patch D60273? Or should I submit D60273 first, even though it is the change in this patch that necessitates the change in D60273?

test/ELF/linkerscript/symbol-only-align.test

2–5

I deliberately put them on separate lines for readability. Would you prefer a single line?

[ruiu]

LGTM

No, the actual issue is that a non-empty PT_LOAD segment that starts with an empty symbol only section does not have a file offset that is congruent modulo the page size. As a result, the output is not a valid ELF.

Got it, thanks.

test/ELF/linkerscript/symbol-only-align.test

2–5

Yeah, I'd prefer not to contain too many space characters in a test file. The result of the concatenation is a long single line, as an escaped newline is removed before given to a command. If something goes wrong, that resulted in something like this, which isn't easy to read:

SECTIONS { foo                                                bar                                                baz                                                foo                                                bar                                                baz                        }
.                                                                                                                                                                                                                                            ^ invalid parameter

For this particular case, the concatenated line wouldn't be that long, but in general, I'd like to not use too many escaped newlines.

[phosek]

This change broke our kernel build, I filed a bug PR41653 which has more details, but it seems like that with this change, linker produces wrong layout. Can we please revert this change until we figure out how to address this issue?

[andrewng]

In D60131#1482865, @phosek wrote:

This change broke our kernel build, I filed a bug PR41653 which has more details, but it seems like that with this change, linker produces wrong layout. Can we please revert this change until we figure out how to address this issue?

Do either or both of D61186 and D61197 help with your issue, as these are related to orphan behaviour as a result of this change to the default output section type.

[phosek]

D61197 seems to have addressed the issue, D61186 made no difference.

[andrewng]

In D60131#1483142, @phosek wrote:

D61197 seems to have addressed the issue, D61186 made no difference.

Hopefully, I'll get an LGTM for D61197 soon and will be able to land it. We're waiting on this fix too :). The default output section type of SHT_NOBITS was actually "hiding" a number of issues...

[phosek]

In D60131#1483160, @andrewng wrote:

In D60131#1483142, @phosek wrote:

D61197 seems to have addressed the issue, D61186 made no difference.

Hopefully, I'll get an LGTM for D61197 soon and will be able to land it. We're waiting on this fix too :). The default output section type of SHT_NOBITS was actually "hiding" a number of issues...

There's still no response on D61197, can we revert this change until the other one is approved and then reland it? I'd argue that in the current state it's actually worse than before.

ELF/LinkerScript.cpp

 OutputSection *LinkerScript::createOutputSection(StringRef Name,
                                                  StringRef Location) {
   OutputSection *&SecRef = NameToOutputSection[Name];
   OutputSection *Sec;
   if (SecRef && SecRef->Location.empty()) {
     // There was a forward reference.
     Sec = SecRef;
   } else {
-    Sec = make<OutputSection>(Name, SHT_NOBITS, 0);
+    Sec = make<OutputSection>(Name, SHT_PROGBITS, 0);
     if (!SecRef)
       SecRef = Sec;
   }
   Sec->Location = Location;
   return Sec;
 }
 
 OutputSection *LinkerScript::getOrCreateOutputSection(StringRef Name) {

test/ELF/linkerscript/arm-exidx-order.test

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

test/ELF/linkerscript/extend-pt-load2.test

SECTIONS {
   .hash : {}
   .dynstr : {}
   .text : { *(.text) }
   bar : { . = ALIGN(0x1000); }
   .data.rel.ro : { *(.data.rel.ro) }
 }
 
 # CHECK:      .text        PROGBITS 00000000000001bc 0001bc 000001 00 AX
-# CHECK-NEXT: bar          NOBITS   00000000000001bd 0001bd 000e43 00 AX
+# CHECK-NEXT: bar          PROGBITS 00000000000001bd 0001bd 000e43 00 AX
 # CHECK-NEXT: .data.rel.ro PROGBITS 0000000000001000 001000 000001 00 WA
 
-# CHECK:      LOAD 0x000000 0x0000000000000000 0x0000000000000000 0x0001bd 0x001000 R E
+# CHECK:      LOAD 0x000000 0x0000000000000000 0x0000000000000000 0x001000 0x001000 R E
 # CHECK-NEXT: LOAD 0x001000 0x0000000000001000 0x0000000000001000 0x000068 0x000068 RW

test/ELF/linkerscript/merge-sections.s

 # CHECK:      Name: end
 # CHECK-NEXT: Value: 0x236
 
 # Check that we don't crash with --gc-sections
 # RUN: ld.lld --gc-sections -o %t2 --script %t.script %t -shared
 # RUN: llvm-readobj -s -t %t2 | FileCheck %s --check-prefix=GC
 
 # GC:        Name: .foo
-# GC-NEXT:   Type: SHT_NOBITS
+# GC-NEXT:   Type: SHT_PROGBITS
 # GC-NEXT:   Flags [
 # GC-NEXT:     SHF_ALLOC
 # GC-NEXT:   ]
 
 .section        .foo.1a,"aMS",@progbits,1
 .asciz "foo"
 
 .section        .foo.1b,"aMS",@progbits,1

test/ELF/linkerscript/orphan-phdrs.s

+# XFAIL: *
+
+# This test fails because empty sections that are explicitly assigned to a
+# segment are kept. The flags of such empty sections are derived from preceding
+# live sections. This unfortunately means that orphan sections can then end up
+# in the wrong segment, in this test .orphan ends up in the rw segment rather
+# than the intended exec segment.
+#
+# This test was previously working because empty sections happened to be
+# SHT_NOBITS, which changes the ranking such that it no longer affects the
+# orphan section.
+
 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: echo "PHDRS { \
 # RUN:   exec PT_LOAD FLAGS(0x4 | 0x1); \
 # RUN:   rw   PT_LOAD FLAGS(0x4 | 0x2); \
 # RUN: } \
 # RUN: SECTIONS { \
 # RUN:  .text : { *(.text) } :exec \

 # CHECK: Section Headers
 # CHECK: .text
 # CHECK-NEXT: .orphan
 # CHECK-NEXT: .empty
 # CHECK-NEXT: .rw
 
 # CHECK: Segment Sections
 # CHECK-NEXT: .text .orphan
-# CHECK-NEXT: .rw
+# CHECK-NEXT: .empty .rw
 
-.section .text, "ax"
+.section .text,"ax"
  ret
 
-.section .rw, "aw"
+.section .rw,"aw"
  .quad 0
 
-.section .orphan, "ax"
+.section .orphan,"ax"
  ret

test/ELF/linkerscript/symbol-only-align.test

+# REQUIRES: x86
+# RUN: echo '.text; \
+# RUN:        ret;  \
+# RUN:       .data; \
+# RUN:       .quad 0' > %t.s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t.s -o %t.o
+# RUN: ld.lld -o %t --script %s %t.o -shared
+# RUN: llvm-readobj -elf-output-style=GNU -s -t -l %t | FileCheck %s
+
+PHDRS {
+  text PT_LOAD FLAGS(0x5);
+  data PT_LOAD FLAGS(0x6);
+}
+
+SECTIONS {
+  . = SIZEOF_HEADERS;
+  .text : { *(.text) } : text
+  . = ALIGN(CONSTANT(MAXPAGESIZE));
+  foo : { __start_foo = .; *(foo); __end_foo = .; } : data
+  .data : { *(.data) }
+  .dynamic : { *(.dynamic) }
+}
+
+## Check that foo, the symbol only section, has the expected aligned address and
+## file offset. Also check that the section's symbols and the data segment's
+## offset and addresses match.
+
+# CHECK: Section Headers
+# CHECK:      foo   PROGBITS 0000000000[[ADDR:[0-9a-f]*]] [[ADDR]]
+# CHECK-NEXT: .data PROGBITS 0000000000[[ADDR]] [[ADDR]]
+
+# CHECK: Symbol table
+# CHECK: 0000000000[[ADDR]] 0 NOTYPE GLOBAL DEFAULT {{[0-9]+}} __start_foo
+# CHECK: 0000000000[[ADDR]] 0 NOTYPE GLOBAL DEFAULT {{[0-9]+}} __end_foo
+
+# CHECK: Program Headers
+# CHECK:      LOAD
+# CHECK-NEXT: LOAD 0x[[ADDR]] 0x0000000000[[ADDR]] 0x0000000000[[ADDR]]

test/ELF/linkerscript/symbol-only-flags.test

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