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 - Michael Spencer -------------- next part -------------- A non-text attachment was scrubbed... Name: COFF-support.patch Type: application/octet-stream Size: 93051 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100611/1c026810/attachment.obj>
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/showHi 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
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
I have updated my patch based on Chris'es feedback. I removed the dbgout_calls macro, but left others in place for now. If there are no objections, I would like to commit this tomorrow evening (~7PM GMT-7). I have compiled and tested it on MSVC, with Michaels testing code and it looks good. Once this is committed, Michaels patch can be applied. -Nathan -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100615/f4b28bed/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: changes.patch Type: application/octet-stream Size: 47596 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100615/f4b28bed/attachment.obj>