[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.

In https://reviews.llvm.org/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.



================
Comment at: test/ELF/arm-exidx-order.s:1
+// REQUIRES: arm
 // RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t
----------------
MaskRay wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > Can you avoid unnecessary changes in the patches, please?
> > > > Moving "requires" is OK, but can be done separately. Such changes makes doing review harder.
> > > Sorry if it did any *unnecessary* changes. But I don't there any change is refactoring.
> > s/there/think/
> 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)
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.


================
Comment at: test/ELF/eh-frame-padding-no-rosegment.s:10
 bar:
+  ret
 
----------------
MaskRay wrote:
> 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).
Thanks for the explanation, that was no obvious.


================
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
----------------
MaskRay wrote:
> 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.
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.


================
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
 
----------------
MaskRay wrote:
> 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 
Yes, please.


================
Comment at: test/ELF/linkerscript/orphan-first-cmd.test:20
 # CHECK-NEXT: ]
-# CHECK-NEXT: Address: 0x1038
----------------
MaskRay wrote:
> 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)
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.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48405