This is an archive of the discontinued LLVM Phabricator instance.

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

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

Repository
rL LLVM

Event Timeline

rahmanl created this revision.Jul 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.

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

This revision was automatically updated to reflect the committed changes.

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

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

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.

In D65478#1620005, @pcc 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#1620005, @pcc 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.