Hi guys, I have attached my patch to support generating win32 COFF object files. I would have posted earlier, but my system drive crashed and I had to rebuild my system; Luckily, my source code was on a secondary drive. I think this would be a good beginning for ongoing support of the COFF object file format and was hoping for some feedback as to whether it was commit worthy or what was needed to be done to get it their. Thanks, Nathan -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100519/273d1b9f/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: changes.patch Type: application/octet-stream Size: 46846 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100519/273d1b9f/attachment.obj>
On Wed, May 19, 2010 at 10:31 PM, Nathan Jeffords <blunted2night at gmail.com> wrote:> Hi guys, > I have attached my patch to support generating win32 COFF object files. I > would have posted earlier, but my system drive crashed and I had to rebuild > my system; Luckily, my source code was on a secondary drive. I think this > would be a good beginning for ongoing support of the COFF object file format > and was hoping for some feedback as to whether it was commit worthy or what > was needed to be done to get it their.The line for commit-worthy is a round of reviews and some tests showing that basic functionality works. Minor comments: 1. Is it possible to allocate extra memory with coff::symbol instead of using std::string and std::vector as class members? The extra allocations can add up. 2. For the following bit: + reinterpret_cast <uint32_t &> (Section->Symbol->Aux [0]) Section->Header.SizeOfRawData; + reinterpret_cast <uint16_t &> (Section->Symbol->Aux [4]) Section->Header.NumberOfRelocations; + reinterpret_cast <uint16_t &> (Section->Symbol->Aux [6]) Section->Header.PointerToLineNumbers; This isn't endian-safe; I'd suggest expanding it to 8 assignments. Same issues with the write_le functions. We need endian safety not only so that cross-compiling, for example, from PPC Linux to MingW works, but also so that tests work cross-platform. 3. For the following bit: + typedef std::list <symbol*> symbols; + typedef std::list <section*> sections; Avoid std::list; see http://llvm.org//docs/ProgrammersManual.html#dss_list . 4. For the following bit: + typedef StringMap <coff::symbol *> name_symbol_map; + typedef StringMap <coff::section *> name_section_map; Unused typedefs? 5. For the following bit (and a few others in the same function): + for (i = Sections.begin(), j = Asm.begin(); + (i != Sections.end()) && (j != Asm.end()); + i++, j++) { http://llvm.org//docs/CodingStandards.html#ll_end 6. +namespace llvm { MCObjectWriter * createWinCOFFObjectWriter (raw_ostream & OS); } If you need a function to be visible across files, use a header; shortcuts like this make the code more difficult to read. -Eli
Thanks for the feedback. The line for commit-worthy is a round of reviews and some tests> showing that basic functionality works. >Regarding tests, I would like to have some form of automated regression testing, but am not sure how to go about doing so. My current testing involves running through my compilers regression test (compiles links and executes a number of test sources) and running a couple of .ll files and manually inspecting the results of Microsoft's dumpbin utility.> > Minor comments: > 1. Is it possible to allocate extra memory with coff::symbol instead > of using std::string and std::vector as class members? The extra > allocations can add up. >Yes, I believe ShortString & ShortVector are the appropriate alternative for these types of cases.> > 2. For the following bit: > + reinterpret_cast <uint32_t &> (Section->Symbol->Aux [0]) > Section->Header.SizeOfRawData; > + reinterpret_cast <uint16_t &> (Section->Symbol->Aux [4]) > Section->Header.NumberOfRelocations; > + reinterpret_cast <uint16_t &> (Section->Symbol->Aux [6]) > Section->Header.PointerToLineNumbers; > > This isn't endian-safe; I'd suggest expanding it to 8 assignments. > Same issues with the write_le functions. We need endian safety not > only so that cross-compiling, for example, from PPC Linux to MingW > works, but also so that tests work cross-platform. >OK, I had put the write_le functions so that I could address this later. I should have used them, or something similar for the auxiliary symbols. I will rework the aux symbol stuff & the write_le functions to be cross platform safe.> > 3. For the following bit: > + typedef std::list <symbol*> symbols; > + typedef std::list <section*> sections; > > Avoid std::list; see http://llvm.org//docs/ProgrammersManual.html#dss_list. >Good catch, I would normally not do that. I must have had a list of objects, then changed it to pointers to objects and didn't rethink what type of container I was using.> > 4. For the following bit: > + typedef StringMap <coff::symbol *> name_symbol_map; > + typedef StringMap <coff::section *> name_section_map; > > Unused typedefs? >Left over from previous version of the implementation, will remove.> > 5. For the following bit (and a few others in the same function): > + for (i = Sections.begin(), j = Asm.begin(); > + (i != Sections.end()) && (j != Asm.end()); > + i++, j++) { > > http://llvm.org//docs/CodingStandards.html#ll_end > >I will recode those statements.> 6. > +namespace llvm { MCObjectWriter * createWinCOFFObjectWriter > (raw_ostream & OS); } > > If you need a function to be visible across files, use a header; > shortcuts like this make the code more difficult to read. > >On this one, it didn't seem appropriate to include WinCOFFObjectWriters header into X86AsmBackend, it also seems to be overkill to add a header to contain a single function. I would like some more feedback on what the sanctioned course of action should be here. - Nathan -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100520/356470dc/attachment.html>
For those that are interested, I have attached the latest version of my Win32 COFF support patch. There is no new functionality, I just fixed some compiler errors due to recent changes in the MC library. - Nathan -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100527/23598d23/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: changes.patch Type: application/octet-stream Size: 49220 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100527/23598d23/attachment.obj>
Nathan Jeffords <blunted2night at gmail.com> writes:> For those that are interested, I have attached the latest version of my > Win32 COFF support patch. There is no new functionality, I just fixed some > compiler errors due to recent changes in the MC library.Thanks Nathan. Any objections for not accepting the patch into LLVM? If none, I'll apply it after the weekend.
On 28 May 2010 05:59, Nathan Jeffords <blunted2night at gmail.com> wrote:> For those that are interested, I have attached the latest version of my > Win32 COFF support patch. There is no new functionality, I just fixed some > compiler errors due to recent changes in the MC library. > > Nathan,Sorry about this but... On Cygwin with GCC-4.2.4 :- In file included from /home/ang/svn/llvm/lib/MC/WinCOFFObjectWriter.cpp:32: /home/ang/svn/llvm/lib/MC/WinCoff.h:68: error: comma at end of enumerator list Otherwise builds okay. I'll run some tests on it... Aaron -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100528/12b37611/attachment.html>
On Thu, May 27, 2010 at 9:59 PM, Nathan Jeffords <blunted2night at gmail.com> wrote:> For those that are interested, I have attached the latest version of my > Win32 COFF support patch. There is no new functionality, I just fixed some > compiler errors due to recent changes in the MC library.One more review comment: + bool isVirtualSection(const MCSection &Section) const { +// const MCSectionCOFF &SE = static_cast<const MCSectionCOFF&>(Section); +// return SE.getType() == MCSectionCOFF::SHT_NOBITS; + + return false; // not sure how to interpret this right now + } Please get rid of the commented-out code and make the comment use FIXME so it's clear there's an issue here. -Eli