Page MenuHomePhabricator

[LLD][ELF] add support for PT_GNU_PROPERTY
ClosedPublic

Authored by peter.smith on Dec 3 2019, 6:02 AM.

Details

Summary

The PT_GNU_PROPERTY program header describes the location of the .note.gnu.property SHT_NOTES section. The linux kernel uses this program header to find the .note.gnu.property section rather than parsing. Executables that have properties that the kernel needs to act on that don't have the PT_GNU_PROPERTY program header will not boot.

The change to add the program header is simple. It shifts the addresses around in the existing .note.gnu.property tests, but otherwise has no effect.

Depends on D70959 for definition of PT_GNU_PROPERTY and llvm-readobj support.

Diff Detail

Event Timeline

peter.smith created this revision.Dec 3 2019, 6:02 AM
Herald added a project: Restricted Project. · View Herald Transcript

Updated to add context to diffs.

grimar added a comment.Dec 3 2019, 6:42 AM

The logic fine to me. A few comments/suggestions are inlined.

lld/test/ELF/aarch64-pt-gnu-property.s
1 ↗(On Diff #231889)

I think you can use x86 for this test.

6 ↗(On Diff #231889)

I think in LLD we also using ## instead of # for comments nowadays.

30 ↗(On Diff #231889)

Please remove the excessive identation.

38 ↗(On Diff #231889)

I'd suggest to be consistent and use "##" for comments too.

peter.smith marked 3 inline comments as done.

Thanks for the comments. Converted test to x86_64, removed indendation and fixed comments.

The code change looks good. I only get some nits about the tests.

lld/test/ELF/aarch64-feature-bti.s
66

Recently I think we can probably keep the address for the first instruction of a basic block and omit addresses for the rest of instructions.

10360: bti c
       stp x16, x30, [sp, #-16]!
       adrp    x16, #131072

Addresses of instructions that are neither the first instructions nor jump targets are not useful, and can make address updating annoying.

lld/test/ELF/aarch64-pt-gnu-property.s
14 ↗(On Diff #231899)

Address: [[ADDR:0x.*]]

Then below, VirtualAddress: [[ADDR]]

21 ↗(On Diff #231899)

We should probably also test PT_NOTE, which has the same offset/size.

1 ↗(On Diff #231889)

Eventually we may need both. I think we also need a similar x86-64-pt-gnu-property.s.

MaskRay added inline comments.Dec 3 2019, 10:48 AM
lld/test/ELF/pt-gnu-property.s
7

section -> segment type

peter.smith marked 5 inline comments as done.Dec 4 2019, 2:22 AM

Thanks for the comments I'll upload a new version shortly.

lld/test/ELF/aarch64-feature-bti.s
66

I think that is a good idea. I've updated the tests to display only the address of the first instruction.

lld/test/ELF/aarch64-pt-gnu-property.s
1 ↗(On Diff #231889)

I've changed to use x86. I think the benefit of having both x86 and aarch64 is marginal right now if we are just testing that PT_GNU_PROPERTY describes the range of .note.gnu.property. I can add a new one if preferred.

New diff not uploaded yet?

lld/test/ELF/aarch64-pt-gnu-property.s
1 ↗(On Diff #231889)

If you think x86 is sufficient, I am fine with it.

peter.smith marked an inline comment as done.

Uploading diff, I could have sworn I did this early today. Maybe I forgot to press something.

MaskRay accepted this revision.Dec 4 2019, 10:09 AM
This revision is now accepted and ready to land.Dec 4 2019, 10:09 AM
grimar accepted this revision.Dec 4 2019, 11:29 PM

LGTM too.

This revision was automatically updated to reflect the committed changes.