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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: test/ELF/eh-frame-padding-no-rosegment.s:10
 bar:
+  ret
 
----------------
grimar wrote:
> Why you had to add that?
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).


================
Comment at: test/ELF/linkerscript/implicit-program-header.test:7
+# RUN: ld.lld --hash-style=sysv -o %t1 --script %s %t.o -shared
+# RUN: llvm-readelf -l %t1 | FileCheck %s
 
----------------
grimar wrote:
> Was it necessary change?
`llvm-readelf` is more common. I happened to change this place so I replaced the command.


================
Comment at: test/ELF/linkerscript/locationcountererr2.s:4
+# RUN: echo "SECTIONS { \
+# RUN: . = 0x150; . = 0x10; .text : {} }" > %t.script
 # RUN: ld.lld %t.o --script %t.script -o %t -shared
----------------
grimar wrote:
> Why are you changing the way how we writing the script?
OK. reverted the change.

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


================
Comment at: test/ELF/linkerscript/no-space.s:6
 # RUN: ld.lld --hash-style=sysv -o %t --script %t.script %t.o -shared
-# RUN: llvm-readobj -elf-output-style=GNU -l %t | FileCheck %s
+# RUN: llvm-readelf -l %t | FileCheck %s
 
----------------
grimar wrote:
> Why are you doing this in this patch?
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 


================
Comment at: test/ELF/linkerscript/non-absolute.s:8
 
+# 0x94 - 169 = B = -0xf
 # DUMP:       Disassembly of section .text:
----------------
grimar wrote:
> This seems wrong
> ```
> 0x94 - 168 != 0xB
> ``` 
Thanks! Fixed.


================
Comment at: test/ELF/linkerscript/non-alloc.s:6
+# RUN: ld.lld --hash-style=sysv -o %t --script %t.script %t.o -shared
+# RUN: llvm-readelf -s -l %t | FileCheck %s
 
----------------
grimar wrote:
> Again, why? What is wrong with using `%t` and `llvm-readelf`?
Nothing wrong but just for consistency.


================
Comment at: test/ELF/linkerscript/orphan-first-cmd.test:20
 # CHECK-NEXT: ]
-# CHECK-NEXT: Address: 0x1038
----------------
grimar wrote:
> Why you removed this?
Added back.

```
# CHECK-NEXT: Address: 0x1070
```

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


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48405