This is an archive of the discontinued LLVM Phabricator instance.

Differential D63432

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

Authored by MaskRay on Jun 17 2019, 8:07 AM.

Details

Reviewers
nickdesaulniers
peter.smith
ruiu
• espindola
grimar
Commits
rG347692e2dedb: [ELF] Allow placing SHF_MERGE sections with different alignments into the same…
rLLD365015: [ELF] Allow placing SHF_MERGE sections with different alignments into the same…
rL365015: [ELF] Allow placing SHF_MERGE sections with different alignments into the same…
Summary

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 different alignments are combined.

Diff Detail

Event Timeline

MaskRay created this revision.Jun 17 2019, 8:07 AM
Herald added a reviewer: • espindola. · View Herald TranscriptJun 17 2019, 8:07 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, arichardson, emaste. · View Herald Transcript
Harbormaster completed remote builds in B33480: Diff 205076.Jun 17 2019, 8:08 AM
MaskRay planned changes to this revision.Jun 17 2019, 8:21 AM

Contents shouldn't be merged...

MaskRay added a comment.Jun 17 2019, 9:17 AM

I'll have to study several SHF_MERGE related changes closely, namely: D25066, D40026 (PR35223), and rLLD328640.

nickdesaulniers added a comment.Jun 17 2019, 11:12 AM

Thanks for taking a look @MaskRay ! From this thread from our mailing list, I think that the different entity size and alignment may be a bug: https://groups.google.com/forum/#!topic/clang-built-linux/0V6APpBcl3k

MaskRay updated this revision to Diff 205236.Jun 17 2019, 7:30 PM
MaskRay edited the summary of this revision. (Show Details)

Update description. add tests

Harbormaster completed remote builds in B33521: Diff 205236.Jun 17 2019, 7:30 PM
MaskRay updated this revision to Diff 205256.Jun 17 2019, 10:31 PM

Improve the test merge-reloc-entsize.s

MaskRay added a parent revision: D63475: [llvm-readobj] Allow --hex-dump/--string-dump to dump multiple sections.Jun 17 2019, 10:31 PM
Harbormaster completed remote builds in B33534: Diff 205256.Jun 17 2019, 10:34 PM
MaskRay added a comment.Jun 17 2019, 10:35 PM

@nickdesaulniers I think this change is still worth making no matter if the Linux kernel use case is a bug or not.

Suppose sh_name,sh_flags,sh_entsize are all the same, for two sections with different sh_addralign, we currently don't combine them. However, I can't find anything that disallows the merge. The -r result will eventually be used as a -no-pie/-pie/-shared link, this patch allows us to merge potentially more strings.

MaskRay added a reviewer: grimar.Jun 18 2019, 12:46 AM
peter.smith added a comment.Jun 18 2019, 2:05 AM

A small nit in the test, but otherwise looks good to me.

test/ELF/merge-reloc-entsize.s
13 ↗(On Diff #205256)

These appear to be unused. I think readelf -x .cst may not be the best here as it only outputs the first, and includes file padding for alignment. Perhaps llvm-objdump -s which gives:

Contents of section .cst:
 0000 01000000 00000000 02000000
Contents of section .cst:
 0000 01000000 00000000 03000000 00000000
MaskRay marked an inline comment as done.Jun 18 2019, 2:41 AM
MaskRay added inline comments.
test/ELF/merge-reloc-entsize.s
13 ↗(On Diff #205256)

Thanks! I noticed this issue (llvm-readelf -x does not dump all sections named .cst). Created D63475 to fix that.

I forgot to add back the deleted llvm-readelf -x .cst RUN line, will add it back in a minute.

MaskRay updated this revision to Diff 205283.Jun 18 2019, 2:41 AM

Add llvm-readelf -x .cst to merge-reloc-entsize.s

Harbormaster completed remote builds in B33542: Diff 205283.Jun 18 2019, 2:43 AM
grimar added a comment.Jun 18 2019, 2:48 AM

I also do not see anything wrong with it. Lets see what Rui think.

MaskRay added a comment.Jun 23 2019, 10:46 PM

🤡

MaskRay updated this revision to Diff 206159.Jun 23 2019, 11:01 PM

merge-reloc-align.s -> merge-align2.s
merge-reloc-entsize.s -> merge-entsize2.s

The tests are useful for non -r link.

Harbormaster completed remote builds in B33765: Diff 206159.Jun 23 2019, 11:04 PM
MaskRay updated this revision to Diff 206160.Jun 23 2019, 11:06 PM

Improve comments

Harbormaster completed remote builds in B33766: Diff 206160.Jun 23 2019, 11:07 PM
MaskRay added a comment.Jun 27 2019, 4:39 AM

🤔

MaskRay added a comment.Jul 2 2019, 7:09 PM

🤖: ping

peter.smith added a comment.Jul 3 2019, 2:34 AM
In D63432#1567744, @MaskRay wrote:

🤖: ping

I'm happy with the change as it should improve the efficiency of merging.

ruiu accepted this revision.Jul 3 2019, 2:45 AM

LGTM

Sorry for the belated review.

This revision is now accepted and ready to land.Jul 3 2019, 2:45 AM
MaskRay updated this revision to Diff 207742.Jul 3 2019, 3:01 AM
MaskRay edited the summary of this revision. (Show Details)

Update description

Harbormaster completed remote builds in B34262: Diff 207742.Jul 3 2019, 3:03 AM
Closed by commit rL365015: [ELF] Allow placing SHF_MERGE sections with different alignments into the same… (authored by MaskRay). · Explain WhyJul 3 2019, 3:04 AM
This revision was automatically updated to reflect the committed changes.
davezarzycki added a subscriber: davezarzycki.Jul 3 2019, 6:03 AM

This change breaks a ton of clang tests, albeit in the Swift fork of clang. Was this expected? If not, can we revert this for now?

I've confirmed that reverting this change allows top-of-tree LLVM+clang+lld to build/test top-of-tree Swift (with forks of llvm, clang, etc)

davezarzycki added a comment.Jul 3 2019, 6:23 AM

Actually, the bug is narrower and doesn't require Swift (my build-and-test script placed the blame incorrectly). One just needs to build llvm+clang+lld after this change, then the following tests fail:

Failing Tests (95):

  LLVM-Unit :: ADT/./ADTTests/APFloatTest.SemanticsDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringDecimalDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringDecimalSignificandDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringHexadecimalDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringHexadecimalExponentDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringHexadecimalSignificandDeath
  LLVM-Unit :: ADT/./ADTTests/APIntTest.StringDeath
  LLVM-Unit :: ADT/./ADTTests/APSIntTest.StringDeath
  LLVM-Unit :: ADT/./ADTTests/AnyTest.BadAnyCast
  LLVM-Unit :: ADT/./ADTTests/BumpPtrListTest.resetAlloc
  LLVM-Unit :: ADT/./ADTTests/FallibleIteratorTest.RegularLoopExitRequiresErrorCheck
  LLVM-Unit :: ADT/./ADTTests/PackedVectorTest.SignedValues
  LLVM-Unit :: ADT/./ADTTests/PackedVectorTest.UnsignedValues
  LLVM-Unit :: ADT/./ADTTests/STLExtrasTest.EarlyIncrementTest
  LLVM-Unit :: AsmParser/./AsmParserTests/AsmParserTest.NonNullTerminatedInput
  LLVM-Unit :: BinaryFormat/./BinaryFormatTests/MsgPackWriter.TestWriteCompatibleNoBin
  LLVM-Unit :: CodeGen/./CodeGenTests/LowLevelTypeTest.ChangeElementTypeDeath
  LLVM-Unit :: CodeGen/./CodeGenTests/MachineInstrBundleIteratorTest.CheckForBundles
  LLVM-Unit :: IR/./IRTests/ConstantsTest.ReplaceWithConstantTest
  LLVM-Unit :: IR/./IRTests/GlobalTest.AlignDeath
  LLVM-Unit :: IR/./IRTests/ValueHandle.AssertingVH_Asserts
  LLVM-Unit :: IR/./IRTests/ValueHandle.PoisoningVH_Asserts
  LLVM-Unit :: IR/./IRTests/ValueHandle.TrackingVH_Asserts
  LLVM-Unit :: IR/./IRTests/ValueTest.getLocalSlotDeath
  LLVM-Unit :: Support/./SupportTests/CommandLineTest.TokenizeWindowsCommandLine2
  LLVM-Unit :: Transforms/Utils/./UtilsTests/ValueMapperTest.mapMetadataLocalAsMetadata
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.FillMemoryOperands_ADD64rm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.FillMemoryOperands_VGATHERDPSZ128rm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.SetRegToVR128Value_Use_VMOVDQU32Z128rm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.SetRegToVR256Value_Use_VMOVDQU32Z256rm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.SetRegToVR512Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2AvxTargetTest.SetRegToVR128Value_Use_VMOVDQUrm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2AvxTargetTest.SetRegToVR256Value_Use_VMOVDQUYrm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.NoHighByteRegs
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetFlags
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToFP0_80Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToFP1_32Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToFP1_4Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToGR16Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToGR32Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToGR64Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToGR8Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToST0_32Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToST0_64Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToST0_80Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToST1_32Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToVR128Value_Use_MOVDQUrm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToVR64Value
  Clang :: CodeCompletion/macros-in-modules.c
  Clang :: CodeCompletion/macros-in-modules.m
  Clang :: CodeCompletion/pch-and-module.m
  Clang :: Driver/crash-report-modules.m
  Clang :: Driver/modules-cache-path.m
  Clang :: Driver/modules.m
  Clang :: Driver/modules.mm
  Clang :: Driver/split-debug.h
  Clang :: Index/Core/index-with-module.m
  Clang :: Index/annotate-comments-objc.m
  Clang :: Index/annotate-module.m
  Clang :: Index/complete-module-undef.m
  Clang :: Index/complete-modules.m
  Clang :: Index/crash-recovery-modules.m
  Clang :: Index/index-module-with-vfs.m
  Clang :: Index/index-module.m
  Clang :: Index/index-pch-with-module.m
  Clang :: Index/modules-objc-categories.m
  Clang :: Index/pch-depending-on-deleted-module.c
  Clang :: Index/pch-with-module.m
  Clang :: Index/preamble-with-implicit-import.m
  Clang :: Index/retain-comments-from-system-headers.c
  Clang :: Modules/crash-vfs-path-emptydir-entries.m
  Clang :: Modules/crash-vfs-path-symlink-component.m
  Clang :: Modules/crash-vfs-path-symlink-topheader.m
  Clang :: Modules/crash-vfs-path-traversal.m
  Clang :: Modules/crash-vfs-relative-overlay.m
  Clang :: Modules/crash-vfs-umbrella-frameworks.m
  Clang :: Modules/driver.c
  Clang :: Modules/fatal-module-loader-error.m
  Clang :: Modules/import-decl.cpp
  Clang :: Modules/module-imported-by-pch-path.m
  Clang :: Modules/self-import-header.m
  Clang :: Modules/serialized-diags.m
  Clang :: Modules/subdirectory-module-maps-working-dir.m
  Clang :: Modules/umbrella-header-include-builtin.mm
  Clang :: VFS/real-path-found-first.m
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/HasNameDeathTest.DiesOnEmptyName
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/HasNameDeathTest.DiesOnEmptyPattern
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/IsDerivedFromDeathTest.DiesOnEmptyBaseName
  Clang-Unit :: ASTMatchers/Dynamic/./DynamicASTMatchersTests/VariantValueTest.Matcher
  Clang-Unit :: Lex/./LexTests/LexerTest.DontOverallocateStringifyArgs
  Clang-Unit :: Serialization/./SerializationTests/InMemoryModuleCacheTest.addBuiltPCM
  Clang-Unit :: Serialization/./SerializationTests/InMemoryModuleCacheTest.addPCM
  Clang-Unit :: Serialization/./SerializationTests/InMemoryModuleCacheTest.initialState
  Clang-Unit :: Serialization/./SerializationTests/InMemoryModuleCacheTest.tryToDropPCM
  Clang-Unit :: libclang/./libclangTests/LibclangReparseTest.ReparseWithModule

Expected Passes    : 34914
Expected Failures  : 71
Unsupported Tests  : 14478
Unexpected Failures: 95
MaskRay added a comment.Jul 3 2019, 6:40 AM
In D63432#1568327, @davezarzycki wrote:

Actually, the bug is narrower and doesn't require Swift (my build-and-test script placed the blame incorrectly). One just needs to build llvm+clang+lld after this change, then the following tests fail:

Failing Tests (95):

  LLVM-Unit :: ADT/./ADTTests/APFloatTest.SemanticsDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringDecimalDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringDecimalSignificandDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringHexadecimalDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringHexadecimalExponentDeath
  LLVM-Unit :: ADT/./ADTTests/APFloatTest.StringHexadecimalSignificandDeath
  LLVM-Unit :: ADT/./ADTTests/APIntTest.StringDeath
  LLVM-Unit :: ADT/./ADTTests/APSIntTest.StringDeath
  LLVM-Unit :: ADT/./ADTTests/AnyTest.BadAnyCast
  LLVM-Unit :: ADT/./ADTTests/BumpPtrListTest.resetAlloc
  LLVM-Unit :: ADT/./ADTTests/FallibleIteratorTest.RegularLoopExitRequiresErrorCheck
  LLVM-Unit :: ADT/./ADTTests/PackedVectorTest.SignedValues
  LLVM-Unit :: ADT/./ADTTests/PackedVectorTest.UnsignedValues
  LLVM-Unit :: ADT/./ADTTests/STLExtrasTest.EarlyIncrementTest
  LLVM-Unit :: AsmParser/./AsmParserTests/AsmParserTest.NonNullTerminatedInput
  LLVM-Unit :: BinaryFormat/./BinaryFormatTests/MsgPackWriter.TestWriteCompatibleNoBin
  LLVM-Unit :: CodeGen/./CodeGenTests/LowLevelTypeTest.ChangeElementTypeDeath
  LLVM-Unit :: CodeGen/./CodeGenTests/MachineInstrBundleIteratorTest.CheckForBundles
  LLVM-Unit :: IR/./IRTests/ConstantsTest.ReplaceWithConstantTest
  LLVM-Unit :: IR/./IRTests/GlobalTest.AlignDeath
  LLVM-Unit :: IR/./IRTests/ValueHandle.AssertingVH_Asserts
  LLVM-Unit :: IR/./IRTests/ValueHandle.PoisoningVH_Asserts
  LLVM-Unit :: IR/./IRTests/ValueHandle.TrackingVH_Asserts
  LLVM-Unit :: IR/./IRTests/ValueTest.getLocalSlotDeath
  LLVM-Unit :: Support/./SupportTests/CommandLineTest.TokenizeWindowsCommandLine2
  LLVM-Unit :: Transforms/Utils/./UtilsTests/ValueMapperTest.mapMetadataLocalAsMetadata
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.FillMemoryOperands_ADD64rm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.FillMemoryOperands_VGATHERDPSZ128rm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.SetRegToVR128Value_Use_VMOVDQU32Z128rm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.SetRegToVR256Value_Use_VMOVDQU32Z256rm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2Avx512TargetTest.SetRegToVR512Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2AvxTargetTest.SetRegToVR128Value_Use_VMOVDQUrm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2AvxTargetTest.SetRegToVR256Value_Use_VMOVDQUYrm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.NoHighByteRegs
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetFlags
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToFP0_80Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToFP1_32Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToFP1_4Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToGR16Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToGR32Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToGR64Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToGR8Value
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToST0_32Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToST0_64Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToST0_80Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToST1_32Bits
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToVR128Value_Use_MOVDQUrm
  LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToVR64Value
  Clang :: CodeCompletion/macros-in-modules.c
  Clang :: CodeCompletion/macros-in-modules.m
  Clang :: CodeCompletion/pch-and-module.m
  Clang :: Driver/crash-report-modules.m
  Clang :: Driver/modules-cache-path.m
  Clang :: Driver/modules.m
  Clang :: Driver/modules.mm
  Clang :: Driver/split-debug.h
  Clang :: Index/Core/index-with-module.m
  Clang :: Index/annotate-comments-objc.m
  Clang :: Index/annotate-module.m
  Clang :: Index/complete-module-undef.m
  Clang :: Index/complete-modules.m
  Clang :: Index/crash-recovery-modules.m
  Clang :: Index/index-module-with-vfs.m
  Clang :: Index/index-module.m
  Clang :: Index/index-pch-with-module.m
  Clang :: Index/modules-objc-categories.m
  Clang :: Index/pch-depending-on-deleted-module.c
  Clang :: Index/pch-with-module.m
  Clang :: Index/preamble-with-implicit-import.m
  Clang :: Index/retain-comments-from-system-headers.c
  Clang :: Modules/crash-vfs-path-emptydir-entries.m
  Clang :: Modules/crash-vfs-path-symlink-component.m
  Clang :: Modules/crash-vfs-path-symlink-topheader.m
  Clang :: Modules/crash-vfs-path-traversal.m
  Clang :: Modules/crash-vfs-relative-overlay.m
  Clang :: Modules/crash-vfs-umbrella-frameworks.m
  Clang :: Modules/driver.c
  Clang :: Modules/fatal-module-loader-error.m
  Clang :: Modules/import-decl.cpp
  Clang :: Modules/module-imported-by-pch-path.m
  Clang :: Modules/self-import-header.m
  Clang :: Modules/serialized-diags.m
  Clang :: Modules/subdirectory-module-maps-working-dir.m
  Clang :: Modules/umbrella-header-include-builtin.mm
  Clang :: VFS/real-path-found-first.m
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/HasNameDeathTest.DiesOnEmptyName
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/HasNameDeathTest.DiesOnEmptyPattern
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/IsDerivedFromDeathTest.DiesOnEmptyBaseName
  Clang-Unit :: ASTMatchers/Dynamic/./DynamicASTMatchersTests/VariantValueTest.Matcher
  Clang-Unit :: Lex/./LexTests/LexerTest.DontOverallocateStringifyArgs
  Clang-Unit :: Serialization/./SerializationTests/InMemoryModuleCacheTest.addBuiltPCM
  Clang-Unit :: Serialization/./SerializationTests/InMemoryModuleCacheTest.addPCM
  Clang-Unit :: Serialization/./SerializationTests/InMemoryModuleCacheTest.initialState
  Clang-Unit :: Serialization/./SerializationTests/InMemoryModuleCacheTest.tryToDropPCM
  Clang-Unit :: libclang/./libclangTests/LibclangReparseTest.ReparseWithModule

Expected Passes    : 34914
Expected Failures  : 71
Unsupported Tests  : 14478
Unexpected Failures: 95

Can you share your simplified build/test instructions? I don't find evidence from other buildbots so I'm not sure this change is the culprit.

peter.smith added a comment.Jul 3 2019, 6:40 AM
In D63432#1568294, @davezarzycki wrote:

This change breaks a ton of clang tests, albeit in the Swift fork of clang. Was this expected? If not, can we revert this for now?

I've confirmed that reverting this change allows top-of-tree LLVM+clang+lld to build/test top-of-tree Swift (with forks of llvm, clang, etc)

It wasn't expected. Those tests don't seem to be reliant on a specific output in the binary so it looks suspiciously like clang, libclang.so etc are being linked incorrectly. In theory this should get caught by http://lab.llvm.org:8011/builders/clang-lld-x86_64-2stage/builds/8556 (still at build-stage 2). If that fails then there is a good case to revert the change for further investigation.

davezarzycki added a comment.Jul 3 2019, 6:44 AM

I'm doing a three-stage build, but perhaps not exactly the same way. I'd be surprised if it passes.

MaskRay added a comment.EditedJul 3 2019, 6:52 AM
In D63432#1568367, @davezarzycki wrote:

I'm doing a three-stage build, but perhaps not exactly the same way. I'd be surprised if it passes.

I'll revert if I see a clear evidence.

Can you also try -DCMAKE_EXE_LINKER_FLAGS=-Wl,-O0 -DCMAKE_SHARED_LINKER_FLAGS=-Wl,-O0 when your test finishes?

If it fails in the default configuration but passes with -Wl,-O0, I can be more certain the failure is likely associated with the SHF_MERGE change.

davezarzycki added a comment.EditedJul 3 2019, 7:25 AM

As it turns out, stage 2 still fails on my workstation, but that's because -DCMAKE_EXE_LINKER_FLAGS=-Wl,-O0 -DCMAKE_SHARED_LINKER_FLAGS=-Wl,-O0 does NOT work as one expects due to LLVM's cmake/modules/AddLLVM.cmake setting -Wl,-O3 *after* CMake's CMAKE_EXE_LINKER_FLAGS or CMAKE_SHARED_LINKER_FLAGS. I'm trying a new two stage build with -DLLVM_LDFLAGS=-Wl,-O0.

davezarzycki added a comment.Jul 3 2019, 7:46 AM

Well, as it turns out, -DLLVM_LDFLAGS is even less effective than the original CMAKE build options.

I took your original suggestion and hacked LLVM's cmake to force -Wl,-O0 instead of -Wl,-O3. A two stage build now works. Is this enough evidence to revert or fix this patch? Do you need any more tests?

MaskRay added a reverting change: rL365048: Revert D63432 "[ELF] Allow placing SHF_MERGE sections with different alignments….Jul 3 2019, 8:29 AM
MaskRay added a reverting change: rLLD365048: Revert D63432 "[ELF] Allow placing SHF_MERGE sections with different alignments….
MaskRay added a reverting change: rGb9bc9f67f573: Revert D63432 "[ELF] Allow placing SHF_MERGE sections with different alignments….
MaskRay added a comment.EditedJul 3 2019, 8:31 AM
In D63432#1568457, @davezarzycki wrote:

Well, as it turns out, -DLLVM_LDFLAGS is even less effective than the original CMAKE build options.

I took your original suggestion and hacked LLVM's cmake to force -Wl,-O0 instead of -Wl,-O3. A two stage build now works. Is this enough evidence to revert or fix this patch? Do you need any more tests?

Reverted in rL365048.

I think some SHF_MERGE sections with the same name have different alignments. They were not merged before but were merged after r365015. Something that assumes address uniqueness of such mergeable data caused the failures. (The lld change still looks innocent to me but I guess it triggered some hidden issues)

I'll try reproducing the issue. If I fail to do that, I'll ask you for help to figure out the culprit :)

peter.smith added a comment.Jul 3 2019, 8:41 AM

I'm trying to think of possible problems that this change could have caused. I think that almost by definition of a merge section, you can't rely on the address of any entry to be unique. The one thing I can think of is something where the alignment requirements are higher than the entry size. By the definition of merge sections this doesn't make a lot of sense to me and might be considered a code-gen or user error.

davezarzycki added a comment.Jul 3 2019, 8:44 AM

I can try -M (a.k.a. --print-map). Is there a simple failing test from the LLVM or clang test suite that would be enlightening to try it with?

I can also help you test this when you think you have a fix too. I'm reachable between the rough hours of 9a to 5p CEST, M-F.

peter.smith added a comment.Jul 3 2019, 8:53 AM
In D63432#1568561, @davezarzycki wrote:

I can try -M (a.k.a. --print-map). Is there a simple failing test from the LLVM or clang test suite that would be enlightening to try it with?

Unfortunately I don't think so. Given where the test failures are (clang linked with LLD), I think that this may be a problem in the clang executable or shared-library itself causing it to miscompile the tests. If I'm right I think we'll need to find where the differences are in the before and after build then track back to the merge sections in the original objects.

I can also help you test this when you think you have a fix too. I'm reachable between the rough hours of 9a to 5p CEST, M-F.

peter.smith added a comment.Jul 4 2019, 2:57 AM

FWIW: The buildbot http://lab.llvm.org:8011/builders/clang-lld-x86_64-2stage/builds/8556/steps/test-stage2-check-all/logs/stdio build did eventually fail with the same errors. I am able to reproduce locally with:

  • Build clang, lld, compiler-rt with this change. Strictly speaking it might be enough to build just LLD, but by doing clang as well it makes stage 2 easier. I used
  • Install (probably not necessary, as this can probably be done out of the build directory)
  • Make new build directory
  • Do ninja check-llvm

See resulting unit test failures. There will probably be more if you do check-clang, but building just llvm is quicker.

The options I used (probably not all necessary, and you'll need to alter the local paths). For stage 2 chang -DCMAKE_C_COMPILER to the installed clang or build directory from stage 1.

cmake -GNinja ../../llvm \
      -DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt" \
      -DCMAKE_C_COMPILER=/work/clang8.0.0/bin/clang\
      -DCMAKE_CXX_COMPILER=/work/clang8.0.0/bin/clang++\
      -DCMAKE_BUILD_TYPE=Release \
      -DLLVM_ENABLE_ASSERTIONS=True \
      -DCMAKE_INSTALL_PREFIX=/work/clangmaster \
      -DCMAKE_CXX_FLAGS="-Wall" \
      -DLLVM_BUILD_TESTS=True \
      -DCMAKE_EXPORT_COMPILE_COMMANDS=True\
      -DLLVM_CCACHE_BUILD=True \
      -DLLVM_ENABLE_LLD=True\
      -DLLVM_BINUTILS_INCDIR=/work/gnu/binutils-gdb/include

I think it would be sufficient to just do a ninja check-llvm with LLD as the linker being used by clang.

I've not tried to work out what is causing the problem. The full errors look like:

******************** TEST 'LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToVR128Value_Use_MOVDQUrm' FAILED ********************
Note: Google Test filter = Core2TargetTest.SetRegToVR128Value_Use_MOVDQUrm
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from Core2TargetTest
[ RUN      ] Core2TargetTest.SetRegToVR128Value_Use_MOVDQUrm
 #0 0x0000000000abed74 PrintStackTraceSignalHandler(void*) (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xabed74)
 #1 0x0000000000abcadc llvm::sys::RunSignalHandlers() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xabcadc)
 #2 0x0000000000abf2b8 SignalHandler(int) (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xabf2b8)
 #3 0x00007fcbdf5fb390 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x11390)
 #4 0x00000000009e5006 llvm::exegesis::(anonymous namespace)::X86TargetTest::X86TargetTest(char const*) (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0x9e5006)
 #5 0x00000000009f3d4d testing::internal::TestFactoryImpl<llvm::exegesis::(anonymous namespace)::Core2TargetTest_SetRegToVR128Value_Use_MOVDQUrm_Test>::CreateTest() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0x9f3d4d)
 #6 0x0000000000d2e9ab testing::TestInfo::Run() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd2e9ab)
 #7 0x0000000000d2f1c7 testing::TestCase::Run() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd2f1c7)
 #8 0x0000000000d37087 testing::internal::UnitTestImpl::RunAllTests() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd37087)
 #9 0x0000000000d36bc7 testing::UnitTest::Run() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd36bc7)
#10 0x0000000000d2663b main (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd2663b)
#11 0x00007fcbde0ed830 __libc_start_main /build/glibc-LK5gWL/glibc-2.23/csu/../csu/libc-start.c:325:0
#12 0x00000000009a6029 _start (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0x9a6029)

********************
Testing Time: 96.27s
********************
MaskRay mentioned this in D64200: [ELF] Allow placing non-string SHF_MERGE sections with different alignments into the same MergeSyntheticSection.Jul 4 2019, 3:21 AM
MaskRay added a comment.EditedJul 4 2019, 3:38 AM
In D63432#1570136, @peter.smith wrote:

FWIW: The buildbot http://lab.llvm.org:8011/builders/clang-lld-x86_64-2stage/builds/8556/steps/test-stage2-check-all/logs/stdio build did eventually fail with the same errors. I am able to reproduce locally with:

  • Build clang, lld, compiler-rt with this change. Strictly speaking it might be enough to build just LLD, but by doing clang as well it makes stage 2 easier. I used
  • Install (probably not necessary, as this can probably be done out of the build directory)
  • Make new build directory
  • Do ninja check-llvm

See resulting unit test failures. There will probably be more if you do check-clang, but building just llvm is quicker.

The options I used (probably not all necessary, and you'll need to alter the local paths). For stage 2 chang -DCMAKE_C_COMPILER to the installed clang or build directory from stage 1.

cmake -GNinja ../../llvm \
      -DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt" \
      -DCMAKE_C_COMPILER=/work/clang8.0.0/bin/clang\
      -DCMAKE_CXX_COMPILER=/work/clang8.0.0/bin/clang++\
      -DCMAKE_BUILD_TYPE=Release \
      -DLLVM_ENABLE_ASSERTIONS=True \
      -DCMAKE_INSTALL_PREFIX=/work/clangmaster \
      -DCMAKE_CXX_FLAGS="-Wall" \
      -DLLVM_BUILD_TESTS=True \
      -DCMAKE_EXPORT_COMPILE_COMMANDS=True\
      -DLLVM_CCACHE_BUILD=True \
      -DLLVM_ENABLE_LLD=True\
      -DLLVM_BINUTILS_INCDIR=/work/gnu/binutils-gdb/include

I think it would be sufficient to just do a ninja check-llvm with LLD as the linker being used by clang.

I've not tried to work out what is causing the problem. The full errors look like:

******************** TEST 'LLVM-Unit :: tools/llvm-exegesis/X86/./LLVMExegesisX86Tests/Core2TargetTest.SetRegToVR128Value_Use_MOVDQUrm' FAILED ********************
Note: Google Test filter = Core2TargetTest.SetRegToVR128Value_Use_MOVDQUrm
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from Core2TargetTest
[ RUN      ] Core2TargetTest.SetRegToVR128Value_Use_MOVDQUrm
 #0 0x0000000000abed74 PrintStackTraceSignalHandler(void*) (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xabed74)
 #1 0x0000000000abcadc llvm::sys::RunSignalHandlers() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xabcadc)
 #2 0x0000000000abf2b8 SignalHandler(int) (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xabf2b8)
 #3 0x00007fcbdf5fb390 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x11390)
 #4 0x00000000009e5006 llvm::exegesis::(anonymous namespace)::X86TargetTest::X86TargetTest(char const*) (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0x9e5006)
 #5 0x00000000009f3d4d testing::internal::TestFactoryImpl<llvm::exegesis::(anonymous namespace)::Core2TargetTest_SetRegToVR128Value_Use_MOVDQUrm_Test>::CreateTest() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0x9f3d4d)
 #6 0x0000000000d2e9ab testing::TestInfo::Run() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd2e9ab)
 #7 0x0000000000d2f1c7 testing::TestCase::Run() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd2f1c7)
 #8 0x0000000000d37087 testing::internal::UnitTestImpl::RunAllTests() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd37087)
 #9 0x0000000000d36bc7 testing::UnitTest::Run() (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd36bc7)
#10 0x0000000000d2663b main (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0xd2663b)
#11 0x00007fcbde0ed830 __libc_start_main /build/glibc-LK5gWL/glibc-2.23/csu/../csu/libc-start.c:325:0
#12 0x00000000009a6029 _start (/work/llvm-project/build/buildstage2/unittests/tools/llvm-exegesis/X86/./LLVMExegesisX86Tests+0x9a6029)

********************
Testing Time: 96.27s
********************

Thank you for investigating the issue! I've also been studying this for a while and get a simpler reproduce:

LLVM_COMMON1=(-GNinja -DCMAKE_CXX_COMPILER=$HOME/llvm/Release/bin/clang++ -DCMAKE_C_COMPILER=$HOME/llvm/Release/bin/clang -DLLVM_ENABLE_LLD=On -DLLVM_INCLUDE_GO_TESTS=OFF -DLLVM_INCLUDE_DOCS=OFF -DLLVM_INCLUDE_EXAMPLES=OFF -DLLVM_APPEND_VC_REV=OFF -DLLVM_TARGETS_TO_BUILD=X86)
cmake -GNinja -H. -BS2Release -DCMAKE_BUILD_TYPE=Release ${LLVM_COMMON1}
ninja -C ~/llvm/S2Release SupportTests
~/llvm/S2Release/unittests/Support/SupportTests #  SIGSEGV due to unaligned MOVAPS

A SHF_STRINGS|SHF_MERGE section is misaligned (address%16!=0) after the -O3 (tail merge is performed) link. I haven't narrowed down to the root cause yet. The proposed fix is at D64200

# Get response.txt used to link SupportTests
ld.lld @response.txt --discard-none
% readelf -s SupportTests | grep _ZN12_GLOBAL__N_143CommandLineTest_TokenizeGNUCommandLine_Test8TestBody
  2981: 0000000000244f64   100 OBJECT  LOCAL  DEFAULT   11 .L__const._ZN12_GLOBAL__N_143CommandLineTest_TokenizeGNUCommandLine_Test8TestBodyEv.Input
# 0000000000244f64 % 16 != 0
MaskRay mentioned this in rL365139: [ELF] Allow placing non-string SHF_MERGE sections with different alignments….Jul 4 2019, 6:35 AM
MaskRay mentioned this in rG5c4bbc274663: [ELF] Allow placing non-string SHF_MERGE sections with different alignments….

Revision Contents

PathSize
ELF/
3 lines
test/
ELF/
25 lines

Diff 205076

ELF/SyntheticSections.cpp

Show First 20 Lines • Show All 2,913 Lines • ▼ Show 20 Lines
template <class ELFT> bool VersionNeedSection<ELFT>::isNeeded() const { template <class ELFT> bool VersionNeedSection<ELFT>::isNeeded() const {
return SharedFile::VernauxNum != 0; return SharedFile::VernauxNum != 0;
} }
void MergeSyntheticSection::addSection(MergeInputSection *MS) { void MergeSyntheticSection::addSection(MergeInputSection *MS) {
MS->Parent = this; MS->Parent = this;
Sections.push_back(MS); Sections.push_back(MS);
Alignment = std::max(Alignment, MS->Alignment);
} }
MergeTailSection::MergeTailSection(StringRef Name, uint32_t Type, MergeTailSection::MergeTailSection(StringRef Name, uint32_t Type,
uint64_t Flags, uint32_t Alignment) uint64_t Flags, uint32_t Alignment)
: MergeSyntheticSection(Name, Type, Flags, Alignment), : MergeSyntheticSection(Name, Type, Flags, Alignment),
Builder(StringTableBuilder::RAW, Alignment) {} Builder(StringTableBuilder::RAW, Alignment) {}
size_t MergeTailSection::getSize() const { return Builder.getSize(); } size_t MergeTailSection::getSize() const { return Builder.getSize(); }
▲ Show 20 Lines • Show All 128 Lines • ▼ Show 20 Lines auto I = llvm::find_if(MergeSections, [=](MergeSyntheticSection *Sec) {
// values of Entsize, it is better to take Entsize into consideration. // values of Entsize, it is better to take Entsize into consideration.
// //
// With a single synthetic section no two pieces with different Entsize // With a single synthetic section no two pieces with different Entsize
// could be equal, so we may as well have two sections. // could be equal, so we may as well have two sections.
// //
// Using Entsize in here also allows us to propagate it to the synthetic // Using Entsize in here also allows us to propagate it to the synthetic
// section. // section.
return Sec->Name == OutsecName && Sec->Flags == MS->Flags && return Sec->Name == OutsecName && Sec->Flags == MS->Flags &&
Sec->Entsize == MS->Entsize && Sec->Alignment == MS->Alignment; Sec->Entsize == MS->Entsize;
}); });
if (I == MergeSections.end()) { if (I == MergeSections.end()) {
MergeSyntheticSection *Syn = MergeSyntheticSection *Syn =
createMergeSynthetic(OutsecName, MS->Type, MS->Flags, MS->Alignment); createMergeSynthetic(OutsecName, MS->Type, MS->Flags, MS->Alignment);
MergeSections.push_back(Syn); MergeSections.push_back(Syn);
I = std::prev(MergeSections.end()); I = std::prev(MergeSections.end());
S = Syn; S = Syn;
Syn->Entsize = MS->Entsize; Syn->Entsize = MS->Entsize;
▲ Show 20 Lines • Show All 541 Lines • Show Last 20 Lines

test/ELF/merge-align2.s

  • This file was added.
# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
# RUN: ld.lld -r %t.o -o %t1.o
# RUN: llvm-readelf -S %t1.o | FileCheck --check-prefix=SEC %s
# RUN: llvm-readelf -x .cst8 %t1.o | FileCheck %s
## Check that if we have two SHF_MERGE sections with the same name, flags and
## entsize, but different alignments, we create one single synthetic section,
## with the larger input alignment as the output alignment.
# SEC: Name Type {{.*}} Size ES Flg Lk Inf Al
# SEC: .cst8 PROGBITS {{.*}} 000018 08 AM 0 0 8
# CHECK: 0x00000000 02000000 00000000 01000000 00000000
# CHECK-NEXT: 0x00000010 03000000 00000000
.section .cst8,"aM",@progbits,8,unique,0
.align 4
.quad 1
.quad 2
.section .cst8,"aM",@progbits,8,unique,1
.align 8
.quad 1
.quad 3