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
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>
On Jun 14, 2010, at 12:41 PM, Nathan Jeffords wrote:> 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.Ok.> 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.Are these intended to stay permanently or are they just for bring-up? Your argument implies that we should pull them into a higher-level header file, but personally I think that explicitly using "dbgs() <<" makes it clearer what is happening. -Chris