Page MenuHomePhabricator

[ELF] Sort notes by alignment in decreasing order
AbandonedPublic

Authored by phosek on Mar 7 2019, 5:08 PM.

Details

Summary

This change implements the solution to PR41000 proposed in the bug
by sorting notes by alignment in decreasing order to close the gap
that might occur when notes use different alignments and to ensure
that PT_NOTE is packed correctly.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

phosek created this revision.Mar 7 2019, 5:08 PM

Would be nice to have a test where 2 sections are out of order, and this change puts them into the correct order. It's not clear from this if the build ID section is naturally placed at the end or not.

phosek added a comment.Mar 7 2019, 5:42 PM

Would be nice to have a test where 2 sections are out of order, and this change puts them into the correct order. It's not clear from this if the build ID section is naturally placed at the end or not.

The build ID test already does the re-ordering so it acts as a test, but I can also make another one if you prefer.

pcc added a subscriber: pcc.Mar 7 2019, 6:03 PM

An alternative solution might be to create a PT_NOTE for each note upfront, and opportunistically merge them if they end up being laid out contiguously. This would also handle the case where a note's size is not a multiple of its alignment.

Peter's suggestion resembles the approach taken by ld.bfd: it can create multiple PT_NOTE segments. I also favor this.

if just .note.gnu.property require 64-bit alignment, I think it time to change the alignment of this section.

I think we need change the .note.gnu.property section alignment to 4-byte in Clang-TableGen, because all its element is 4-byte required now. 
The old ABI required some of note.gnu.property' elements be 8-byte aligned in 64-bit machine, so the section required 8-byte alignment.
Last year, I found a bug about this alignment between GCC and CLANG, Then I discussed with the ABI designer and changed its element's alignment in https://reviews.llvm.org/D56080 .
I think there are already no ‘significance’ of 8 byte alignment for this section, we can change it now in LLVM.

I personally favour a separate PT_NOTE section per alignment, that gives a note parser simple rule to iterate through the section (the p_align field). I think ordering by decreasing alignment will work in practice for the note sections that we have now, but it may not be in the future. I also think that this is worth doing even if clang is changed to generate .note.gnu.property sections that are 4-byte aligned; clang will often use libraries compiled by gcc (or some other compiler) that does use 8-byte aligned sections.

There is a long glibc thread that covers a similar discussion in the gnu community https://sourceware.org/ml/libc-alpha/2018-08/msg00340.html , it is quite long so I've pulled out a few bits of it here:

From what I can tell:

  • ld.bfd outputs a separate PT_NOTE per p_align.
  • ld.gold ignores the 8-byte alignment of the .note.gnu.property in the input object and changes it to 4 so that only one PT_NOTE is required.
  • glibc works for both, given that .note.gnu.property doesn't strictly need 8 byte alignment.

ld.bfd outputs a separate PT_NOTE per p_align.

No, it merges adjacent SHT_NOTE in some circumstances. The rule is complicated, though.

We can place adjacent SHT_NOTE|SHF_ALLOC sections with the same section alignment into the same PT_NOTE segment, as what the ld.bfd patch does
https://sourceware.org/git/?p=binutils-gdb.git;a=blobdiff;f=bfd/elf.c;h=8850efe20e9ea5c9f59bac839cf453790594b6af;hp=5320ae2237fcb67e39c7fd9f47a8ce81338d2aa1;hb=23e463ed7c0d289e2291aaefd576bf02efd98df8;hpb=e66cfcef729d758a5e605d57600530ad9b1bb9c3

I'm not sure if a separate PT_NOTE section per alignment would work for all cases. I don't think the size of a note section with 8-byte alignment is guaranteed to be a multiple of 8 byte.

I believe valid SHF_NOTE sections are either 4-byte or 8-byte aligned and the size is guaranteed to be a multiple of its section alignment. The input is broken if it isn't. It doesn't look wrong for me if lld produces a PT_NOTE that cannot be parsed by ld.so or other tools. Such check may still be nice to have, though.

.section .note.a, "a", @note
.p2align 2
.space 1

.section .note.b, "a", @note
.p2align 2
.space 2

In this example, .note.a's size is not aligned but ld.bfd places .note.a .note.b in the same PT_NOTE segment.

ruiu added a comment.Mar 13 2019, 4:13 PM

At this point I'm wonder if we can simply emit one PT_NOTE for each .note section. Guys, what do you think?

Both choices look good to me:

  • emit one PT_NOTE for each SHF_ALLOC .note*
  • emit one PT_NOTE for adjacent SHF_ALLOC .note* with the same alignment (don't check alignment of their sizes)

Both choices look good to me:

  • emit one PT_NOTE for each SHF_ALLOC .note*
  • emit one PT_NOTE for adjacent SHF_ALLOC .note* with the same alignment (don't check alignment of their sizes)

I agree. I think either of those options will work.

phosek abandoned this revision.Apr 1 2019, 7:35 PM

Superseded by D59531.