[lld] r238936 - COFF: Change OutputSections' type from vector<unique_ptr<T>> to vector<T*>.
David Blaikie
dblaikie at gmail.com
Thu Jun 4 15:52:56 PDT 2015
On Thu, Jun 4, 2015 at 3:48 PM, Rui Ueyama <ruiu at google.com> wrote:
> On Thu, Jun 4, 2015 at 3:46 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Thu, Jun 4, 2015 at 3:41 PM, Rui Ueyama <ruiu at google.com> wrote:
>>
>>> 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.
>>>
>>
>> I'd still be in favor of unque_ptr as the default unless there's extra
>> semantics/behavior that's desired of something else (basically:
>> performance). But it's not my codebase/project - so take that or leave it.
>>
>>
>>> As to the dtor, this bump pointer is SpecificBumpPtrAllocator, which I
>>> believe calls dtors.
>>>
>>
>> I believe that only happens if you call destroyAll.
>>
>
> SpecificBumpPtrAllocator's dtor seems to call DestroyAll.
>
So it does, hiding there amongst the ctors. Don't mind me.
- David
>
>
>>
>>
>>>
>>> On Thu, Jun 4, 2015 at 2:41 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Thu, Jun 4, 2015 at 2:37 PM, Sean Silva <chisophugis at gmail.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Thu, Jun 4, 2015 at 11:16 AM, David Blaikie <dblaikie at gmail.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Jun 3, 2015 at 12:33 PM, Sean Silva <chisophugis at gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Jun 3, 2015 at 10:27 AM, Aaron Ballman <
>>>>>>> aaron at aaronballman.com> wrote:
>>>>>>>
>>>>>>>> On Wed, Jun 3, 2015 at 1:15 PM, Rui Ueyama <ruiu at google.com> wrote:
>>>>>>>> > It has clear ownership semantics -- the writer always owns these
>>>>>>>> object.
>>>>>>>>
>>>>>>>> There are now naked pointers stored in a vector. I don't see that as
>>>>>>>> being clear ownership semantics. Nothing enforces that ownership. We
>>>>>>>> have this problem in a lot of places, but the push is usually to
>>>>>>>> turn
>>>>>>>> those naked pointers into std::unique_ptr or some other smart
>>>>>>>> pointer,
>>>>>>>> not the converse. That's why I was surprised at this commit and was
>>>>>>>> hoping there was some performance reason (or something else) to
>>>>>>>> justify it.
>>>>>>>>
>>>>>>>
>>>>>>> std::unique_ptr isn't the right choice when it isn't actually the
>>>>>>> thing keeping the memory alive, as is the case here.
>>>>>>>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Or am I missing something subtle about how the allocation/ownership
>>>>>> worked prior to this patch?
>>>>>>
>>>>>
>>>>> 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.
>>>>>
>>>>
>>>> *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)
>>>>
>>>>
>>>>>
>>>>> -- Sean Silva
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> -- Sean Silva
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> > I
>>>>>>>> > think it is more clear on that point than before because the
>>>>>>>> ownership
>>>>>>>> > cannot be transferred by any means now.
>>>>>>>>
>>>>>>>> You can assign these pointers, let them escape the parent container,
>>>>>>>> and you can call delete on them. All of these are bad ideas that
>>>>>>>> people should not do, but nothing prevents that from happening any
>>>>>>>> longer.
>>>>>>>>
>>>>>>>> ~Aaron
>>>>>>>>
>>>>>>>> >
>>>>>>>> > On Wed, Jun 3, 2015 at 9:55 AM, Aaron Ballman <
>>>>>>>> aaron at aaronballman.com>
>>>>>>>> > wrote:
>>>>>>>> >>
>>>>>>>> >> On Wed, Jun 3, 2015 at 12:44 PM, Rui Ueyama <ruiu at google.com>
>>>>>>>> wrote:
>>>>>>>> >> > Author: ruiu
>>>>>>>> >> > Date: Wed Jun 3 11:44:00 2015
>>>>>>>> >> > New Revision: 238936
>>>>>>>> >> >
>>>>>>>> >> > URL: http://llvm.org/viewvc/llvm-project?rev=238936&view=rev
>>>>>>>> <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=>
>>>>>>>> >> > Log:
>>>>>>>> >> > COFF: Change OutputSections' type from vector<unique_ptr<T>> to
>>>>>>>> >> > vector<T*>.
>>>>>>>> >> >
>>>>>>>> >> > This is mainly for readability. OutputSection objects are
>>>>>>>> still owned
>>>>>>>> >> > by the writer using SpecificBumpPtrAllocator.
>>>>>>>> >>
>>>>>>>> >> I think clear ownership semantics are more readable than naked
>>>>>>>> >> pointers. If this is just for readability, it seems like a step
>>>>>>>> in the
>>>>>>>> >> wrong direction for maintenance. Am I simply missing something?
>>>>>>>> >>
>>>>>>>> >> ~Aaron
>>>>>>>> >>
>>>>>>>> >> >
>>>>>>>> >> > Modified:
>>>>>>>> >> > lld/trunk/COFF/Writer.cpp
>>>>>>>> >> > lld/trunk/COFF/Writer.h
>>>>>>>> >> >
>>>>>>>> >> > Modified: lld/trunk/COFF/Writer.cpp
>>>>>>>> >> > URL:
>>>>>>>> >> >
>>>>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/Writer.cpp?rev=238936&r1=238935&r2=238936&view=diff
>>>>>>>> <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=>
>>>>>>>> >> >
>>>>>>>> >> >
>>>>>>>> ==============================================================================
>>>>>>>> >> > --- lld/trunk/COFF/Writer.cpp (original)
>>>>>>>> >> > +++ lld/trunk/COFF/Writer.cpp Wed Jun 3 11:44:00 2015
>>>>>>>> >> > @@ -117,14 +117,14 @@ void Writer::createSections() {
>>>>>>>> >> > StringRef SectionName = P.first;
>>>>>>>> >> > std::vector<Chunk *> &Chunks = P.second;
>>>>>>>> >> > std::stable_sort(Chunks.begin(), Chunks.end(), Comp);
>>>>>>>> >> > - auto Sec =
>>>>>>>> >> > - llvm::make_unique<OutputSection>(SectionName,
>>>>>>>> >> > OutputSections.size());
>>>>>>>> >> > + size_t SectIdx = OutputSections.size();
>>>>>>>> >> > + auto Sec = new (CAlloc.Allocate())
>>>>>>>> OutputSection(SectionName,
>>>>>>>> >> > SectIdx);
>>>>>>>> >> > for (Chunk *C : Chunks) {
>>>>>>>> >> > - C->setOutputSection(Sec.get());
>>>>>>>> >> > + C->setOutputSection(Sec);
>>>>>>>> >> > Sec->addChunk(C);
>>>>>>>> >> > Sec->addPermissions(C->getPermissions());
>>>>>>>> >> > }
>>>>>>>> >> > - OutputSections.push_back(std::move(Sec));
>>>>>>>> >> > + OutputSections.push_back(Sec);
>>>>>>>> >> > }
>>>>>>>> >> > }
>>>>>>>> >> >
>>>>>>>> >> > @@ -210,9 +210,7 @@ void Writer::createImportTables() {
>>>>>>>> >> > // The Windows loader doesn't seem to like empty sections,
>>>>>>>> >> > // so we remove them if any.
>>>>>>>> >> > void Writer::removeEmptySections() {
>>>>>>>> >> > - auto IsEmpty = [](const std::unique_ptr<OutputSection> &S) {
>>>>>>>> >> > - return S->getVirtualSize() == 0;
>>>>>>>> >> > - };
>>>>>>>> >> > + auto IsEmpty = [](OutputSection *S) { return
>>>>>>>> S->getVirtualSize() ==
>>>>>>>> >> > 0; };
>>>>>>>> >> > OutputSections.erase(
>>>>>>>> >> > std::remove_if(OutputSections.begin(),
>>>>>>>> OutputSections.end(),
>>>>>>>> >> > IsEmpty),
>>>>>>>> >> > OutputSections.end());
>>>>>>>> >> > @@ -225,7 +223,7 @@ void Writer::assignAddresses() {
>>>>>>>> >> > HeaderSize + sizeof(coff_section) *
>>>>>>>> OutputSections.size(),
>>>>>>>> >> > PageSize);
>>>>>>>> >> > uint64_t RVA = 0x1000; // The first page is kept unmapped.
>>>>>>>> >> > uint64_t FileOff = SizeOfHeaders;
>>>>>>>> >> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections) {
>>>>>>>> >> > + for (OutputSection *Sec : OutputSections) {
>>>>>>>> >> > Sec->setRVA(RVA);
>>>>>>>> >> > Sec->setFileOffset(FileOff);
>>>>>>>> >> > RVA += RoundUpToAlignment(Sec->getVirtualSize(),
>>>>>>>> PageSize);
>>>>>>>> >> > @@ -320,7 +318,7 @@ void Writer::writeHeader() {
>>>>>>>> >> > // Name field in the section table is 8 byte long. Longer
>>>>>>>> names need
>>>>>>>> >> > // to be written to the string table. First, construct
>>>>>>>> string table.
>>>>>>>> >> > std::vector<char> Strtab;
>>>>>>>> >> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections) {
>>>>>>>> >> > + for (OutputSection *Sec : OutputSections) {
>>>>>>>> >> > StringRef Name = Sec->getName();
>>>>>>>> >> > if (Name.size() <= COFF::NameSize)
>>>>>>>> >> > continue;
>>>>>>>> >> > @@ -330,7 +328,7 @@ void Writer::writeHeader() {
>>>>>>>> >> > }
>>>>>>>> >> >
>>>>>>>> >> > // Write section table
>>>>>>>> >> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections) {
>>>>>>>> >> > + for (OutputSection *Sec : OutputSections) {
>>>>>>>> >> > Sec->writeHeader(Buf);
>>>>>>>> >> > Buf += sizeof(coff_section);
>>>>>>>> >> > }
>>>>>>>> >> > @@ -365,7 +363,7 @@ std::error_code Writer::openFile(StringR
>>>>>>>> >> > // Write section contents to a mmap'ed file.
>>>>>>>> >> > void Writer::writeSections() {
>>>>>>>> >> > uint8_t *Buf = Buffer->getBufferStart();
>>>>>>>> >> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections) {
>>>>>>>> >> > + for (OutputSection *Sec : OutputSections) {
>>>>>>>> >> > // Fill gaps between functions in .text with INT3
>>>>>>>> instructions
>>>>>>>> >> > // instead of leaving as NUL bytes (which can be
>>>>>>>> interpreted as
>>>>>>>> >> > // ADD instructions).
>>>>>>>> >> > @@ -377,15 +375,15 @@ void Writer::writeSections() {
>>>>>>>> >> > }
>>>>>>>> >> >
>>>>>>>> >> > OutputSection *Writer::findSection(StringRef Name) {
>>>>>>>> >> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections)
>>>>>>>> >> > + for (OutputSection *Sec : OutputSections)
>>>>>>>> >> > if (Sec->getName() == Name)
>>>>>>>> >> > - return Sec.get();
>>>>>>>> >> > + return Sec;
>>>>>>>> >> > return nullptr;
>>>>>>>> >> > }
>>>>>>>> >> >
>>>>>>>> >> > uint32_t Writer::getSizeOfInitializedData() {
>>>>>>>> >> > uint32_t Res = 0;
>>>>>>>> >> > - for (std::unique_ptr<OutputSection> &S : OutputSections)
>>>>>>>> >> > + for (OutputSection *S : OutputSections)
>>>>>>>> >> > if (S->getPermissions() & IMAGE_SCN_CNT_INITIALIZED_DATA)
>>>>>>>> >> > Res += S->getRawSize();
>>>>>>>> >> > return Res;
>>>>>>>> >> > @@ -407,15 +405,16 @@ OutputSection *Writer::createSection(Str
>>>>>>>> >> > .Default(0);
>>>>>>>> >> > if (!Perm)
>>>>>>>> >> > llvm_unreachable("unknown section name");
>>>>>>>> >> > - auto Sec = new OutputSection(Name, OutputSections.size());
>>>>>>>> >> > + size_t SectIdx = OutputSections.size();
>>>>>>>> >> > + auto Sec = new (CAlloc.Allocate()) OutputSection(Name,
>>>>>>>> SectIdx);
>>>>>>>> >> > Sec->addPermissions(Perm);
>>>>>>>> >> > -
>>>>>>>> OutputSections.push_back(std::unique_ptr<OutputSection>(Sec));
>>>>>>>> >> > + OutputSections.push_back(Sec);
>>>>>>>> >> > return Sec;
>>>>>>>> >> > }
>>>>>>>> >> >
>>>>>>>> >> > void Writer::applyRelocations() {
>>>>>>>> >> > uint8_t *Buf = Buffer->getBufferStart();
>>>>>>>> >> > - for (std::unique_ptr<OutputSection> &Sec : OutputSections)
>>>>>>>> >> > + for (OutputSection *Sec : OutputSections)
>>>>>>>> >> > for (Chunk *C : Sec->getChunks())
>>>>>>>> >> > C->applyRelocations(Buf);
>>>>>>>> >> > }
>>>>>>>> >> >
>>>>>>>> >> > Modified: lld/trunk/COFF/Writer.h
>>>>>>>> >> > URL:
>>>>>>>> >> >
>>>>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/Writer.h?rev=238936&r1=238935&r2=238936&view=diff
>>>>>>>> <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=>
>>>>>>>> >> >
>>>>>>>> >> >
>>>>>>>> ==============================================================================
>>>>>>>> >> > --- lld/trunk/COFF/Writer.h (original)
>>>>>>>> >> > +++ lld/trunk/COFF/Writer.h Wed Jun 3 11:44:00 2015
>>>>>>>> >> > @@ -92,7 +92,8 @@ private:
>>>>>>>> >> >
>>>>>>>> >> > SymbolTable *Symtab;
>>>>>>>> >> > std::unique_ptr<llvm::FileOutputBuffer> Buffer;
>>>>>>>> >> > - std::vector<std::unique_ptr<OutputSection>> OutputSections;
>>>>>>>> >> > + llvm::SpecificBumpPtrAllocator<OutputSection> CAlloc;
>>>>>>>> >> > + std::vector<OutputSection *> OutputSections;
>>>>>>>> >> > Chunk *ImportAddressTable = nullptr;
>>>>>>>> >> > uint32_t ImportDirectoryTableSize = 0;
>>>>>>>> >> > uint32_t ImportAddressTableSize = 0;
>>>>>>>> >> >
>>>>>>>> >> >
>>>>>>>> >> > _______________________________________________
>>>>>>>> >> > llvm-commits mailing list
>>>>>>>> >> > llvm-commits at cs.uiuc.edu
>>>>>>>> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>> >
>>>>>>>> >
>>>>>>>> _______________________________________________
>>>>>>>> llvm-commits mailing list
>>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150604/7f04f809/attachment.html>
More information about the llvm-commits
mailing list