[LLVMdev] Win32 COFF Support

[Nathan Jeffords]

Michael,

I have not made any changes since I last posted a patch. If you would like
to make the final updates, thats fine by me. I don't mind making the changes
either, I can have them done this evening as well.

Chris,

As far as naming goes, PE/COFF used in Microsoft's document because they are
describing both their version of COFF, and their Portable Execution format
which is a slightly modified version of their COFF format. I don't think
that PE belongs there since this code doesn't generate executables. I used
the Win prefix to differentiate it from non Microsoft Windows versions of
COFF.

For the debug macros, I personally like them because they reduce the impact
debug code has on the "real" code. I find dbgout("message") simpler and
more concise than DEBUG(dbgs() << "message") for the consumer of the debug
API. The other macros are there to not have to reproduce the same pattern a
repeatedly.

-Nathan

On Mon, Jun 14, 2010 at 12:25 PM, Michael Spencer <bigcheesegs at gmail.com>wrote:

> 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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100614/9cfe773d/attachment.html>