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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

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.



================
Comment at: test/ELF/arm-exidx-order.s:1
+// REQUIRES: arm
 // RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t
----------------
Can you avoid unnecessary changes in the patches, please?
Moving "requires" is OK, but can be done separately. Such changes makes doing review harder.


================
Comment at: test/ELF/arm-exidx-sentinel-orphan.s:1
+// REQUIRES: arm
 // RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t
----------------
The same.


================
Comment at: test/ELF/arm-plt-reloc.s:165
 // DSORELHIGH: Relocations [
-// DSORELHIGH-NEXT:   Section (6) .rel.plt {
+// DSORELHIGH-NEXT:   Section (4) .rel.plt {
 // DSORELHIGH-NEXT:     0x110000C R_ARM_JUMP_SLOT func1 0x0
----------------
This can be 


```
Section {{.*}} .rel.plt
```


================
Comment at: test/ELF/eh-frame-padding-no-rosegment.s:10
 bar:
+  ret
 
----------------
Why you had to add that?


================
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
 
----------------
Was it necessary change?


================
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
----------------
Why are you changing the way how we writing the script?


================
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
 
----------------
Why are you doing this in this patch?


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


================
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
 
----------------
Again, why? What is wrong with using `%t` and `llvm-readelf`?


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


================
Comment at: test/ELF/linkerscript/sections-keep.s:3
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/keep.s -o %t1.o
 
----------------
Why are you renaming inputs? It brings so many changes to the rest of the test case, please don't do that.


================
Comment at: test/ELF/linkerscript/sort-non-script.s:6
+# RUN: ld.lld --hash-style=sysv -o %t --script %t.script %t.o -shared
+# RUN: llvm-readelf -s %t | FileCheck %s
 
----------------
Same here and below.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48405