<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 28, 2015 at 7:03 AM, Rafael Ávila de Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This is beautiful!<br>
<br>
Please do check it in. Given the time I would be eager to explore a similar design for ELF.<br>
<br>
<br>
================<br>
Comment at: CMakeLists.txt:99<br>
@@ -98,1 +98,2 @@<br>
 add_subdirectory(docs)<br>
+add_subdirectory(COFF)<br>
----------------<br>
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?<br>
<br>
================<br>
Comment at: COFF/Chunks.h:19<br>
@@ +18,3 @@<br>
+<br>
+using llvm::COFF::ImportDirectoryTableEntry;<br>
+using llvm::object::COFFSymbolRef;<br>
----------------<br>
Using "using" in headers? Maybe at least move them to a namespace.<br>
<br>
<br>
================<br>
Comment at: COFF/Chunks.h:40<br>
@@ +39,3 @@<br>
+public:<br>
+  virtual ~Chunk() {}<br>
+<br>
----------------<br>
Is there an out of line key method?<br>
<br>
Is this deleted polimorficaly?<br></blockquote><div><br>(& prefer "= default" over "{}", probably)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: COFF/Chunks.h:44<br>
@@ +43,3 @@<br>
+  // this is a common or BSS chunk.<br>
+  virtual const uint8_t *getData() const { llvm_unreachable("internal error"); }<br>
+<br>
----------------<br>
A more descriptive string would be nice :-)<br>
<br>
================<br>
Comment at: COFF/Chunks.h:65<br>
@@ +64,3 @@<br>
+  // by this chunk is filled with zeros.<br>
+  virtual bool hasData() const { return true; }<br>
+<br>
----------------<br>
Having a HasData protected member would be more llvm like.<br>
<br>
================<br>
Comment at: COFF/Chunks.h:79<br>
@@ +78,3 @@<br>
+  // is illegal to call this function on non-section chunks because<br>
+  // only section chunks are subject of garbage collection.<br>
+  virtual void printDiscardedMessage() { llvm_unreachable("internal error"); }<br>
----------------<br>
COFF has nothing like SHF_MERGE sections, right?<br>
<br>
This is called only for COMDAT discarding, right? There is nothing like --gc_sections at the moment.<br>
<br>
Where should it print? STDOUT? Document that.<br>
<br>
================<br>
Comment at: COFF/Chunks.h:88<br>
@@ +87,3 @@<br>
+  // Used by the garbage collector.<br>
+  virtual bool isRoot() { return false; }<br>
+  virtual bool isLive() { return true; }<br>
----------------<br>
This is a bit confusing. If this represents an "output thing", how can it be a root?<br>
<br>
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.<br>
<br>
================<br>
Comment at: COFF/Chunks.h:99<br>
@@ +98,3 @@<br>
+  // The RVA of this chunk in the output. The writer sets a value.<br>
+  uint64_t RVA = 0;<br>
+<br>
----------------<br>
Thank you so much for using llvm naming style.<br>
<br>
================<br>
Comment at: COFF/Chunks.h:112<br>
@@ +111,3 @@<br>
+<br>
+// A chunk representing a section of an input file.<br>
+class SectionChunk : public Chunk {<br>
----------------<br>
s/representing/corresponding/?<br>
<br>
================<br>
Comment at: COFF/Chunks.h:228<br>
@@ +227,3 @@<br>
+<br>
+// A chunk for the import descriptor table.<br>
+class NullChunk : public Chunk {<br>
----------------<br>
Why is it called Null then?<br>
<br>
================<br>
Comment at: COFF/Driver.cpp:124<br>
@@ +123,3 @@<br>
+    OS.flush();<br>
+    return make_dynamic_error_code(StringRef(S));<br>
+  }<br>
----------------<br>
So, make_dynamic_error_code is a bit of a misfeature IMHO.<br>
<br>
There are two things at play<br>
* Issuing a diagnostic<br>
* Returning an error<br>
<br>
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.<br>
<br>
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.<br>
<br>
================<br>
Comment at: COFF/Driver.cpp:169<br>
@@ +168,3 @@<br>
+std::unique_ptr<InputFile> createFile(StringRef Path) {<br>
+  if (StringRef(Path).endswith_lower(".lib"))<br>
+    return llvm::make_unique<ArchiveFile>(Path);<br>
----------------<br>
Does link.exe really use the file name instead of the file signature?<br>
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.<br>
<br>
================<br>
Comment at: COFF/InputFiles.cpp:111<br>
@@ +110,3 @@<br>
+<br>
+  if (!isa<COFFObjectFile>(Bin.get()))<br>
+    return make_dynamic_error_code(Twine(Name) + " is not a COFF file.");<br>
----------------<br>
dyn_cast instead of isa+cast<br>
<br>
================<br>
Comment at: COFF/InputFiles.cpp:150<br>
@@ +149,3 @@<br>
+      continue;<br>
+    auto *C = new (Alloc) SectionChunk(this, Sec, I);<br>
+    Chunks.push_back(C);<br>
----------------<br>
who owns this?<br>
<br>
================<br>
Comment at: COFF/InputFiles.h:41<br>
@@ +40,3 @@<br>
+  // Returns symbols defined by this file.<br>
+  virtual std::vector<SymbolBody *> &getSymbols() = 0;<br>
+<br>
----------------<br>
Exposing an iterator might be better, no?<br>
<br>
================<br>
Comment at: COFF/Memory.h:22<br>
@@ +21,3 @@<br>
+<br>
+class StringAllocator {<br>
+public:<br>
----------------<br>
We should make this the one true StringSaver in llvm :-)<br>
<br>
================<br>
Comment at: COFF/README.md:154<br>
@@ +153,3 @@<br>
+* Reduced number of file visits<br>
+<br>
+  The symbol table implements the Windows linker semantics. We treat<br>
----------------<br>
Nice explanation of the differences. I must say I like the COFF way :-)<br>
<br>
================<br>
Comment at: COFF/Symbols.cpp:24<br>
@@ +23,3 @@<br>
+<br>
+// Returns 1, 0 or -1 if this symbol should take precedence over the<br>
+// Other in the symbol table, tie or lose, respectively.<br>
----------------<br>
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?<br>
<span class="im HOEnZb"><br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10036&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=1qkw6s6C2duE0qNWjB0VhMSC4auEhGaKN16Pht2cSq0&s=MH89TdA5GyvOtlaUXAzjwjsAyDJ5_As6t3hBGHaBQF4&e=" target="_blank">http://reviews.llvm.org/D10036</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=1qkw6s6C2duE0qNWjB0VhMSC4auEhGaKN16Pht2cSq0&s=efgST9oNBbDIfwCEvelk8sZNUupcBMSflUwffm0eYLE&e=" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
<br>
</span><div class="HOEnZb"><div class="h5">_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>