<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 4, 2015 at 2:37 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, Jun 4, 2015 at 11:16 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Jun 3, 2015 at 12:33 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Jun 3, 2015 at 10:27 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On Wed, Jun 3, 2015 at 1:15 PM, Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br>
> It has clear ownership semantics -- the writer always owns these object.<br>
<br>
</span>There are now naked pointers stored in a vector. I don't see that as<br>
being clear ownership semantics. Nothing enforces that ownership. We<br>
have this problem in a lot of places, but the push is usually to turn<br>
those naked pointers into std::unique_ptr or some other smart pointer,<br>
not the converse. That's why I was surprised at this commit and was<br>
hoping there was some performance reason (or something else) to<br>
justify it.<br></blockquote><div><br></div></span><div>std::unique_ptr isn't the right choice when it isn't actually the thing keeping the memory alive, as is the case here.</div></div></div></div></blockquote></span><div><br>But the unique_ptrs were keeping the memory alive, until the BumpPtrAllocator was added in this patch, no? We aren't usually adding 'interesting' allocation techniques except where they're justified/required for performance, in my experience.<br><br>Or am I missing something subtle about how the allocation/ownership worked prior to this patch?<br></div></div></div></div></blockquote><div><br></div></span><div>Yeah, I think you're right. I must have understood the existing code. Actually the current state seems leaky since the destructor of `std::vector<Chunk *> Chunks;` inside OutputSection doesn't seem to be called.</div></div></div></div></blockquote><div><br>*nod* I'm going to +1 Aaron's original feedback: this patch doesn't seem like an obvious readability (or correctness) improvement. I think it's reasonable to prefer unique_ptr over bump pointer allocator, all things being equal (in the absence of evidence that the allocation is a hot path/cost that needs to be reduced - and in that case, it requires some care to check that the different semantics (not running dtors, mostly) are applicable)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
> I<br>
> think it is more clear on that point than before because the ownership<br>
> cannot be transferred by any means now.<br>
<br>
</span>You can assign these pointers, let them escape the parent container,<br>
and you can call delete on them. All of these are bad ideas that<br>
people should not do, but nothing prevents that from happening any<br>
longer.<br>
<span><font color="#888888"><br>
~Aaron<br>
</font></span><div><div><br>
><br>
> On Wed, Jun 3, 2015 at 9:55 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> On Wed, Jun 3, 2015 at 12:44 PM, Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br>
>> > Author: ruiu<br>
>> > Date: Wed Jun 3 11:44:00 2015<br>
>> > New Revision: 238936<br>
>> ><br>
>> > URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D238936-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=OulgiZemPQ2F5klDUiM5ii6E1-R2Skzn4fBikFgKkFw&s=cDW7ZXt9L2VDUMzIFcgEWcYS5dv3svFt_LxWrsovBcQ&e=" target="_blank">http://llvm.org/viewvc/llvm-project?rev=238936&view=rev</a><br>
>> > Log:<br>
>> > COFF: Change OutputSections' type from vector<unique_ptr<T>> to<br>
>> > vector<T*>.<br>
>> ><br>
>> > This is mainly for readability. OutputSection objects are still owned<br>
>> > by the writer using SpecificBumpPtrAllocator.<br>
>><br>
>> I think clear ownership semantics are more readable than naked<br>
>> pointers. If this is just for readability, it seems like a step in the<br>
>> wrong direction for maintenance. Am I simply missing something?<br>
>><br>
>> ~Aaron<br>
>><br>
>> ><br>
>> > Modified:<br>
>> > lld/trunk/COFF/Writer.cpp<br>
>> > lld/trunk/COFF/Writer.h<br>
>> ><br>
>> > Modified: lld/trunk/COFF/Writer.cpp<br>
>> > URL:<br>
>> > <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lld_trunk_COFF_Writer.cpp-3Frev-3D238936-26r1-3D238935-26r2-3D238936-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=OulgiZemPQ2F5klDUiM5ii6E1-R2Skzn4fBikFgKkFw&s=7_N7oOaPzOQDaUrLlcy3KRN6Xp3-2MoqLsCcvXqXGNA&e=" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/Writer.cpp?rev=238936&r1=238935&r2=238936&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- lld/trunk/COFF/Writer.cpp (original)<br>
>> > +++ lld/trunk/COFF/Writer.cpp Wed Jun 3 11:44:00 2015<br>
>> > @@ -117,14 +117,14 @@ void Writer::createSections() {<br>
>> > StringRef SectionName = P.first;<br>
>> > std::vector<Chunk *> &Chunks = P.second;<br>
>> > std::stable_sort(Chunks.begin(), Chunks.end(), Comp);<br>
>> > - auto Sec =<br>
>> > - llvm::make_unique<OutputSection>(SectionName,<br>
>> > OutputSections.size());<br>
>> > + size_t SectIdx = OutputSections.size();<br>
>> > + auto Sec = new (CAlloc.Allocate()) OutputSection(SectionName,<br>
>> > SectIdx);<br>
>> > for (Chunk *C : Chunks) {<br>
>> > - C->setOutputSection(Sec.get());<br>
>> > + C->setOutputSection(Sec);<br>
>> > Sec->addChunk(C);<br>
>> > Sec->addPermissions(C->getPermissions());<br>
>> > }<br>
>> > - OutputSections.push_back(std::move(Sec));<br>
>> > + OutputSections.push_back(Sec);<br>
>> > }<br>
>> > }<br>
>> ><br>
>> > @@ -210,9 +210,7 @@ void Writer::createImportTables() {<br>
>> > // The Windows loader doesn't seem to like empty sections,<br>
>> > // so we remove them if any.<br>
>> > void Writer::removeEmptySections() {<br>
>> > - auto IsEmpty = [](const std::unique_ptr<OutputSection> &S) {<br>
>> > - return S->getVirtualSize() == 0;<br>
>> > - };<br>
>> > + auto IsEmpty = [](OutputSection *S) { return S->getVirtualSize() ==<br>
>> > 0; };<br>
>> > OutputSections.erase(<br>
>> > std::remove_if(OutputSections.begin(), OutputSections.end(),<br>
>> > IsEmpty),<br>
>> > OutputSections.end());<br>
>> > @@ -225,7 +223,7 @@ void Writer::assignAddresses() {<br>
>> > HeaderSize + sizeof(coff_section) * OutputSections.size(),<br>
>> > PageSize);<br>
>> > uint64_t RVA = 0x1000; // The first page is kept unmapped.<br>
>> > uint64_t FileOff = SizeOfHeaders;<br>
>> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections) {<br>
>> > + for (OutputSection *Sec : OutputSections) {<br>
>> > Sec->setRVA(RVA);<br>
>> > Sec->setFileOffset(FileOff);<br>
>> > RVA += RoundUpToAlignment(Sec->getVirtualSize(), PageSize);<br>
>> > @@ -320,7 +318,7 @@ void Writer::writeHeader() {<br>
>> > // Name field in the section table is 8 byte long. Longer names need<br>
>> > // to be written to the string table. First, construct string table.<br>
>> > std::vector<char> Strtab;<br>
>> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections) {<br>
>> > + for (OutputSection *Sec : OutputSections) {<br>
>> > StringRef Name = Sec->getName();<br>
>> > if (Name.size() <= COFF::NameSize)<br>
>> > continue;<br>
>> > @@ -330,7 +328,7 @@ void Writer::writeHeader() {<br>
>> > }<br>
>> ><br>
>> > // Write section table<br>
>> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections) {<br>
>> > + for (OutputSection *Sec : OutputSections) {<br>
>> > Sec->writeHeader(Buf);<br>
>> > Buf += sizeof(coff_section);<br>
>> > }<br>
>> > @@ -365,7 +363,7 @@ std::error_code Writer::openFile(StringR<br>
>> > // Write section contents to a mmap'ed file.<br>
>> > void Writer::writeSections() {<br>
>> > uint8_t *Buf = Buffer->getBufferStart();<br>
>> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections) {<br>
>> > + for (OutputSection *Sec : OutputSections) {<br>
>> > // Fill gaps between functions in .text with INT3 instructions<br>
>> > // instead of leaving as NUL bytes (which can be interpreted as<br>
>> > // ADD instructions).<br>
>> > @@ -377,15 +375,15 @@ void Writer::writeSections() {<br>
>> > }<br>
>> ><br>
>> > OutputSection *Writer::findSection(StringRef Name) {<br>
>> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections)<br>
>> > + for (OutputSection *Sec : OutputSections)<br>
>> > if (Sec->getName() == Name)<br>
>> > - return Sec.get();<br>
>> > + return Sec;<br>
>> > return nullptr;<br>
>> > }<br>
>> ><br>
>> > uint32_t Writer::getSizeOfInitializedData() {<br>
>> > uint32_t Res = 0;<br>
>> > - for (std::unique_ptr<OutputSection> &S : OutputSections)<br>
>> > + for (OutputSection *S : OutputSections)<br>
>> > if (S->getPermissions() & IMAGE_SCN_CNT_INITIALIZED_DATA)<br>
>> > Res += S->getRawSize();<br>
>> > return Res;<br>
>> > @@ -407,15 +405,16 @@ OutputSection *Writer::createSection(Str<br>
>> > .Default(0);<br>
>> > if (!Perm)<br>
>> > llvm_unreachable("unknown section name");<br>
>> > - auto Sec = new OutputSection(Name, OutputSections.size());<br>
>> > + size_t SectIdx = OutputSections.size();<br>
>> > + auto Sec = new (CAlloc.Allocate()) OutputSection(Name, SectIdx);<br>
>> > Sec->addPermissions(Perm);<br>
>> > - OutputSections.push_back(std::unique_ptr<OutputSection>(Sec));<br>
>> > + OutputSections.push_back(Sec);<br>
>> > return Sec;<br>
>> > }<br>
>> ><br>
>> > void Writer::applyRelocations() {<br>
>> > uint8_t *Buf = Buffer->getBufferStart();<br>
>> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections)<br>
>> > + for (OutputSection *Sec : OutputSections)<br>
>> > for (Chunk *C : Sec->getChunks())<br>
>> > C->applyRelocations(Buf);<br>
>> > }<br>
>> ><br>
>> > Modified: lld/trunk/COFF/Writer.h<br>
>> > URL:<br>
>> > <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lld_trunk_COFF_Writer.h-3Frev-3D238936-26r1-3D238935-26r2-3D238936-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=OulgiZemPQ2F5klDUiM5ii6E1-R2Skzn4fBikFgKkFw&s=NvMaybesUToiXNxfE2TrkpG51eKPwxtEjUhpZWB4xrY&e=" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/Writer.h?rev=238936&r1=238935&r2=238936&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- lld/trunk/COFF/Writer.h (original)<br>
>> > +++ lld/trunk/COFF/Writer.h Wed Jun 3 11:44:00 2015<br>
>> > @@ -92,7 +92,8 @@ private:<br>
>> ><br>
>> > SymbolTable *Symtab;<br>
>> > std::unique_ptr<llvm::FileOutputBuffer> Buffer;<br>
>> > - std::vector<std::unique_ptr<OutputSection>> OutputSections;<br>
>> > + llvm::SpecificBumpPtrAllocator<OutputSection> CAlloc;<br>
>> > + std::vector<OutputSection *> OutputSections;<br>
>> > Chunk *ImportAddressTable = nullptr;<br>
>> > uint32_t ImportDirectoryTableSize = 0;<br>
>> > uint32_t ImportAddressTableSize = 0;<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>
><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>
</div></div></blockquote></div></div></div><br></div></div>
<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>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>