[lld] r238936 - COFF: Change OutputSections' type from vector<unique_ptr<T>> to vector<T*>.

[Aaron Ballman]

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