[lld] r232866 - [ELF] OrderPass : Order atoms only by ordinals.
Rui Ueyama
ruiu at google.com
Fri Mar 20 17:40:25 PDT 2015
OK. Maybe we should sort .init_array and fini_array section members
unconditionally. Unlike atoms, the number of output sections is pretty
small. This change seems a bit too clever than it needs to be. No need to
parameterize this code for x86/x86-64.
On Fri, Mar 20, 2015 at 5:31 PM, Shankar Easwaran <shankare at codeaurora.org>
wrote:
> The sorting is done by using the input section name. Sorting still holds
> good.
>
>
> On 3/20/2015 7:26 PM, Rui Ueyama wrote:
>
>> 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
>>>>
>>>>
>>>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
> by the Linux Foundation
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150320/655870ea/attachment.html>
More information about the llvm-commits
mailing list