[lld] r238936 - COFF: Change OutputSections' type from vector<unique_ptr<T>> to vector<T*>.
Rui Ueyama
ruiu at google.com
Thu Jun 4 15:41:42 PDT 2015
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.
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/2e1c7d22/attachment.html>
More information about the llvm-commits
mailing list