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