This is an archive of the discontinued LLVM Phabricator instance.

Differential D65478

Create unique, but identically-named ELF sections for explicitly-sectioned functions and globals when using -function-sections and -data-sections.
ClosedPublic

Authored by rahmanl on Jul 30 2019, 2:25 PM.

Details

Reviewers
eugenis
tmsriram
pcc
Commits
rGfbc563e2cb6c: Create unique, but identically-named ELF sections for explicitly-sectioned…
rL367501: Create unique, but identically-named ELF sections for explicitly-sectioned…
Summary

This allows functions and globals to to be reordered later in the linking phase (using the -symbol-ordering-file) even though reordering will be limited to the scope of the explicit section.

Diff Detail

Event Timeline

rahmanl created this revision.Jul 30 2019, 2:25 PM
Herald added a subscriber: llvm-commits. · View Herald TranscriptJul 30 2019, 2:25 PM
pcc added a subscriber: pcc.Jul 30 2019, 2:38 PM

I am in favour of this change.

Should we do the same for globals+data sections?

lib/CodeGen/TargetLoweringObjectFileImpl.cpp
596 ↗(On Diff #212437)

clang-format

test/CodeGen/X86/explicit-function-sections.ll
20 ↗(On Diff #212437)

Shouldn't there be a ,unique,1 here?

rahmanl updated this revision to Diff 212486.Jul 30 2019, 6:05 PM
rahmanl retitled this revision from Create unique, but identically-named ELF sections for explicitly-sectioned functions when using -function-sections. to Create unique, but identically-named ELF sections for explicitly-sectioned functions and globals when using -function-sections and -data-sections..
rahmanl edited the summary of this revision. (Show Details)

Updated the patch to handle data sections as well (thanks to @pcc for the suggestion).

rahmanl updated this revision to Diff 212614.Jul 31 2019, 10:05 AM

Fix regular expression in explicit-elf-sections.ll test.

pcc added a comment.Jul 31 2019, 1:11 PM

Looks like you uploaded a diff without the code changes.

rahmanl updated this revision to Diff 212663.Jul 31 2019, 2:10 PM
pcc accepted this revision.Jul 31 2019, 3:54 PM

LGTM

lib/CodeGen/TargetLoweringObjectFileImpl.cpp
591 ↗(On Diff #212663)

nit: s/section/sections/g

610 ↗(On Diff #212663)

Remove braces

This revision is now accepted and ready to land.Jul 31 2019, 3:54 PM
rahmanl updated this revision to Diff 212704.Jul 31 2019, 4:41 PM

Fixed the comment and removed the braces.

rahmanl added a comment.Jul 31 2019, 4:48 PM

Thanks for the LGTM @ppc
Please go ahead and commit this change as I don't have commit access yet.
Differential Revision: https://reviews.llvm.org/D65478

Closed by commit rL367501: Create unique, but identically-named ELF sections for explicitly-sectioned… (authored by pcc). · Explain WhyJul 31 2019, 6:38 PM
This revision was automatically updated to reflect the committed changes.
inglorion added a subscriber: inglorion.Aug 7 2019, 1:29 PM

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

rahmanl added a comment.EditedAug 7 2019, 3:19 PM
In D65478#1619713, @inglorion wrote:

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

Hi Bob,
We plan to gate this feature behind a condition: D65837
I think in the Chromium case, what it needs is the -funique-section-names. More precisely, Chromium needs this flag only for the explicitly defined section "protected_memory", but I don't think it hurts to do it globally ( Chromium build may actually be using this flag anyways. Please confirm.)
If you think that the chromium build could use this flag, I will change D65837 to do the check.

pcc added a comment.Aug 7 2019, 3:24 PM
In D65478#1619990, @rahmanl wrote:
In D65478#1619713, @inglorion wrote:

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

Hi Bob,
We plan to gate this feature behind a condition: D65837
I think in the Chromium case, what it needs is the -funique-section-names. More precisely, Chromium needs this flag only for the explicitly defined section "protected_memory", but I don't think it hurts to do it globally ( Chromium build may actually be using this flag anyways. Please confirm.)
If you think that the chromium build could use this flag, I will change D65837 to do the check.

Chromium is depending on an implementation detail here and the revert is temporary until we can land a compiler feature that will let us avoid the implementation detail dependence. It doesn't seem necessary to change the condition that you've already implemented in D65837.

rahmanl added a comment.Aug 7 2019, 3:45 PM
In D65478#1620005, @pcc wrote:
In D65478#1619990, @rahmanl wrote:
In D65478#1619713, @inglorion wrote:

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

Hi Bob,
We plan to gate this feature behind a condition: D65837
I think in the Chromium case, what it needs is the -funique-section-names. More precisely, Chromium needs this flag only for the explicitly defined section "protected_memory", but I don't think it hurts to do it globally ( Chromium build may actually be using this flag anyways. Please confirm.)
If you think that the chromium build could use this flag, I will change D65837 to do the check.

Chromium is depending on an implementation detail here and the revert is temporary until we can land a compiler feature that will let us avoid the implementation detail dependence. It doesn't seem necessary to change the condition that you've already implemented in D65837.

Agreed.
However, I was thinking that the implementation detail relies on the assumption that only one section exists of the name "protected_memory" and if we somehow let the programmer specify that a particular section name can only appear once, it would resolve the problem. Enforcing the use of funique-section-names does look overkill.

pcc added a comment.Aug 7 2019, 3:50 PM
In D65478#1620027, @rahmanl wrote:
In D65478#1620005, @pcc wrote:
In D65478#1619990, @rahmanl wrote:
In D65478#1619713, @inglorion wrote:

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

Hi Bob,
We plan to gate this feature behind a condition: D65837
I think in the Chromium case, what it needs is the -funique-section-names. More precisely, Chromium needs this flag only for the explicitly defined section "protected_memory", but I don't think it hurts to do it globally ( Chromium build may actually be using this flag anyways. Please confirm.)
If you think that the chromium build could use this flag, I will change D65837 to do the check.

Chromium is depending on an implementation detail here and the revert is temporary until we can land a compiler feature that will let us avoid the implementation detail dependence. It doesn't seem necessary to change the condition that you've already implemented in D65837.

Agreed.
However, I was thinking that the implementation detail relies on the assumption that only one section exists of the name "protected_memory" and if we somehow let the programmer specify that a particular section name can only appear once, it would resolve the problem. Enforcing the use of funique-section-names does look overkill.

Yeah, I feel like we should avoid letting this be toggleable though unless we really need to, and if we do add a toggle for it, it should be a separate flag from -funique-section-names.

Revision Contents

PathSize
test/
CodeGen/
X86/
16 lines

Diff 212614

test/CodeGen/X86/explicit-elf-sections.ll

Show All 32 Lines
; NSECTIONS-NOT: {{\.section|\.text|\.data}} ; NSECTIONS-NOT: {{\.section|\.text|\.data}}
; NSECTIONS: z: ; NSECTIONS: z:
; NSECTIONS: .section aaa,"ax",@progbits ; NSECTIONS: .section aaa,"ax",@progbits
; NSECTIONS-NOT: {{\.section|\.text|\.data}} ; NSECTIONS-NOT: {{\.section|\.text|\.data}}
; NSECTIONS: w: ; NSECTIONS: w:
; SECTIONS: .section aaa,"ax",@progbits,unique,1 ; SECTIONS: .section aaa,"ax",@progbits,unique,1
; SECTIONS-NOT: {{.section|\.text|\.data}} ; SECTIONS-NOT: {{\.section|\.text|\.data}}
; SECTIONS: f: ; SECTIONS: f:
; SECTIONS: .section aaa,"ax",@progbits,unique,2 ; SECTIONS: .section aaa,"ax",@progbits,unique,2
; SECTIONS-NOT: {{.section|\.text|\.data}} ; SECTIONS-NOT: {{\.section|\.text|\.data}}
; SECTIONS: g: ; SECTIONS: g:
; SECTIONS: .section .text.h,"ax",@progbits ; SECTIONS: .section .text.h,"ax",@progbits
; SECTIONS-NOT: {{.section|\.text|\.data}} ; SECTIONS-NOT: {{\.section|\.text|\.data}}
; SECTIONS: h: ; SECTIONS: h:
; SECTIONS: .section aaa,"ax",@progbits,unique,3 ; SECTIONS: .section aaa,"ax",@progbits,unique,3
; SECTIONS-NOT: {{.section|\.text|\.data}} ; SECTIONS-NOT: {{\.section|\.text|\.data}}
; SECTIONS: i: ; SECTIONS: i:
; SECTIONS: .section aaa,"aw",@progbits,unique,4 ; SECTIONS: .section aaa,"aw",@progbits,unique,4
; SECTIONS-NOT: {{.section|\.text|\.data}} ; SECTIONS-NOT: {{\.section|\.text|\.data}}
; SECTIONS: x: ; SECTIONS: x:
; SECTIONS: .section aaa,"aw",@progbits,unique,5 ; SECTIONS: .section aaa,"aw",@progbits,unique,5
; SECTIONS-NOT: {{.section|\.text|\.data}} ; SECTIONS-NOT: {{\.section|\.text|\.data}}
; SECTIONS: y: ; SECTIONS: y:
; SECTIONS: .section .data.z,"aw",@progbits ; SECTIONS: .section .data.z,"aw",@progbits
; SECTIONS-NOT: {{.section|\.text|\.data}} ; SECTIONS-NOT: {{\.section|\.text|\.data}}
; SECTIONS: z: ; SECTIONS: z:
; SECTIONS: .section aaa,"aw",@progbits,unique,6 ; SECTIONS: .section aaa,"aw",@progbits,unique,6
; SECTIONS-NOT: {{.section|\.text|\.data}} ; SECTIONS-NOT: {{\.section|\.text|\.data}}
; SECTIONS: w: ; SECTIONS: w: