[LLVMdev] Win32 COFF Support

[Nathan Jeffords]

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>