[LLVMdev] MC ELF support

[Daniel Dunbar]

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:
--
+  if (Type.equals(StringRef("function"))) {
+    Attr = MCSA_ELF_TypeFunction;
+  } else if (Type.equals(StringRef("object"))) {
+    Attr = MCSA_ELF_TypeObject;
--

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

 - Daniel

On Tue, May 11, 2010 at 4:48 PM, Matt Fleming <matt at console-pimps.org> wrote:
>
> 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?
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>