[llvm] r191042 - Fix LTO handling of module-level assembly (PR14152).

[Daniel Dunbar]

Hi Tom,

Thanks for the changes. A couple comments:

1. It would be cleaner to move the initialized variable inside
lto_initialize, and call it unconditionally in each LTO interface function.
Also, it should probably be a static  function (unless we add it to the
public interface in lto.h, but I don't see a need to do that now).

2. > +  // initialize the configured targets
Initial caps and punctuation in comments please.

Otherwise LGTM, thanks!

 - Daniel


On Mon, Sep 23, 2013 at 5:09 PM, Tom Roeder <tmroeder at google.com> wrote:

> On Mon, Sep 23, 2013 at 4:30 PM, Peter Collingbourne <peter at pcc.me.uk>
> wrote:
> > On Mon, Sep 23, 2013 at 04:06:50PM -0700, Tom Roeder wrote:
> >> Please take a look.
> >
> > Hi Tom,
> >
> >>  /**
> >> - * Initializes LLVM disassemblers.
> >> - * FIXME: This doesn't really belong here.
> >> + * Initializes LLVM Targets for lto. This must be called before
> >> + * creating an lto_code_gen_t or lto_module_t.
> >>   */
> >>  extern void
> >> -lto_initialize_disassembler(void);
> >> +lto_initialize(void);
> >
> > I don't think this is right.  libLTO is a stable external interface used
> > by Apple's linker.  Can you make the lto_codegen_create and
> lto_module_create*
> > functions do the target initialization if needed?
> >
> > Thanks,
> > --
> > Peter
>
> Ah, thanks, I didn't know that.
>
> I've restored the LTODisassembler and its associated call in
> llvm-c/lto.h, and I've added a static variable "initialized" to
> capture the initialization state of the lto library as well as adding
> initialization calls in the module/code_gen creation paths. It feels a
> little kludgy, but the tools/lto library has already bought the notion
> of static state with the variable sLastErrorString, and I don't see
> another way of keeping this state short of changing the interface.
> According to the headers, it's legit to call the InitializeAll*
> functions multiple times, but I don't think we want to have to do it
> for every module handled by libLTO.so.
>
> Here's the changed version.
>
> Thanks,
>
> Tom
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130924/f0d64a0e/attachment.html>