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

[Peter Collingbourne]

Thanks Tom, committed as r191343.

Peter

On Tue, Sep 24, 2013 at 01:29:06PM -0700, Tom Roeder wrote:
> OK, here's the fixed patch.
> 
> Thanks,
> 
> Tom
> 
> On Tue, Sep 24, 2013 at 11:25 AM, Daniel Dunbar <daniel at zuster.org> wrote:
> > 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
> >
> >



-- 
Peter