<div dir="ltr">Not sure if that's applicable here because this is a frequent pattern in this code base. If you read other files and then this file, this should not be strange at least. As to the dtor, this bump pointer is SpecificBumpPtrAllocator, which I believe calls dtors.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 4, 2015 at 2:41 PM, 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: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 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>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></span><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><div><div class="h5"><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><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><div><div><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></div></div><br></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">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><br></div>