This is an archive of the discontinued LLVM Phabricator instance.

[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

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.

Event Timeline

MaskRay created this revision.Jun 20 2018, 4:24 PM
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...

MaskRay updated this revision to Diff 152218.Jun 20 2018, 7:11 PM

Fix some tests

MaskRay updated this revision to Diff 152221.Jun 20 2018, 8:13 PM

Update description

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

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

MaskRay marked an inline comment as done.Jun 21 2018, 9:40 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 :)

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! 😼)

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

MaskRay updated this revision to Diff 152721.Jun 25 2018, 10:28 AM

Change comment as echristo suggested

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.

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