This is an archive of the discontinued LLVM Phabricator instance.

Differential D48405

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

Authored by MaskRay on Jun 20 2018, 4:24 PM.

Details

Reviewers
• espindola
javed.absar
ruiu
grimar
echristo
Commits
rG68fa4e8c3407: [ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used
rL335627: [ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used
rLLD335627: [ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used
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.
Diff Detail
Event Timeline
MaskRay created this revision.Jun 20 2018, 4:24 PM
Herald added a reviewer: • espindola.  ·  View Herald TranscriptJun 20 2018, 4:24 PM
Herald added subscribers: llvm-commits, arichardson, emaste.  ·  View Herald Transcript
MaskRay added a comment.EditedJun 20 2018, 4:32 PM
30+ failing tests because of changed addresses...
MaskRay updated this revision to Diff 152202.Jun 20 2018, 4:59 PM
Update test. 30 more...
Herald added a reviewer: javed.absar.  ·  View Herald TranscriptJun 20 2018, 4:59 PM
Harbormaster completed remote builds in B19555: Diff 152202.Jun 20 2018, 4:59 PM
MaskRay added a child revision: D48406: [ELF] Make non-writable non-executable PROGBITS sections closer to .text.Jun 20 2018, 5:00 PM
MaskRay added reviewers: ruiu, grimar, echristo.
MaskRay updated this revision to Diff 152218.Jun 20 2018, 7:11 PM
Fix some tests
Herald added a subscriber: atanasyan.  ·  View Herald TranscriptJun 20 2018, 7:11 PM
Harbormaster completed remote builds in B19560: Diff 152218.Jun 20 2018, 7:11 PM
MaskRay updated this revision to Diff 152221.Jun 20 2018, 8:13 PM
Update description
Harbormaster completed remote builds in B19561: Diff 152221.Jun 20 2018, 8:13 PM
MaskRay retitled this revision from [ELF] Put .rodata before .text even if --no-rosegment to [ELF] Put .rodata before .text even if --no-rosegment or SECTIONS command is used.Jun 20 2018, 8:14 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay updated this revision to Diff 152222.Jun 20 2018, 9:42 PM
MaskRay edited the summary of this revision. (Show Details)
More
MaskRay updated this revision to Diff 152224.Jun 20 2018, 10:37 PM
Updated all tests
Harbormaster completed remote builds in B19563: Diff 152224.Jun 20 2018, 10:37 PM
atanasyan added inline comments.Jun 20 2018, 11:23 PM
test/ELF/mips-gp-ext.s


38 ↗(On Diff #152224)
Please change in this comment 0x13c to 0x1ec (new _gp value)
MaskRay updated this revision to Diff 152327.Jun 21 2018, 9:40 AM
update _gp in mips-gp-ext.s
Harbormaster completed remote builds in B19587: Diff 152327.Jun 21 2018, 9:40 AM
MaskRay marked an inline comment as done.Jun 21 2018, 9:40 AM
echristo added a comment.Jun 21 2018, 11:27 AM
I think this is obsolete after the other patch?



-eric
MaskRay abandoned this revision.Jun 21 2018, 11:54 AM
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 reclaimed this revision.Jun 21 2018, 11:55 AM
MaskRay retitled this revision from [ELF] Put .rodata before .text even if --no-rosegment or SECTIONS command is used to [ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used.
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 added inline comments.Jun 21 2018, 12:32 PM
ELF/Writer.cpp


767
Can you rebase this on the other patch and then add some comments about the ranking into the source?
MaskRay marked an inline comment as done.Jun 21 2018, 1:22 PM
MaskRay added inline comments.
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 marked an inline comment as done.Jun 21 2018, 1:25 PM
MaskRay added inline comments.
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 added inline comments.Jun 21 2018, 11:53 PM
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 added a comment.Jun 22 2018, 1:07 AM
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 added inline comments.Jun 22 2018, 9:45 AM
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 added inline comments.Jun 22 2018, 9:48 AM
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 added inline comments.Jun 22 2018, 10:03 AM
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 added a comment.EditedJun 22 2018, 10:31 AM
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 added a comment.Jun 25 2018, 2:10 AM


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 updated this revision to Diff 152718.Jun 25 2018, 10:18 AM
Committed style change separately as grimar suggested. Thanks!
MaskRay edited the summary of this revision. (Show Details)Jun 25 2018, 10:25 AM
MaskRay updated this revision to Diff 152720.Jun 25 2018, 10:26 AM
Reword description
Harbormaster completed remote builds in B19687: Diff 152720.Jun 25 2018, 10:26 AM
MaskRay updated this revision to Diff 152721.Jun 25 2018, 10:28 AM
Change comment as echristo suggested
Harbormaster completed remote builds in B19688: Diff 152721.Jun 25 2018, 10:28 AM
MaskRay marked 18 inline comments as done.Jun 25 2018, 10:31 AM
ruiu accepted this revision.Jun 25 2018, 11:42 PM
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.
This revision is now accepted and ready to land.Jun 25 2018, 11:42 PM
grimar accepted this revision.Jun 25 2018, 11:59 PM
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.
Closed by commit rLLD335627: [ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used (authored by MaskRay).  ·  Explain WhyJun 26 2018, 10:09 AM
Closed by commit rL335627: [ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used (authored by MaskRay).  ·  Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.
MaskRay marked an inline comment as not done.Jun 26 2018, 10:10 AM
Revision Contents
PathSize
ELF/
2 lines
Diff 152199
ELF/Writer.cpp
Show First 20 Lines • Show All 758 Lines • ▼ Show 20 Linesstatic unsigned getSectionRank(const OutputSection *Sec) {

  // * Writable sections come last, such that .bss lands at the very
  // * Writable sections come last, such that .bss lands at the very

  //   end of the last PT_LOAD.
  //   end of the last PT_LOAD.

  bool IsExec = Sec->Flags & SHF_EXECINSTR;
  bool IsExec = Sec->Flags & SHF_EXECINSTR;

  bool IsWrite = Sec->Flags & SHF_WRITE;
  bool IsWrite = Sec->Flags & SHF_WRITE;




  if (IsExec) {
  if (IsExec) {

    if (IsWrite)
    if (IsWrite)

      Rank |= RF_EXEC_WRITE;
      Rank |= RF_EXEC_WRITE;

    else if (!Config->SingleRoRx)
    else

echristoUnsubmitted
Done
ReplyInline Actions
Can you rebase this on the other patch and then add some comments about the ranking into the source?
echristo: Can you rebase this on the other patch and then add some comments about the ranking into the…
MaskRayAuthorUnsubmitted
Done
ReplyInline Actions
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: D48406 is based on this one (which I'd like to collect some feedback about section reordering…
MaskRayAuthorUnsubmitted
Done
ReplyInline Actions
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).
MaskRay: This patch merely removes code (the  `if (!Config->SingleRoRx)` condition). It is fairly…
echristoUnsubmitted
Done
ReplyInline Actions
I agree with all of that. I'm still pretty sure that the comment above it needs to be updated :)
echristo: I agree with all of that. I'm still pretty sure that the comment above it needs to be updated :)
      Rank |= RF_EXEC;
      Rank |= RF_EXEC;

  } else {
  } else {

    if (IsWrite)
    if (IsWrite)

      Rank |= RF_WRITE;
      Rank |= RF_WRITE;

  }
  }




  // If we got here we know that both A and B are in the same PT_LOAD.
  // If we got here we know that both A and B are in the same PT_LOAD.




▲ Show 20 Lines • Show All 1,602 Lines • Show Last 20 Lines