[LLVMdev] Win32 COFF Support

[Michael Spencer]

Nathan, do you have any other changes? If not, I can make these  
changes and have a patch ready this evening.

Sent from my iPhone

On Jun 14, 2010, at 2:16 PM, Chris Lattner <clattner at apple.com> wrote:

>
> 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