[lld] r238936 - COFF: Change OutputSections' type from vector<unique_ptr<T>> to vector<T*>.
Aaron Ballman
aaron at aaronballman.com
Wed Jun 3 10:27:30 PDT 2015
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.
> 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
>
>
More information about the llvm-commits
mailing list