[lld] r232866 - [ELF] OrderPass : Order atoms only by ordinals.
Rui Ueyama
ruiu at google.com
Fri Mar 20 17:26:46 PDT 2015
Oh, I'm not sure if this change is correct at all.
Instead of sorting atoms, you sort output sections. Output sections are
created, and its name is given by getOutputSection function. That function
returns ".fini_array" for any input sections start with ".fini_array".
Thus, all .fini_array input sections are binned to the same section
*before* you sort them. So, sorting it is too late, no?
I really don't like to see this kind of patch is submitted without review.
On Fri, Mar 20, 2015 at 5:22 PM, Rui Ueyama <ruiu at google.com> wrote:
> This is non-trivial change, you should have sent this to pre-commit
> review. Please do so next time. This change sacrificed readability a bit.
> Also, I believe there's a bug in the code.
>
> On Fri, Mar 20, 2015 at 4:47 PM, Shankar Easwaran <shankare at codeaurora.org
> > wrote:
>
>> Author: shankare
>> Date: Fri Mar 20 18:47:03 2015
>> New Revision: 232866
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=232866&view=rev
>> Log:
>> [ELF] OrderPass : Order atoms only by ordinals.
>>
>> Move the init array/fini array sorting to the Output ELF writer.
>>
>> AFAIK, this is only needed by the X86_64/ARM ABI.
>>
>> This shaves time taken to self host lld by 0.2 seconds.
>>
>> Before patch
>> ----------------
>> 4791.062059 task-clock # 1.634 CPUs utilized
>> ( +- 0.28% )
>> 61,107 context-switches # 0.013 M/sec
>> ( +- 0.56% )
>> 2.932902671 seconds time elapsed
>> ( +- 0.84% )
>>
>> After patch
>> -------------
>> 4608.417248 task-clock # 1.669 CPUs utilized
>> ( +- 0.30% )
>> 61,616 context-switches # 0.013 M/sec
>> ( +- 0.63% )
>> 2.761012703 seconds time elapsed
>> ( +- 0.63% )
>>
>> Modified:
>> lld/trunk/lib/ReaderWriter/ELF/DefaultLayout.h
>> lld/trunk/lib/ReaderWriter/ELF/OrderPass.h
>> lld/trunk/lib/ReaderWriter/ELF/X86_64/X86_64TargetHandler.h
>> lld/trunk/test/elf/init_array-order.test
>>
>> Modified: lld/trunk/lib/ReaderWriter/ELF/DefaultLayout.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/DefaultLayout.h?rev=232866&r1=232865&r2=232866&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/lib/ReaderWriter/ELF/DefaultLayout.h (original)
>> +++ lld/trunk/lib/ReaderWriter/ELF/DefaultLayout.h Fri Mar 20 18:47:03
>> 2015
>> @@ -316,13 +316,25 @@ public:
>> return _copiedDynSymNames.count(sla->name());
>> }
>>
>> + /// \brief Handle SORT_BY_PRIORITY.
>> + void sortOutputSectionByPriority(StringRef outputSectionName,
>> + StringRef prefix);
>> +
>> protected:
>> + /// \brief TargetLayouts may use these functions to reorder the input
>> sections
>> + /// in a order defined by their ABI.
>> + virtual void finalizeOutputSectionLayout() {}
>> +
>> /// \brief Allocate a new section.
>> virtual AtomSection<ELFT> *createSection(
>> StringRef name, int32_t contentType,
>> DefinedAtom::ContentPermissions contentPermissions,
>> SectionOrder sectionOrder);
>>
>> +private:
>> + /// Helper function that returns the priority value from an input
>> section.
>> + uint32_t getPriorityFromSectionName(StringRef sectionName) const;
>> +
>> protected:
>> llvm::BumpPtrAllocator _allocator;
>> SectionMapT _sectionMap;
>> @@ -657,13 +669,56 @@ template <class ELFT> void DefaultLayout
>> }
>> }
>>
>> +template <class ELFT>
>> +uint32_t
>> +DefaultLayout<ELFT>::getPriorityFromSectionName(StringRef sectionName)
>> const {
>> + StringRef priority = sectionName.drop_front().rsplit('.').second;
>> + uint32_t prio;
>> + if (priority.getAsInteger(10, prio))
>> + return std::numeric_limits<uint32_t>::max();
>> + return prio;
>> +}
>> +
>> +template <class ELFT>
>> +void DefaultLayout<ELFT>::sortOutputSectionByPriority(
>> + StringRef outputSectionName, StringRef prefix) {
>> + OutputSection<ELFT> *outputSection =
>> findOutputSection(outputSectionName);
>> + if (!outputSection)
>> + return;
>> +
>> + auto sections = outputSection->sections();
>> +
>> + std::sort(sections.begin(), sections.end(),
>> + [&](Chunk<ELFT> *lhs, Chunk<ELFT> *rhs) {
>> + Section<ELFT> *lhsSection = dyn_cast<Section<ELFT>>(lhs);
>> + Section<ELFT> *rhsSection = dyn_cast<Section<ELFT>>(rhs);
>> + if (!lhsSection || !rhsSection)
>> + return false;
>> + StringRef lhsSectionName = lhsSection->inputSectionName();
>> + StringRef rhsSectionName = rhsSection->inputSectionName();
>> +
>> + if (!prefix.empty()) {
>> + if (!lhsSectionName.startswith(prefix) ||
>> + !rhsSectionName.startswith(prefix))
>> + return false;
>> + }
>
> + return getPriorityFromSectionName(lhsSectionName) <
>> + getPriorityFromSectionName(rhsSectionName);
>> + });
>>
>
> This sort does not preserve the original section order if they don't start
> with a given prefix.
>
> +};
>> +
>> template <class ELFT> void
>> DefaultLayout<ELFT>::assignSectionsToSegments() {
>> ScopedTask task(getDefaultDomain(), "assignSectionsToSegments");
>> ELFLinkingContext::OutputMagic outputMagic = _context.getOutputMagic();
>> // sort the sections by their order as defined by the layout
>> sortInputSections();
>> +
>> // Create output sections.
>> createOutputSections();
>> +
>> + // Finalize output section layout.
>> + finalizeOutputSectionLayout();
>> +
>> // Set the ordinal after sorting the sections
>> int ordinal = 1;
>> for (auto osi : _outputSections) {
>>
>> Modified: lld/trunk/lib/ReaderWriter/ELF/OrderPass.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/OrderPass.h?rev=232866&r1=232865&r2=232866&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/lib/ReaderWriter/ELF/OrderPass.h (original)
>> +++ lld/trunk/lib/ReaderWriter/ELF/OrderPass.h Fri Mar 20 18:47:03 2015
>> @@ -17,49 +17,13 @@ namespace lld {
>> namespace elf {
>>
>> /// \brief This pass sorts atoms by file and atom ordinals.
>> -/// .{init,fini}_array.<priority> sections are handled specially.
>> class OrderPass : public Pass {
>> public:
>> void perform(std::unique_ptr<MutableFile> &file) override {
>> - MutableFile::DefinedAtomRange defined = file->definedAtoms();
>> - auto last = std::partition(defined.begin(), defined.end(),
>> isInitFini);
>> - parallel_sort(defined.begin(), last, compareInitFini);
>> - parallel_sort(last, defined.end(), DefinedAtom::compareByPosition);
>> - }
>> -
>> -private:
>> - static bool isInitFini(const DefinedAtom *atom) {
>> - if (atom->sectionChoice() != DefinedAtom::sectionCustomRequired)
>> - return false;
>> - StringRef name = atom->customSectionName();
>> - return name.startswith(".init_array") ||
>> name.startswith(".fini_array");
>> - }
>> -
>> - // Parses the number in .{init,fini}_array.<number>.
>> - // Returns UINT32_MAX by default.
>> - static uint32_t getPriority(const DefinedAtom *atom) {
>> - StringRef sec = atom->customSectionName();
>> - StringRef num = sec.drop_front().rsplit('.').second;
>> - uint32_t prio;
>> - if (num.getAsInteger(10, prio))
>> - return std::numeric_limits<uint32_t>::max();
>> - return prio;
>> - }
>> -
>> - static bool compareInitFini(const DefinedAtom *lhs, const DefinedAtom
>> *rhs) {
>> - // Sort {.init_array, .fini_array}[.<num>] sections
>> - // according to their number. Sections without optional
>> - // numer suffix should go last.
>> - uint32_t lp = getPriority(lhs);
>> - uint32_t rp = getPriority(rhs);
>> - if (lp != rp)
>> - return lp < rp;
>> -
>> - // If both atoms have the same priority, fall back to default.
>> - return DefinedAtom::compareByPosition(lhs, rhs);
>> + parallel_sort(file->definedAtoms().begin(),
>> file->definedAtoms().end(),
>> + DefinedAtom::compareByPosition);
>> }
>> };
>> -
>> }
>> }
>>
>>
>> Modified: lld/trunk/lib/ReaderWriter/ELF/X86_64/X86_64TargetHandler.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/X86_64/X86_64TargetHandler.h?rev=232866&r1=232865&r2=232866&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/lib/ReaderWriter/ELF/X86_64/X86_64TargetHandler.h (original)
>> +++ lld/trunk/lib/ReaderWriter/ELF/X86_64/X86_64TargetHandler.h Fri Mar
>> 20 18:47:03 2015
>> @@ -24,6 +24,11 @@ class X86_64TargetLayout : public Target
>> public:
>> X86_64TargetLayout(X86_64LinkingContext &context)
>> : TargetLayout(context) {}
>> +
>> + void finalizeOutputSectionLayout() override {
>> + sortOutputSectionByPriority(".init_array", ".init_array");
>> + sortOutputSectionByPriority(".fini_array", ".fini_array");
>>
>
> And you call the function twice. The result of the first invocation would
> be invalidated by the second invocation.
>
>
>> + }
>> };
>>
>> class X86_64TargetHandler
>>
>> Modified: lld/trunk/test/elf/init_array-order.test
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/test/elf/init_array-order.test?rev=232866&r1=232865&r2=232866&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/test/elf/init_array-order.test (original)
>> +++ lld/trunk/test/elf/init_array-order.test Fri Mar 20 18:47:03 2015
>> @@ -1,6 +1,7 @@
>> #RUN: yaml2obj -format=elf %s > %t
>> #RUN: lld -flavor gnu -target x86_64-linux %t --noinhibit-exec \
>> -#RUN: --output-filetype=yaml | FileCheck %s
>> +#RUN: -o %t1.out
>> +#RUN: llvm-objdump -s %t1.out | FileCheck %s
>>
>> !ELF
>> FileHeader:
>> @@ -62,12 +63,5 @@ Symbols:
>> Type: STT_SECTION
>> Section: .init_array
>>
>> -#CHECK: defined-atoms:
>> -#CHECK: content: [ 01,
>> -#CHECK: section-name: .init_array.1
>> -#CHECK: content: [ 02,
>> -#CHECK: section-name: .init_array.2
>> -#CHECK: content: [ 03,
>> -#CHECK: section-name: .init_array.3
>> -#CHECK: content: [ 99,
>> -#CHECK: section-name: .init_array
>> +#CHECK: {{[0xa-f0-9]+}} 01000000 00000000 02000000 00000000
>> +#CHECK: {{[0xa-f0-9]+}} 03000000 00000000 99000000 00000000
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150320/f578a991/attachment.html>
More information about the llvm-commits
mailing list