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? -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-target-asm-backend-add-reloc-info.patch Type: text/x-patch Size: 2378 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100512/e803a54c/attachment.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0002-mcelfstreamer.patch Type: text/x-patch Size: 46288 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100512/e803a54c/attachment-0001.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0003-hookup-x86-elf-writer.patch Type: text/x-patch Size: 1336 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100512/e803a54c/attachment-0002.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0004-type-asm-directive.patch Type: text/x-patch Size: 3952 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100512/e803a54c/attachment-0003.bin>
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 > >
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!