[lld] r238936 - COFF: Change OutputSections' type from vector<unique_ptr<T>> to vector<T*>.
Aaron Ballman
aaron at aaronballman.com
Wed Jun 3 12:42:06 PDT 2015
On Wed, Jun 3, 2015 at 3: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.
Okay, that is a sufficiently clear explanation to get the concept
through my head. ;-) Thank you (both)!
~Aaron
>
> -- 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
>> >> > 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
>> >> >
>> >> >
>> >> > ==============================================================================
>> >> > --- 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
>> >> >
>> >> >
>> >> > ==============================================================================
>> >> > --- 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
>
>
More information about the llvm-commits
mailing list