[ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used

[MaskRay]

Summary

Currently when --no-rosegment is specified or a linker script with SECTIONS command is used,
.rodata (A) .text (AX) are assigned the same rank and .rodata may be placed after .text .
This increases the gap between .text and .bss and can cause pc-relative relocation overflow (e.g. gcc crtbegin.o crtbegin.S have R_X86_64_PC32 relocation from .text to .bss).

This patch makes SingleRoRx affect only segment layout, not section layout. As a consequence, .rodata will be placed before .text regardless of SingleRoRx.

[MaskRay]

30+ failing tests because of changed addresses...

[MaskRay]

Update test. 30 more...

[MaskRay]

Fix some tests

[MaskRay]

Update description

[MaskRay]

More

[MaskRay]

Updated all tests

[atanasyan]

test/ELF/mips-gp-ext.s

38 ↗

(On Diff #152224)

Please change in this comment 0x13c to 0x1ec (new _gp value)

[MaskRay]

update _gp in mips-gp-ext.s

[echristo]

I think this is obsolete after the other patch?

-eric

[MaskRay]

Sorry this has been superseded by https://reviews.llvm.org/D48406

(The only commands about arc I know are arc diff origin/master and sometimes it got messed up)

[MaskRay]

Sorry this is still useful...

This is a subset of https://reviews.llvm.org/D48406 but this also has less failing tests to update (30+)

D48406 requires to update 60+ tests. I'd like to see this revision reviewed first before working on D48406 tests.

[echristo]

ELF/Writer.cpp

767

Can you rebase this on the other patch and then add some comments about the ranking into the source?

[MaskRay]

ELF/Writer.cpp

767

D48406 is based on this one (which I'd like to collect some feedback about section reordering as fixing 60+ tests are painful)

This one D48405 is not dependent on the other.

[MaskRay]

ELF/Writer.cpp

767

This patch merely removes code (the if (!Config->SingleRoRx) condition). It is fairly obvious what it does.

I think SingleRoRx (true if --no-rosegment or SECTION command is used) should be an option that changes the segment layout (one more PT_LOAD), not an option that also changes section layout (so I think it should have been documented why the SingleRoRx flag disabled the RF_EXEC assignment).

[echristo]

ELF/Writer.cpp

767

I agree with all of that. I'm still pretty sure that the comment above it needs to be updated :)

[grimar]

This patch seems to do a lot of unnecessary changes to the test cases.
Generally, a best practice for the patch is to do a minimal amount of changes,
though here I see unnecessary renaming changes, refactoring(?) changes etc in tests.

test/ELF/arm-exidx-order.s

1 ↗

(On Diff #152327)

Can you avoid unnecessary changes in the patches, please?
Moving "requires" is OK, but can be done separately. Such changes makes doing review harder.

test/ELF/arm-exidx-sentinel-orphan.s

1 ↗

(On Diff #152327)

The same.

test/ELF/arm-plt-reloc.s

165 ↗

(On Diff #152327)

This can be

Section {{.*}} .rel.plt

test/ELF/eh-frame-padding-no-rosegment.s

10 ↗

(On Diff #152327)

Why you had to add that?

test/ELF/linkerscript/implicit-program-header.test

7 ↗

(On Diff #152327)

Was it necessary change?

test/ELF/linkerscript/locationcountererr2.s

4 ↗

(On Diff #152327)

Why are you changing the way how we writing the script?

test/ELF/linkerscript/no-space.s

6 ↗

(On Diff #152327)

Why are you doing this in this patch?

test/ELF/linkerscript/non-absolute.s

8 ↗

(On Diff #152327)

This seems wrong

0x94 - 168 != 0xB

test/ELF/linkerscript/non-alloc.s

6 ↗

(On Diff #152327)

Again, why? What is wrong with using %t and llvm-readelf?

test/ELF/linkerscript/orphan-first-cmd.test

20 ↗

(On Diff #152327)

Why you removed this?

test/ELF/linkerscript/sections-keep.s

3 ↗

(On Diff #152327)

Why are you renaming inputs? It brings so many changes to the rest of the test case, please don't do that.

test/ELF/linkerscript/sort-non-script.s

6 ↗

(On Diff #152327)

Same here and below.

[MaskRay]

test/ELF/arm-exidx-order.s

1 ↗

(On Diff #152327)

Sorry if it did any *unnecessary* changes. But I don't there any change is refactoring.

1 ↗

(On Diff #152327)

s/there/think/

[MaskRay]

test/ELF/arm-exidx-order.s

1 ↗

(On Diff #152327)

Fixing these tests are already painful and I am not prepared to split to simple REQUIRES: moving change to a separate patch (double work for the case that is not worthy)

[MaskRay]

test/ELF/eh-frame-padding-no-rosegment.s

10 ↗

(On Diff #152327)

Otherwise .text is zero-sized and the segment layout will be different.

PHDR: Segment Sections
PHDR: .eh_frame {{.*}}.text

zero-sized section may have weird display in llvm-readelf (it may be counted multiple times).

test/ELF/linkerscript/implicit-program-header.test

7 ↗

(On Diff #152327)

llvm-readelf is more common. I happened to change this place so I replaced the command.

test/ELF/linkerscript/locationcountererr2.s

4 ↗

(On Diff #152327)

OK. reverted the change.

This backslash form is more common so I replaced it. Since you have objection, I reverted it.

test/ELF/linkerscript/no-space.s

6 ↗

(On Diff #152327)

Just before llvm-readelf is used more and for consistency I wanted to replace them.

If you insist removing them I would probably have to move that to a separate revision

test/ELF/linkerscript/non-absolute.s

8 ↗

(On Diff #152327)

Thanks! Fixed.

test/ELF/linkerscript/non-alloc.s

6 ↗

(On Diff #152327)

Nothing wrong but just for consistency.

test/ELF/linkerscript/orphan-first-cmd.test

20 ↗

(On Diff #152327)

Added back.

# CHECK-NEXT: Address: 0x1070

But I do not think this address is useful (it merely causes trouble when section layout is changed)

[MaskRay]

You regard these changes:

llvm-readobj --elf-output-style=GNU -> llvm-readelf
llvm-mc ... -o %t -> llvm-mc ... -o %t.o

as "refactoring". They were for the purpose of consistency.
Sorry about all these and I did not know they made you think so. I thought the reviewer would be annoyed to see these changes being reviewed twice as I took them as lifting a finger when I happened to change places nearby. (it was really really painful for me to go through the 30+ failing tests and I didn't want to double my work) If you had mercy upon me and let me save my efforts on D48406 I would very appreciate it (also a section layout change and would affect another 30+ failing tests. But the section layout has not been finalized. I do want to remove some hackery DYNSTR DYNSYM nad make them more general).

Another annoyance I found when updating the tests is the llvm-objdump display of TEXT DATA. I have made D48472 and its companion LLD change D48473 and if you can review it I will be very grateful. (I promise there are only TEXT DATA -> DATA changes, absolutely nothing else sneaked in! 😼)

[grimar]

In D48405#1140879, @MaskRay wrote:

You regard these changes:

llvm-readobj --elf-output-style=GNU -> llvm-readelf
llvm-mc ... -o %t -> llvm-mc ... -o %t.o

as "refactoring". They were for the purpose of consistency.

Please do not do it. I am OK if you will commit this changes separately as NFC,
but don't mix functionality changes and unrelative cosmetic changes in the patches.

test/ELF/arm-exidx-order.s

1 ↗

(On Diff #152327)

Then just revert it, please. There is no need to move this "REQUIRES arm" in this patch.

Doing as less as possible changes in the patch is really important not only during the review stage.
It is also useful for future cases when we might need to review previous commits. Imagine what is better,
to review many places where most of them are unimportant or to focus on the changes which were a purpose of the patch?

Also if we might want to revert this patch (who knows, that is possible) then such "refactorings" would be probably reverted too.

test/ELF/eh-frame-padding-no-rosegment.s

10 ↗

(On Diff #152327)

Thanks for the explanation, that was no obvious.

test/ELF/linkerscript/locationcountererr2.s

4 ↗

(On Diff #152327)

I have no objections using "backslash form" or even maybe writing this echo in a single line,
but best practice is to do as minimum changes as possible in the patch you are posting,
so that you should focus the reviewer's attention on the necessary changes you make and not on the refactorings.

So such changes can be committed as NFC separately if you wish, but not included into patch doing functional change.

test/ELF/linkerscript/no-space.s

6 ↗

(On Diff #152327)

Yes, please.

test/ELF/linkerscript/orphan-first-cmd.test

20 ↗

(On Diff #152327)

Actually, it seems to be the most important line of the test case for me. It shows where the orphan .text section was placed.
Without that line, what this test does/checks in your opinion?

Initial commit message of this test case says: "Don't put an orphan before the first . assignment.
This is an horrible special case, but seems to match bfd's behaviour
and is important for avoiding placing an orphan section before the
expected start of the file."

I think that line checks that behavior.

[MaskRay]

Committed style change separately as grimar suggested. Thanks!

[MaskRay]

Reword description

[MaskRay]

Change comment as echristo suggested

[ruiu]

LGTM

I dug into the history of this file to see why we have this if condition in the first place, and looks like there's no strong reason to do that.

[grimar]

LGTM with 2 nits.

test/ELF/arm-exidx-order.s

1 ↗

(On Diff #152721)

One more place to commit separately.

test/ELF/linkerscript/non-absolute.s

8 ↗

(On Diff #152721)

The calculation is now correct, but I am not sure how it is useful.

What is -0xF here? How is it correlated with the output which is 0xFFFFFFFFFFFFFFF1?
Since there was no such comment originally, I would either remove it or fix so that it becomes useful.

ELF/Writer.cpp

static unsigned getSectionRank(const OutputSection *Sec) {
   // * Writable sections come last, such that .bss lands at the very
   //   end of the last PT_LOAD.
   bool IsExec = Sec->Flags & SHF_EXECINSTR;
   bool IsWrite = Sec->Flags & SHF_WRITE;
 
   if (IsExec) {
     if (IsWrite)
       Rank |= RF_EXEC_WRITE;
-    else if (!Config->SingleRoRx)
+    else
       Rank |= RF_EXEC;
   } else {
     if (IsWrite)
       Rank |= RF_WRITE;
   }
 
   // If we got here we know that both A and B are in the same PT_LOAD.
 

test/ELF/aarch64-thunk-pi.s

  .section .text_low, "ax", %progbits
  .globl low_target
  .type low_target, %function
 low_target:
  // Need thunk to high_target@plt
  bl high_target
  ret
 // CHECK: low_target:
-// CHECK-NEXT:        0:        04 00 00 94     bl      #16
-// CHECK-NEXT:        4:        c0 03 5f d6     ret
+// CHECK-NEXT:        8:        04 00 00 94     bl      #16
+// CHECK-NEXT:        c:        c0 03 5f d6     ret
 
  .hidden low_target2
  .globl low_target2
  .type low_target2, %function
 low_target2:
  // Need thunk to high_target
  bl high_target2
  ret
 // CHECK: low_target2:
-// CHECK-NEXT:        8:        05 00 00 94     bl      #20
-// CHECK-NEXT:        c:        c0 03 5f d6     ret
+// CHECK-NEXT:        0:        05 00 00 94     bl      #20
+// CHECK-NEXT:        4:        c0 03 5f d6     ret
 
 // Expect range extension thunks for .text_low
 // adrp calculation is (PC + signed immediate) & (!0xfff)
 // CHECK: __AArch64ADRPThunk_high_target:
-// CHECK-NEXT:       70:       10 00 08 90     adrp    x16, #268435456
-// CHECK-NEXT:       74:       10 02 03 91     add     x16, x16, #192
-// CHECK-NEXT:       78:       00 02 1f d6     br      x16
+// CHECK-NEXT:       e8:       10 00 08 90     adrp    x16, #268435456
+// CHECK-NEXT:       ec:       10 02 01 91     add     x16, x16, #64
+// CHECK-NEXT:       f0:       00 02 1f d6     br      x16
 // CHECK: __AArch64ADRPThunk_high_target2:
-// CHECK-NEXT:       7c:       10 00 08 90     adrp    x16, #268435456
-// CHECK-NEXT:       80:       10 22 00 91     add     x16, x16, #8
-// CHECK-NEXT:       84:       00 02 1f d6     br      x16
+// CHECK-NEXT:       f4:       10 00 08 90     adrp    x16, #268435456
+// CHECK-NEXT:       f8:       10 22 00 91     add     x16, x16, #8
+// CHECK-NEXT:       fc:       00 02 1f d6     br      x16
 
 
  .section .text_high, "ax", %progbits
  .globl high_target
  .type high_target, %function
 high_target:
  // No thunk needed as we can reach low_target@plt
  bl low_target
  ret
 // CHECK: high_target:
-// CHECK-NEXT: 10000000:        34 00 00 94     bl      #208
+// CHECK-NEXT: 10000000:        14 00 00 94     bl #80
 // CHECK-NEXT: 10000004:        c0 03 5f d6     ret
 
  .hidden high_target2
  .globl high_target2
  .type high_target2, %function
 high_target2:
  // Need thunk to low_target
  bl low_target2
  ret
 // CHECK: high_target2:
 // CHECK-NEXT: 10000008:        02 00 00 94     bl      #8
 // CHECK-NEXT: 1000000c:        c0 03 5f d6     ret
 
 // Expect Thunk for .text.high
 
 // CHECK: __AArch64ADRPThunk_low_target2:
 // CHECK-NEXT: 10000010:  10 00 f8 90   adrp  x16, #-268435456
-// CHECK-NEXT: 10000014:  10 a2 01 91   add x16, x16, #104
+// CHECK-NEXT: 10000014:  10 82 03 91   add x16, x16, #224
 // CHECK-NEXT: 10000018:  00 02 1f d6   br  x16
 
 // CHECK: Disassembly of section .plt:
 // CHECK-NEXT: .plt:
-// CHECK-NEXT: 100000a0:       f0 7b bf a9     stp     x16, x30, [sp, #-16]!
-// CHECK-NEXT: 100000a4:       10 00 00 90     adrp    x16, #0
-// CHECK-NEXT: 100000a8:       11 7a 40 f9     ldr     x17, [x16, #240]
-// CHECK-NEXT: 100000ac:       10 c2 03 91     add     x16, x16, #240
-// CHECK-NEXT: 100000b0:       20 02 1f d6     br      x17
-// CHECK-NEXT: 100000b4:       1f 20 03 d5     nop
-// CHECK-NEXT: 100000b8:       1f 20 03 d5     nop
-// CHECK-NEXT: 100000bc:       1f 20 03 d5     nop
-// CHECK-NEXT: 100000c0:       10 00 00 90     adrp    x16, #0
-// CHECK-NEXT: 100000c4:       11 7e 40 f9     ldr     x17, [x16, #248]
-// CHECK-NEXT: 100000c8:       10 e2 03 91     add     x16, x16, #248
-// CHECK-NEXT: 100000cc:       20 02 1f d6     br      x17
-// CHECK-NEXT: 100000d0:       10 00 00 90     adrp    x16, #0
-// CHECK-NEXT: 100000d4:       11 82 40 f9     ldr     x17, [x16, #256]
-// CHECK-NEXT: 100000d8:       10 02 04 91     add     x16, x16, #256
-// CHECK-NEXT: 100000dc:       20 02 1f d6     br      x17
+// CHECK-NEXT: 10000020:       f0 7b bf a9     stp     x16, x30, [sp, #-16]!
+// CHECK-NEXT: 10000024:       10 00 00 90     adrp    x16, #0
+// CHECK-NEXT: 10000028:       11 3a 40 f9     ldr     x17, [x16, #112]
+// CHECK-NEXT: 1000002c:       10 c2 01 91     add     x16, x16, #112
+// CHECK-NEXT: 10000030:       20 02 1f d6     br      x17
+// CHECK-NEXT: 10000034:       1f 20 03 d5     nop
+// CHECK-NEXT: 10000038:       1f 20 03 d5     nop
+// CHECK-NEXT: 1000003c:       1f 20 03 d5     nop
+// CHECK-NEXT: 10000040:       10 00 00 90     adrp    x16, #0
+// CHECK-NEXT: 10000044:       11 3e 40 f9     ldr     x17, [x16, #120]
+// CHECK-NEXT: 10000048:       10 e2 01 91     add     x16, x16, #120
+// CHECK-NEXT: 1000004c:       20 02 1f d6     br      x17
+// CHECK-NEXT: 10000050:       10 00 00 90     adrp    x16, #0
+// CHECK-NEXT: 10000054:       11 42 40 f9     ldr     x17, [x16, #128]
+// CHECK-NEXT: 10000058:       10 02 02 91     add     x16, x16, #128
+// CHECK-NEXT: 1000005c:       20 02 1f d6     br      x17

test/ELF/synthetic-got.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: echo "SECTIONS { }" > %t0.script
 # RUN: ld.lld --hash-style=sysv -shared %t.o -o %t0.out --script %t0.script
 # RUN: llvm-objdump -section-headers %t0.out | FileCheck %s --check-prefix=GOT
 # RUN: llvm-objdump -s -section=.got -section=.got.plt %t0.out \
 # RUN:   | FileCheck %s --check-prefix=GOTDATA
 
 # GOT:     Sections:
-# GOT:      8  .got.plt     00000020 00000000000000d0 DATA
-# GOT:      10 .got         00000008 00000000000001c0 DATA
+# GOT:      8  .got.plt     00000020 00000000000000e0 DATA
+# GOT:      10 .got         00000008 00000000000001d0 DATA
 # GOTDATA:     Contents of section .got.plt:
-# GOTDATA-NEXT:  00d0 f0000000 00000000 00000000 00000000
-# GOTDATA-NEXT:  00e0 00000000 00000000 c6000000 00000000
+# GOTDATA-NEXT:  00e0 00010000 00000000 00000000 00000000
+# GOTDATA-NEXT:  00f0 00000000 00000000 d6000000 00000000
 # GOTDATA-NEXT: Contents of section .got:
-# GOTDATA-NEXT:  01c0 00000000 00000000
+# GOTDATA-NEXT:  01d0 00000000 00000000
 
 # RUN: echo "SECTIONS { .mygot : { *(.got) *(.got.plt) } }" > %t1.script
 # RUN: ld.lld --hash-style=sysv -shared %t.o -o %t1.out --script %t1.script
 # RUN: llvm-objdump -section-headers %t1.out | FileCheck %s --check-prefix=MYGOT
 # RUN: llvm-objdump -s -section=.mygot %t1.out | FileCheck %s --check-prefix=MYGOTDATA
 
 # MYGOT:     Sections:
-# MYGOT:      8  .mygot     00000028 00000000000000d0 DATA
+# MYGOT:      8  .mygot     00000028 00000000000000e0 DATA
 # MYGOT-NOT:  .got
 # MYGOT-NOT:  .got.plt
-# MYGOTDATA:      00d0 00000000 00000000 f8000000 00000000
-# MYGOTDATA-NEXT: 00e0 00000000 00000000 00000000 00000000
-# MYGOTDATA-NEXT: 00f0 c6000000 00000000
+# MYGOTDATA:      00e0 00000000 00000000 08010000 00000000
+# MYGOTDATA-NEXT: 00f0 00000000 00000000 00000000 00000000
+# MYGOTDATA-NEXT: 0100 d6000000 00000000
 
 mov bar@gotpcrel(%rip), %rax
 call foo@plt

test/ELF/x86-64-retpoline-linkerscript.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/shared.s -o %t2.o
 // RUN: ld.lld -shared %t2.o -o %t2.so
 
 // RUN: echo "SECTIONS { \
 // RUN:   .text : { *(.text) } \
 // RUN:   .plt : { *(.plt) } \
 // RUN:   .got.plt : { *(.got.plt) } \
 // RUN:   .dynstr : { *(.dynstr) } \
 // RUN: }" > %t.script
 // RUN: ld.lld -shared %t1.o %t2.so -o %t.exe -z retpolineplt --script %t.script
 // RUN: llvm-objdump -d -s %t.exe | FileCheck %s
 
 // CHECK:      Disassembly of section .plt:
 // CHECK-NEXT: .plt:
-// CHECK-NEXT: 10:       ff 35 ea 00 00 00       pushq   234(%rip)
-// CHECK-NEXT: 16:       4c 8b 1d eb 00 00 00    movq    235(%rip), %r11
+// CHECK-NEXT: 10:       ff 35 72 00 00 00       pushq   114(%rip)
+// CHECK-NEXT: 16:       4c 8b 1d 73 00 00 00    movq    115(%rip), %r11
 // CHECK-NEXT: 1d:       e8 0e 00 00 00  callq   14 <.plt+0x20>
 // CHECK-NEXT: 22:       f3 90   pause
 // CHECK-NEXT: 24:       0f ae e8        lfence
 // CHECK-NEXT: 27:       eb f9   jmp     -7 <.plt+0x12>
 // CHECK-NEXT: 29:       cc      int3
 // CHECK-NEXT: 2a:       cc      int3
 // CHECK-NEXT: 2b:       cc      int3
 // CHECK-NEXT: 2c:       cc      int3
 // CHECK-NEXT: 2d:       cc      int3
 // CHECK-NEXT: 2e:       cc      int3
 // CHECK-NEXT: 2f:       cc      int3
 // CHECK-NEXT: 30:       4c 89 1c 24     movq    %r11, (%rsp)
 // CHECK-NEXT: 34:       c3      retq
 // CHECK-NEXT: 35:       cc      int3
 // CHECK-NEXT: 36:       cc      int3
 // CHECK-NEXT: 37:       cc      int3
 // CHECK-NEXT: 38:       cc      int3
 // CHECK-NEXT: 39:       cc      int3
 // CHECK-NEXT: 3a:       cc      int3
 // CHECK-NEXT: 3b:       cc      int3
 // CHECK-NEXT: 3c:       cc      int3
 // CHECK-NEXT: 3d:       cc      int3
 // CHECK-NEXT: 3e:       cc      int3
 // CHECK-NEXT: 3f:       cc      int3
-// CHECK-NEXT: 40:       4c 8b 1d c9 00 00 00    movq    201(%rip), %r11
+// CHECK-NEXT: 40:       4c 8b 1d 51 00 00 00    movq    81(%rip), %r11
 // CHECK-NEXT: 47:       e8 e4 ff ff ff  callq   -28 <.plt+0x20>
 // CHECK-NEXT: 4c:       e9 d1 ff ff ff  jmp     -47 <.plt+0x12>
 // CHECK-NEXT: 51:       68 00 00 00 00  pushq   $0
 // CHECK-NEXT: 56:       e9 b5 ff ff ff  jmp     -75 <.plt>
 // CHECK-NEXT: 5b:       cc      int3
 // CHECK-NEXT: 5c:       cc      int3
 // CHECK-NEXT: 5d:       cc      int3
 // CHECK-NEXT: 5e:       cc      int3
 // CHECK-NEXT: 5f:       cc      int3
-// CHECK-NEXT: 60:       4c 8b 1d b1 00 00 00    movq    177(%rip), %r11
+// CHECK-NEXT: 60:       4c 8b 1d 39 00 00 00    movq    57(%rip), %r11
 // CHECK-NEXT: 67:       e8 c4 ff ff ff  callq   -60 <.plt+0x20>
 // CHECK-NEXT: 6c:       e9 b1 ff ff ff  jmp     -79 <.plt+0x12>
 // CHECK-NEXT: 71:       68 01 00 00 00  pushq   $1
 // CHECK-NEXT: 76:       e9 95 ff ff ff  jmp     -107 <.plt>
 // CHECK-NEXT: 7b:       cc      int3
 // CHECK-NEXT: 7c:       cc      int3
 // CHECK-NEXT: 7d:       cc      int3
 // CHECK-NEXT: 7e:       cc      int3
 // CHECK-NEXT: 7f:       cc      int3
 
 .global _start
 _start:
   jmp bar@PLT
   jmp zed@PLT