Page MenuHomePhabricator

[LLD][ELF] .note.gnu.property sections should have wordsize alignment
ClosedPublic

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

Details

Summary

The .note.gnu.property SHT_NOTE sections on AArch64 (a 64-bit target) should have alignment 8 to more closely match the binutils implementation where alignment is 4-bytes on 32-bit machines and 8-bytes on 64-bit machines.

Previously LLD was using 4 for both 32-bit and 64-bit machines.

There was a long discussion on the right alignment of the .note.gnu.property section on the binutils mailing list. The basic argument was that generic ELF requires 8-byte alignment for SHT_NOTES sections, however this hadn't been respected by other GNU notes sections. The implementation in GNU ld uses 8 and as there is no binary legacy of using 4 in LLD (BTI is only just being picked up and used) I'd like to keep LLD in line with GNU ld.

Although at present LLD only does something useful with AArch64 properties, this also applies to X86.

Depends on D70961 as it updates a test added there.

Diff Detail

Event Timeline

peter.smith created this revision.Dec 3 2019, 6:11 AM

Updated to add context.

grimar added inline comments.Dec 3 2019, 6:46 AM
lld/test/ELF/aarch64-gnu-property-align.s
1 ↗(On Diff #231898)

Seems you can use x86 here too?

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

Do we have a test for !config->is64?

[LLD][ELF][AArch64] .note.gnu.property sections should have alignment 8

Mention x86-64 or just delete [AArch64]. I think all 64-bit architectures use 8 (though currently only x86-64 and AArch64 define architecture specific GNU_PROPERTY_*). The code in binutils-gdb is architecture-agnostic bfd/elf-properties.c:_bfd_elf_convert_gnu_properties

bed = get_elf_backend_data (obfd);
align_shift = bed->s->elfclass == ELFCLASS64 ? 3 : 2;

/* Get the output .note.gnu.property section size.  */
size = bfd_section_size (isec->output_section);

/* Update the output .note.gnu.property section alignment.  */
bfd_set_section_alignment (isec->output_section, align_shift);
lld/ELF/SyntheticSections.cpp
302

config->wordsize

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

A !config->is64 test will need GNU_PROPERTY_X86_* on EM_386. There is no GNU_PROPERTY_ARM_*.

peter.smith retitled this revision from [LLD][ELF][AArch64] .note.gnu.property sections should have alignment 8 to [LLD][ELF] .note.gnu.property sections should have wordsize alignment.
peter.smith edited the summary of this revision. (Show Details)

Thanks both for the comments. Updated to use config->wordsize, converted test to x86_64 and added 32-bit x86 test for the alignment 4 case.

MaskRay added inline comments.Dec 3 2019, 10:21 AM
lld/test/ELF/gnu-property-align.s
34

There is no GNU_PROPERTY_X86_FEATURE_1_AND...

@grimar I don't mind keeping this as REQUIRES: aarch64. I think an aarch64 test for 64-bit and an x86 test for 32-bit should be sufficient.

MaskRay accepted this revision.Dec 3 2019, 10:40 AM
MaskRay added inline comments.
lld/test/ELF/gnu-property-align.s
34

Sorry, GNU_PROPERTY_X86_FEATURE_1_AND exists. This looks fine.

This revision is now accepted and ready to land.Dec 3 2019, 10:40 AM
grimar accepted this revision.EditedDec 4 2019, 12:35 AM

LGTM.

lld/test/ELF/gnu-property-align.s
34

I think an aarch64 test for 64-bit and an x86 test for 32-bit should be sufficient.

One of the reasons to use x86 for testing generic features is build bots. x86 is the most popular target,
which is enabled everywhere (or almost everywhere). x86 BB are also faster than others, in case of the breakage
they usually report the issue first.

It is also hard to imagine that for local development x86 target is disabled. Most of LLD tests rely on x86.
While aarch64 can be off.

This revision was automatically updated to reflect the committed changes.