[LLVMdev] MC ELF support

[Matt Fleming]

On Tue, 11 May 2010 19:03:27 -0700, Daniel Dunbar <daniel at zuster.org> wrote:
> 
> High level comments:
>  (1) The order of patches is odd, to me. It would be great to start by
> adding the AsmParser support, then the MCStreamer support, so that we
> can have test cases in the 'llvm-mc cat' mode, where it just parses
> and prints out again.

The order of the patches is a result of me trying to not break the
build. I can certainly rework them if you'd prefer.

> On 0001:
>  - What is hasRelocationAddend? It doesn't seem to be needed to me,
> and I'm not sure why it would be. If this is a private detail to ELF,
> it shouldn't go in TargetAsmBackend, but rather be an argument to the
> object writer constructor.

The reason I put it in TargetAsmBackend is because the concept of a
relocation addend isn't specific to ELF; ELF is just the only object
format that uses it ;-) I'll make this an argument to the object writer
constructor.

>  - Feel free to submit a patch to split out ELFX86_{32,64}AsmBackend,
> if you want me to apply it. Little / obvious patches like that are
> great ones to get in first, and make subsequent patches easier to
> read.

Sure. I'll write a patch for that.

> On 0002:
>   - Looks great, overall!
>   - WriteSymbolEntry isn't endianness neutral. I would find it easier
> to read if Is64Bit weren't the top level branch but was only used
> where it matters, but that is me.

D'oh! Yeah, the endianness is broken. I'll fix that. Changing the
branching will complicate this code quite a bit as not only are the
sizes of the fields different for 64-bit and 32-bit, the layout of the
fields is different also.

>   - LLVM style is to use assert(... && "Message to include in the assert").
>   - Please use array_pod_sort (from ADT/STLExtras.h) instead of std::sort.
>   - WriteSecHdrEntry would be easier to read if it just used WriteWord
> instead of Is64Bit.

Good points. I'll fix these up.

>   - The changes to MCSectionELF shouldn't be needed. These should go
> in MCSectionData instead, or in private maps if possible. I can give
> you extra target dependent fields if need be. This enforces layering
> between the parts CodeGen can access and the parts that are private to
> the assembler backend.

OK. I'll move this into MCSectionData, which will allow us to handle the
.align directive.

> On 0003:
>  - Might as well merge this with 0002.

Will do!

> On 0004:
>   - This looks good, might as well bring it in first.
>   - You could use ADT/StringSwitch for this sequence, if you like:

Will do!

> I recommend optimizing for getting the obvious parts or stub
> implementations in first, so it is easy to review subsequent patches.

Sure, sounds sensible.

Thanks very much for the review!