[PATCH] [LLD] Add a new PE/COFF port

[David Blaikie]

On Thu, May 28, 2015 at 7:03 AM, Rafael Ávila de Espíndola <
rafael.espindola at gmail.com> wrote:

> This is beautiful!
>
> Please do check it in. Given the time I would be eager to explore a
> similar design for ELF.
>
>
> ================
> Comment at: CMakeLists.txt:99
> @@ -98,1 +98,2 @@
>  add_subdirectory(docs)
> +add_subdirectory(COFF)
> ----------------
> No need to change it now, but what do you think this will look like once
> we add ELF? Will this be renamed SectionObj or there will be an ELF
> directory and the common code will be moved to lib?
>
> ================
> Comment at: COFF/Chunks.h:19
> @@ +18,3 @@
> +
> +using llvm::COFF::ImportDirectoryTableEntry;
> +using llvm::object::COFFSymbolRef;
> ----------------
> Using "using" in headers? Maybe at least move them to a namespace.
>
>
> ================
> Comment at: COFF/Chunks.h:40
> @@ +39,3 @@
> +public:
> +  virtual ~Chunk() {}
> +
> ----------------
> Is there an out of line key method?
>
> Is this deleted polimorficaly?
>

(& prefer "= default" over "{}", probably)


>
> ================
> Comment at: COFF/Chunks.h:44
> @@ +43,3 @@
> +  // this is a common or BSS chunk.
> +  virtual const uint8_t *getData() const { llvm_unreachable("internal
> error"); }
> +
> ----------------
> A more descriptive string would be nice :-)
>
> ================
> Comment at: COFF/Chunks.h:65
> @@ +64,3 @@
> +  // by this chunk is filled with zeros.
> +  virtual bool hasData() const { return true; }
> +
> ----------------
> Having a HasData protected member would be more llvm like.
>
> ================
> Comment at: COFF/Chunks.h:79
> @@ +78,3 @@
> +  // is illegal to call this function on non-section chunks because
> +  // only section chunks are subject of garbage collection.
> +  virtual void printDiscardedMessage() { llvm_unreachable("internal
> error"); }
> ----------------
> COFF has nothing like SHF_MERGE sections, right?
>
> This is called only for COMDAT discarding, right? There is nothing like
> --gc_sections at the moment.
>
> Where should it print? STDOUT? Document that.
>
> ================
> Comment at: COFF/Chunks.h:88
> @@ +87,3 @@
> +  // Used by the garbage collector.
> +  virtual bool isRoot() { return false; }
> +  virtual bool isLive() { return true; }
> ----------------
> This is a bit confusing. If this represents an "output thing", how can it
> be a root?
>
> Reading the rest of the code makes it clear I just just confused by Chunk
> being an output entity. It is something that is being "copied" to the
> output. Not sure where/if to document it, but it is clear enough after
> reading a bit.
>
> ================
> Comment at: COFF/Chunks.h:99
> @@ +98,3 @@
> +  // The RVA of this chunk in the output. The writer sets a value.
> +  uint64_t RVA = 0;
> +
> ----------------
> Thank you so much for using llvm naming style.
>
> ================
> Comment at: COFF/Chunks.h:112
> @@ +111,3 @@
> +
> +// A chunk representing a section of an input file.
> +class SectionChunk : public Chunk {
> ----------------
> s/representing/corresponding/?
>
> ================
> Comment at: COFF/Chunks.h:228
> @@ +227,3 @@
> +
> +// A chunk for the import descriptor table.
> +class NullChunk : public Chunk {
> ----------------
> Why is it called Null then?
>
> ================
> Comment at: COFF/Driver.cpp:124
> @@ +123,3 @@
> +    OS.flush();
> +    return make_dynamic_error_code(StringRef(S));
> +  }
> ----------------
> So, make_dynamic_error_code is a bit of a misfeature IMHO.
>
> There are two things at play
> * Issuing a diagnostic
> * Returning an error
>
> For the diagnostic, something as simple as llvm::errs() is a reasonable
> start (or passing down a raw_ostream). At some point we really have move
> the diagnostic handling code out of lib/IR so that it can be used for all
> of llvm.
>
> For error handling, we just need as many error_code enums as the callers
> can handle. For example, for all of the bitcode reading we use just
> InvalidBitcodeSignature, CorruptedBitcode.
>
> ================
> Comment at: COFF/Driver.cpp:169
> @@ +168,3 @@
> +std::unique_ptr<InputFile> createFile(StringRef Path) {
> +  if (StringRef(Path).endswith_lower(".lib"))
> +    return llvm::make_unique<ArchiveFile>(Path);
> ----------------
> Does link.exe really use the file name instead of the file signature?
> Even if it does, it might be better to pass a MemoryBufferRef to the
> ArchiveFile and ObjectFile constructors and have them *not* own the buffer.
>
> ================
> Comment at: COFF/InputFiles.cpp:111
> @@ +110,3 @@
> +
> +  if (!isa<COFFObjectFile>(Bin.get()))
> +    return make_dynamic_error_code(Twine(Name) + " is not a COFF file.");
> ----------------
> dyn_cast instead of isa+cast
>
> ================
> Comment at: COFF/InputFiles.cpp:150
> @@ +149,3 @@
> +      continue;
> +    auto *C = new (Alloc) SectionChunk(this, Sec, I);
> +    Chunks.push_back(C);
> ----------------
> who owns this?
>
> ================
> Comment at: COFF/InputFiles.h:41
> @@ +40,3 @@
> +  // Returns symbols defined by this file.
> +  virtual std::vector<SymbolBody *> &getSymbols() = 0;
> +
> ----------------
> Exposing an iterator might be better, no?
>
> ================
> Comment at: COFF/Memory.h:22
> @@ +21,3 @@
> +
> +class StringAllocator {
> +public:
> ----------------
> We should make this the one true StringSaver in llvm :-)
>
> ================
> Comment at: COFF/README.md:154
> @@ +153,3 @@
> +* Reduced number of file visits
> +
> +  The symbol table implements the Windows linker semantics. We treat
> ----------------
> Nice explanation of the differences. I must say I like the COFF way :-)
>
> ================
> Comment at: COFF/Symbols.cpp:24
> @@ +23,3 @@
> +
> +// Returns 1, 0 or -1 if this symbol should take precedence over the
> +// Other in the symbol table, tie or lose, respectively.
> ----------------
> At least on ELF common is not that simple. The end result of linking two
> common symbols with the same name is a common symbol with the maximum size
> and the maximum alignment, so it is possible that it corresponds to none of
> the input common symbols. Is that the case for COFF?
>
> http://reviews.llvm.org/D10036
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
> _______________________________________________
> 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/20150528/88ac6ca5/attachment.html>