[llvm] r191042 - Fix LTO handling of module-level assembly (PR14152).
tmroeder at google.com
Tue Sep 24 13:29:06 PDT 2013
OK, here's the fixed patch.
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>
>> > 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 134479 bytes
Desc: not available
More information about the llvm-commits