[llvm-commits] [PATCH] MC abstraction and stub patches

[Matt Fleming]

On Thu, 20 May 2010 18:27:18 -0700, Daniel Dunbar <daniel at zuster.org> wrote:
> On Sun, May 16, 2010 at 10:55 AM, Matt Fleming <matt at console-pimps.org> wrote:
> >
> > 0001-type-asm-directive.patch:
> > This patch adds support for the ELF .type directive.
> 
> Looks good, please apply.

Thanks, applied.

> > 0002-split-elfx86-asmbackend.patch:
> > This splits the ELF X86 AsmBackend classes into 64Bit and 32Bit which
> > will help when ELF support gets merged as the createObjectWriter
> > implementations will differ for 64Bit and 32Bit.
> 
> Looks good, please apply.

Thanks, applied.

> > 0003-target-streamer.patch:
> > Currently createMachOStreamer is invoked directly in llvm-mc which isn't
> > a good idea if we want to be able to use another object file
> > format. This patch adds a createStreamer() factory method to the correct
> > object file streamer to be instantiated for a given target triple.
> 
> Looks good, but can you change the functions to be
> "createObjectStreamer", etc? That makes more sense, and reflects a
> refactoring I am going to (eventually) do to pull out an
> MCObjectStreamer class, which will be shared by ELF, COFF, and MachO.

I've made your suggested changes and applied this.

> > 0004-section-abstraction.patch:
> >
> > Instead of calling getMachOSection() directly when parsing section
> > directives we should be asking the streamer backend to create the
> > correct section given a section name. One thing I didn't understand when
> > writing this patch was exactly how MCAsmStreamer should behave, hence
> > the FIXME in MCAsmStreamer::LookupSection(). I've left the current
> > behaviour in place, e.g. creating a MachOSection because I couldn't
> > think of what else to do. I'm guessing the type of Section doesn't
> > really matter?
> 
> I'd like to go a different direction on this. What I think we should
> do is allow targets to define and/or override the directive
> processing. For very target dependent directives like .section, we
> will just assume that any target parser implements those directives.
> It doesn't seem worth trying to make abstract versions of these, and
> we know we will need other support for target dependent directives
> anyway.
>
> We should start by factoring out the current directive handling to be
> table driven, and then allow the target asm parser to override entries
> in that table. What do you think about this approach?

Sure, that makes sense. I'll look into it.

Thanks for the review and feedback, I know you guys are extremely busy
at the moment.