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

[Tom Roeder]

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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lib_lto.patch
Type: application/octet-stream
Size: 134479 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130924/3ae0bc9b/attachment.obj>