<div dir="ltr">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.</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 20, 2015 at 5:31 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">The sorting is done by using the input section name. Sorting still holds good.<div class="HOEnZb"><div class="h5"><br>
<br>
On 3/20/2015 7:26 PM, Rui Ueyama wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Oh, I'm not sure if this change is correct at all.<br>
<br>
Instead of sorting atoms, you sort output sections. Output sections are<br>
created, and its name is given by getOutputSection function. That function<br>
returns ".fini_array" for any input sections start with ".fini_array".<br>
Thus, all .fini_array input sections are binned to the same section<br>
*before* you sort them. So, sorting it is too late, no?<br>
<br>
I really don't like to see this kind of patch is submitted without review.<br>
<br>
On Fri, Mar 20, 2015 at 5:22 PM, Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This is non-trivial change, you should have sent this to pre-commit<br>
review. Please do so next time. This change sacrificed readability a bit.<br>
Also, I believe there's a bug in the code.<br>
<br>
On Fri, Mar 20, 2015 at 4:47 PM, Shankar Easwaran <<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
wrote:<br>
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-<u></u>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<br>
( +- 0.28% )<br>
61,107 context-switches # 0.013 M/sec<br>
( +- 0.56% )<br>
2.932902671 seconds time elapsed<br>
( +- 0.84% )<br>
<br>
After patch<br>
-------------<br>
4608.417248 task-clock # 1.669 CPUs utilized<br>
( +- 0.30% )<br>
61,616 context-switches # 0.013 M/sec<br>
( +- 0.63% )<br>
2.761012703 seconds time elapsed<br>
( +- 0.63% )<br>
<br>
Modified:<br>
lld/trunk/lib/ReaderWriter/<u></u>ELF/DefaultLayout.h<br>
lld/trunk/lib/ReaderWriter/<u></u>ELF/OrderPass.h<br>
lld/trunk/lib/ReaderWriter/<u></u>ELF/X86_64/X86_<u></u>64TargetHandler.h<br>
lld/trunk/test/elf/init_array-<u></u>order.test<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/<u></u>ELF/DefaultLayout.h<br>
URL:<br>
<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-<u></u>project/lld/trunk/lib/<u></u>ReaderWriter/ELF/<u></u>DefaultLayout.h?rev=232866&r1=<u></u>232865&r2=232866&view=diff</a><br>
<br>
==============================<u></u>==============================<u></u>==================<br>
--- lld/trunk/lib/ReaderWriter/<u></u>ELF/DefaultLayout.h (original)<br>
+++ lld/trunk/lib/ReaderWriter/<u></u>ELF/DefaultLayout.h Fri Mar 20 18:47:03<br>
2015<br>
@@ -316,13 +316,25 @@ public:<br>
return _copiedDynSymNames.count(sla-><u></u>name());<br>
}<br>
<br>
+ /// \brief Handle SORT_BY_PRIORITY.<br>
+ void sortOutputSectionByPriority(<u></u>StringRef outputSectionName,<br>
+ StringRef prefix);<br>
+<br>
protected:<br>
+ /// \brief TargetLayouts may use these functions to reorder the input<br>
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::<u></u>ContentPermissions contentPermissions,<br>
SectionOrder sectionOrder);<br>
<br>
+private:<br>
+ /// Helper function that returns the priority value from an input<br>
section.<br>
+ uint32_t getPriorityFromSectionName(<u></u>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>::<u></u>getPriorityFromSectionName(<u></u>StringRef sectionName)<br>
const {<br>
+ StringRef priority = sectionName.drop_front().<u></u>rsplit('.').second;<br>
+ uint32_t prio;<br>
+ if (priority.getAsInteger(10, prio))<br>
+ return std::numeric_limits<uint32_t>:<u></u>:max();<br>
+ return prio;<br>
+}<br>
+<br>
+template <class ELFT><br>
+void DefaultLayout<ELFT>::<u></u>sortOutputSectionByPriority(<br>
+ StringRef outputSectionName, StringRef prefix) {<br>
+ OutputSection<ELFT> *outputSection =<br>
findOutputSection(<u></u>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()<u></u>;<br>
+ StringRef rhsSectionName = rhsSection->inputSectionName()<u></u>;<br>
+<br>
+ if (!prefix.empty()) {<br>
+ if (!lhsSectionName.startswith(<u></u>prefix) ||<br>
+ !rhsSectionName.startswith(<u></u>prefix))<br>
+ return false;<br>
+ }<br>
</blockquote>
+ return getPriorityFromSectionName(<u></u>lhsSectionName) <<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ getPriorityFromSectionName(<u></u>rhsSectionName);<br>
+ });<br>
<br>
</blockquote>
This sort does not preserve the original section order if they don't start<br>
with a given prefix.<br>
<br>
+};<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
template <class ELFT> void<br>
DefaultLayout<ELFT>::<u></u>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/<u></u>ELF/OrderPass.h<br>
URL:<br>
<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-<u></u>project/lld/trunk/lib/<u></u>ReaderWriter/ELF/OrderPass.h?<u></u>rev=232866&r1=232865&r2=<u></u>232866&view=diff</a><br>
<br>
==============================<u></u>==============================<u></u>==================<br>
--- lld/trunk/lib/ReaderWriter/<u></u>ELF/OrderPass.h (original)<br>
+++ lld/trunk/lib/ReaderWriter/<u></u>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<<u></u>MutableFile> &file) override {<br>
- MutableFile::DefinedAtomRange defined = file->definedAtoms();<br>
- auto last = std::partition(defined.begin()<u></u>, defined.end(),<br>
isInitFini);<br>
- parallel_sort(defined.begin(), last, compareInitFini);<br>
- parallel_sort(last, defined.end(), DefinedAtom::<u></u>compareByPosition);<br>
- }<br>
-<br>
-private:<br>
- static bool isInitFini(const DefinedAtom *atom) {<br>
- if (atom->sectionChoice() != DefinedAtom::<u></u>sectionCustomRequired)<br>
- return false;<br>
- StringRef name = atom->customSectionName();<br>
- return name.startswith(".init_array") ||<br>
name.startswith(".fini_array")<u></u>;<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('.').<u></u>second;<br>
- uint32_t prio;<br>
- if (num.getAsInteger(10, prio))<br>
- return std::numeric_limits<uint32_t>:<u></u>:max();<br>
- return prio;<br>
- }<br>
-<br>
- static bool compareInitFini(const DefinedAtom *lhs, const DefinedAtom<br>
*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::<u></u>compareByPosition(lhs, rhs);<br>
+ parallel_sort(file-><u></u>definedAtoms().begin(),<br>
file->definedAtoms().end(),<br>
+ DefinedAtom::<u></u>compareByPosition);<br>
}<br>
};<br>
-<br>
}<br>
}<br>
<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/<u></u>ELF/X86_64/X86_<u></u>64TargetHandler.h<br>
URL:<br>
<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-<u></u>project/lld/trunk/lib/<u></u>ReaderWriter/ELF/X86_64/X86_<u></u>64TargetHandler.h?rev=232866&<u></u>r1=232865&r2=232866&view=diff</a><br>
<br>
==============================<u></u>==============================<u></u>==================<br>
--- lld/trunk/lib/ReaderWriter/<u></u>ELF/X86_64/X86_<u></u>64TargetHandler.h (original)<br>
+++ lld/trunk/lib/ReaderWriter/<u></u>ELF/X86_64/X86_<u></u>64TargetHandler.h Fri Mar<br>
20 18:47:03 2015<br>
@@ -24,6 +24,11 @@ class X86_64TargetLayout : public Target<br>
public:<br>
X86_64TargetLayout(X86_<u></u>64LinkingContext &context)<br>
: TargetLayout(context) {}<br>
+<br>
+ void finalizeOutputSectionLayout() override {<br>
+ sortOutputSectionByPriority(".<u></u>init_array", ".init_array");<br>
+ sortOutputSectionByPriority(".<u></u>fini_array", ".fini_array");<br>
<br>
</blockquote>
And you call the function twice. The result of the first invocation would<br>
be invalidated by the second invocation.<br>
<br>
<br>
<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-<u></u>order.test<br>
URL:<br>
<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-<u></u>project/lld/trunk/test/elf/<u></u>init_array-order.test?rev=<u></u>232866&r1=232865&r2=232866&<u></u>view=diff</a><br>
<br>
==============================<u></u>==============================<u></u>==================<br>
--- lld/trunk/test/elf/init_array-<u></u>order.test (original)<br>
+++ lld/trunk/test/elf/init_array-<u></u>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>
______________________________<u></u>_________________<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/<u></u>mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote>
<br>
</blockquote></blockquote>
<br>
<br></div></div><div class="HOEnZb"><div class="h5">
-- <br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation<br>
<br>
</div></div></blockquote></div><br></div>