Page MenuHomePhabricator

[ELF] Allow placing non-string SHF_MERGE sections with different alignments into the same MergeSyntheticSection
ClosedPublic

Authored by MaskRay on Jul 4 2019, 3:21 AM.

Details

Summary

The difference from D63432/r365015 is that this patch does not place
SHF_STRINGS sections with different alignments into the same
MergeSyntheticSection. That would:

(1) create unnecessary padding and thus waste space
(2) create unaligned sections when tail merge (-O2) is enabled.

The alignment of MergeTailAlignment::Builder was out of sync in D63432.
MOVAPS on such unaligned strings can raise SIGSEGV.

This should fix PR42289: the Linux kernel has a use case that input
files have .rodata.cst32 sections with different alignments. The
expectation (and what ld.bfd and gold do) is that in the -r link, there
is only one .rodata.cst32 (SHF_MERGE sections with different alignments
can be combined), but lld currently creates one for each different
alignment.

The current merging strategy:

  1. Group SHF_MERGE sections by (name, sh_flags, sh_entsize and sh_addralign). String merging is performed among a group, even if -O0 is specified.
  2. Create one output section for each group. This is a special case in addInputSec().

This patch changes 1) to:

  1. Group SHF_MERGE sections by (name, sh_flags, sh_entsize). String merging is performed among a group, even if -O0 is specified.

We will thus create just one .rodata.cst32 . This also improves merging
efficiency when sections with the same name but different alignments are
combined.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jul 4 2019, 3:21 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay updated this revision to Diff 208009.Jul 4 2019, 3:22 AM
MaskRay added a reviewer: nickdesaulniers.

Add nickdesaulniers

Harbormaster completed remote builds in B34373: Diff 208009.

That would (1) waste space and (2) create unaligned sections when tail merge (-O2) is enabled. MOVAPS on such unaligned strings can raise SIGSEGV.

I haven't figured out the root cause of (2) yet.

(1) justifies a change, otherwise we will get something like a.......b.......c......., which wastes space.

That would (1) waste space and (2) create unaligned sections when tail merge (-O2) is enabled. MOVAPS on such unaligned strings can raise SIGSEGV.

I haven't figured out the root cause of (2) yet.

(1) justifies a change, otherwise we will get something like a.......b.......c......., which wastes space.

I hadn't realised that the change would also affect SHF_STRINGS sections as well, IIRC sh_entsize means the size of the character so it can make sense for the alignment to be higher than the sh_entsize. This is bringing back memories. Many years ago we had a similar problem mixing objects from Arm's proprietary compiler and arm-none-eabi-gcc. The ARM compiler only used SHF_STRINGS with sh_align 1 with the code using byte by byte accesses to those strings. In arm-none-eabi-gcc the SHF_STRINGS has sh_align 4 and copied strings using words instead of bytes. Within the SHF_STRINGS section the strings were padded to a 4-byte boundary. On the ARM processors of the time didn't support unaligned accesses so when the strings were merged without taking into account alignment padding the resulting binary crashed.

In that particular case we permitted strings from an equal to or lower alignment to be merged, but not a higher to a lower alignment. Within that higher aligned string section we needed to maintain alignment padding to ensure each string started at an aligned boundary. This might be more difficult to do in LLD.

I think for now excluding SHF_STRINGS from different alignment merging is the right thing to do. If we want to enable it we'll need to ensure each string starts on a correctly aligned boundary to ensure correctness.

I just did a quick test of this patch and now stage two testing works (Fedora 30, x86-64)

MaskRay updated this revision to Diff 208022.Jul 4 2019, 6:04 AM
MaskRay edited the summary of this revision. (Show Details)

Update description

The root cause is clear now.

MergeTailSection::MergeTailSection(StringRef Name, uint32_t Type,
                                   uint64_t Flags, uint32_t Alignment)
    : MergeSyntheticSection(Name, Type, Flags, Alignment),
      Builder(StringTableBuilder::RAW, Alignment) {}

Builder::Alignment was out of sync if we update MergeTailSection::Alignment.

That would (1) waste space and (2) create unaligned sections when tail merge (-O2) is enabled. MOVAPS on such unaligned strings can raise SIGSEGV.

I haven't figured out the root cause of (2) yet.

(1) justifies a change, otherwise we will get something like a.......b.......c......., which wastes space.

I hadn't realised that the change would also affect SHF_STRINGS sections as well, IIRC sh_entsize means the size of the character so it can make sense for the alignment to be higher than the sh_entsize. This is bringing back memories. Many years ago we had a similar problem mixing objects from Arm's proprietary compiler and arm-none-eabi-gcc. The ARM compiler only used SHF_STRINGS with sh_align 1 with the code using byte by byte accesses to those strings. In arm-none-eabi-gcc the SHF_STRINGS has sh_align 4 and copied strings using words instead of bytes. Within the SHF_STRINGS section the strings were padded to a 4-byte boundary. On the ARM processors of the time didn't support unaligned accesses so when the strings were merged without taking into account alignment padding the resulting binary crashed.

D63432 had the very issue. A MergeTailSection was created with alignment 1. In its ctor the StringTableBuilder was initialized with alignment 1. Then, the MergeTailSection's alignment was updated to 16 but StringTableBuilder::Alignment was not updated. StringTableBuilder actually can handle strings with alignment greater than 1 (though some strings may not be mergeable, e.g. "abc" and "bc" can be merged if sh_addralign is 1 but can't if sh_addralign is 2).

In that particular case we permitted strings from an equal to or lower alignment to be merged, but not a higher to a lower alignment. Within that higher aligned string section we needed to maintain alignment padding to ensure each string started at an aligned boundary. This might be more difficult to do in LLD.

Agreed.

I think for now excluding SHF_STRINGS from different alignment merging is the right thing to do. If we want to enable it we'll need to ensure each string starts on a correctly aligned boundary to ensure correctness.

I agree. tail-merge-string-align2.s demonstrates that different alignments on SHF_STRINGS can create too much padding.

@davezarzycki Thank you for testing!

peter.smith accepted this revision.Jul 4 2019, 6:21 AM

Based on the closeness to D63432 and Dave's testing LGTM.

This revision is now accepted and ready to land.Jul 4 2019, 6:21 AM
This revision was automatically updated to reflect the committed changes.