MC ELF support

Hi guys,

attached are a couple of work in progress patches for ELF support in the
MC module. I'm sending this email to gather some general feedback on the
code. Applying these patches doesn't get you a fully working llvm-mc
that understands ELF; it's just the ground work. I've got a couple more
small patches that fixup some places that assume Mach-O object format
which I'll send later in the week.

0001-target-asm-backend-add-reloc-info.patch:

This patch adds information to allow MC to handle ELF relocations.

0002-mcelfstreamer.patch:

This is the largest patch. It fleshes out the ELF support.

0003-hookup-x86-elf-writer.patch:

Code to get the ELF writer working with x86.

0004-type-asm-directive.patch:

This patch allows the assembler to handle the .type directive.

Comments?

0001-target-asm-backend-add-reloc-info.patch (2.32 KB)

0002-mcelfstreamer.patch (45.2 KB)

0003-hookup-x86-elf-writer.patch (1.3 KB)

0004-type-asm-directive.patch (3.86 KB)

Hi Matt,

Awesome! This is excellent progress, and it is great to see someone
working on this.

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.

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.
- 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.

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.
  - 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.
  - 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.

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

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

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 :wink: 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!