[LLVMdev] Win32 COFF Support

[Chris Lattner]

On Jun 11, 2010, at 6:44 PM, Michael Spencer wrote:

> Here is a full patch including Nathan's COFF support, tests that pass
> on Windows, and some changes to lit.
> 
> Obviously the COFF support and changes to lit should be separate
> patches in the end.
> 
> http://codereview.appspot.com/1624043/show

Hi Guys,

What is the status of this patch?  Here are some comments on the code, can someone update and send the most recent version?

I'd like to get this cleaned up applied even if it isn't functionally perfect yet.

Thanks!


A general comment: for many of these, I only list one instance of a particular issue.


+++ lib/MC/WinCOFF.h (working copy)
@@ -0,0 +1,327 @@
+//===-- llvm/MC/MCWin32Coff.h -----------------------------------*- C++ -*-===//

Please fix the filename in the comment line.

+#include <llvm/MC/MCObjectWriter.h>
+#include <llvm/ADT/SmallString.h>
+#include <llvm/ADT/SmallVector.h>

Please use "" style includes for llvm headers.



+namespace coff {
+
+  using llvm::raw_ostream;
+  using llvm::MCSymbolData;
+  using llvm::MCSectionData;

Please don't use 'using' directives in headers.



+    uint8_t * Ptr = reinterpret_cast <uint8_t *> (Data);
+    Ptr [0] = (Value & 0x000000FF) >>  0;

It would be more consistent to space this sort of thing as:

+    uint8_t *Ptr = reinterpret_cast<uint8_t *>(Data);
+    Ptr[0] = (Value & 0x000000FF) >>  0;



Notably, we don't put spaces before parens in function calls or before template arguments:

+    symbol(llvm::StringRef Name, size_t) :
+      Name(Name.begin (), Name.end ()),
...
+    typedef std::vector <symbol*> symbols;





+  // this class contians staging data for a coff section
+  struct section {

typo 'contians'.    COFF should be capitalized and the comment should end with a period.


+//===-- llvm/MC/WinCOFFObjectWriter.cpp -------------------------*- C++ -*-===//
+//

A terminology question: is it "Win32 COFF" file or "PECOFF" file?  What is the difference?  What does Win64 use?



+#include <stdio.h>

Do you really need this?  If so, please use <cstdio>



+    assert (Section->Symbol->Aux.size () == 15);
+    write_uint32_le(Section->Symbol->Aux.data() + 0, Section->Header.SizeOfRawData);
+    write_uint16_le(Section->Symbol->Aux.data() + 4, Section->Header.NumberOfRelocations);
+    write_uint16_le(Section->Symbol->Aux.data() + 6, Section->Header.PointerToLineNumbers);
...
+    void add_common_symbol(MCSymbol *Symbol, uint64_t Size, unsigned ByteAlignment, bool External);

Please wrap to 80 columns.

+        for (coff::relocations::const_iterator k = (*i)->Relocations.begin();
+                                               k != (*i)->Relocations.end();
+                                               k++) {

No need to reevaluate ".end()" every iteration of the loop.


+#define dbgout(x)  DEBUG(dbgs() << x)
+#define dbgerr(x)  do { dbgout (__FILE__ << "(" << __LINE__ <<\
+  ") : " << x << '\n'); llvm_unreachable("unexpected error occured");\
+  } while (false)
+
+#define dbgout_call(x)  DEBUG_WITH_TYPE(DEBUG_TYPE"_calls", dbgs() << \
+  __FUNCTION__ << " (" << x << ")\n")
+
+#define dbg_notimpl(x) dbgerr("not imlemented, " << __FUNCTION__ \
+                              << " (" << x << ")")

I'm not a big fan of these macros (plus typos like imlemented) are they really needed?

Overall, looks like great progress!

-Chris