llvm dev - Sep 2017 - [ThinLTO] static library failure with object files with the same name

I've created a review for your patch Mehdi: https://reviews.llvm.org/D37961

First time using `arc`, so hope things went well.

- Johan


On Tue, Sep 12, 2017 at 5:25 AM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
> Hi Johan,
>
> 2017-09-11 14:21 GMT-07:00 Johan Engelen <jbc.engelen at gmail.com>:
>
>> On Fri, Sep 8, 2017 at 9:04 PM, Johan Engelen <jbc.engelen at
gmail.com>
>> wrote:
>>
>>>
>>> On Thu, Sep 7, 2017 at 5:44 PM, Mehdi AMINI <joker.eph at
gmail.com> wrote:
>>>
>>>> Hi Johan,
>>>>
>>>> ld64 only calls functions from llvm/include/llvm-c/lto.h 
(defined
>>>> in llvm/tools/lto/lto.cpp)
>>>>
>>>> For instance ThinLTOCodeGenerator::addModule is called
>>>> through thinlto_codegen_add_module().
>>>>
>>>> Apple hasn't released the code for ld64 in Xcode 9 yet, did
you check
>>>> if it is fixed in Xcode 9?
>>>> (I think I remember fixing it in ld64 but I'm not totally
sure...).
>>>>
>>>
>>> I haven't tried with Xcode 9.
>>>
>>>
>>>>
>>>> From what I can see with Xcode 8.2, the linker just passes the
file
>>>> name (prefixed with the archive name):
https://github.com/michaelweis
>>>> er/ld64/blob/master/src/ld/parsers/lto_file.cpp#L546
>>>> (original here:
https://opensource.apple.com/source/ld64/ld64-274.2/sr
>>>> c/ld/parsers/lto_file.cpp.auto.html )
>>>>
>>>> We could workaround this in ThinLTOCodeGenerator by adding a
>>>> incremental suffix to every new buffer. Something like this
diff:
>>>>
>>>
>>> I was assuming that we do want to assert/error on calling addModule
with
>>> the exact same module twice? Otherwise your diff is nice, thanks.
>>>
>>>
>>
>> Hi Mehdi,
>>   Can you advise me?
>> Is it OK to not error upon the exact same module being added twice?
(and
>> thus your patch would be good)
>>
>>
> It is a matter of API contract, so I'm not sure I perceive a clear
> right/wrong here. If we want to enforce an API contract on the caller's
> responsibility to provide a unique ID, then the assert may be OK (however
> we disable assertions in production and it means random crashes which
isn't
> super user-friendly).
> On the other hand my patch here was trying to be resilient to API misuse
> for "little cost".
>
> Note that the plan was that this API (and the ThinLTOCodegenerator.cpp)
> should just die, but unfortunately I haven't had time to work on the
> replacement (a C binding for the new LTO API).
>
> --
> Mehdi
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170917/64cfb9d6/attachment.html>

The fix (https://reviews.llvm.org/D37961) does not work.  From what I have
learned thusfar, the module identifier is used as filename sometimes (I
think when writing an intermediate module index summary), and so a bunch of
lit tests fail with the "fix".

I'll look further into fixing this, any help is appreciated.

( One thing that may be important is to have a deterministic suffix. The
counter solution of https://reviews.llvm.org/D37961 would name the same
module differently depending on order, which may happen when doing thinlto
in several different steps. Adding the size of the module (or its offset)
would be the same in each invocation. Regardless, changing the suffix to a
size suffix does not fix the lit test problems. )

regards,
  Johan



On Sun, Sep 17, 2017 at 6:32 PM, Johan Engelen <jbc.engelen at gmail.com>
wrote:
> I've created a review for your patch Mehdi: https://reviews.llvm.
> org/D37961
>
> First time using `arc`, so hope things went well.
>
> - Johan
>
>
> On Tue, Sep 12, 2017 at 5:25 AM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
>
>> Hi Johan,
>>
>> 2017-09-11 14:21 GMT-07:00 Johan Engelen <jbc.engelen at
gmail.com>:
>>
>>> On Fri, Sep 8, 2017 at 9:04 PM, Johan Engelen <jbc.engelen at
gmail.com>
>>> wrote:
>>>
>>>>
>>>> On Thu, Sep 7, 2017 at 5:44 PM, Mehdi AMINI <joker.eph at
gmail.com>
>>>> wrote:
>>>>
>>>>> Hi Johan,
>>>>>
>>>>> ld64 only calls functions from llvm/include/llvm-c/lto.h 
(defined
>>>>> in llvm/tools/lto/lto.cpp)
>>>>>
>>>>> For instance ThinLTOCodeGenerator::addModule is called
>>>>> through thinlto_codegen_add_module().
>>>>>
>>>>> Apple hasn't released the code for ld64 in Xcode 9 yet,
did you check
>>>>> if it is fixed in Xcode 9?
>>>>> (I think I remember fixing it in ld64 but I'm not
totally sure...).
>>>>>
>>>>
>>>> I haven't tried with Xcode 9.
>>>>
>>>>
>>>>>
>>>>> From what I can see with Xcode 8.2, the linker just passes
the file
>>>>> name (prefixed with the archive name):
https://github.com/michaelweis
>>>>> er/ld64/blob/master/src/ld/parsers/lto_file.cpp#L546
>>>>> (original here:
https://opensource.apple.com/source/ld64/ld64-274.2/sr
>>>>> c/ld/parsers/lto_file.cpp.auto.html )
>>>>>
>>>>> We could workaround this in ThinLTOCodeGenerator by adding
a
>>>>> incremental suffix to every new buffer. Something like this
diff:
>>>>>
>>>>
>>>> I was assuming that we do want to assert/error on calling
addModule
>>>> with the exact same module twice? Otherwise your diff is nice,
thanks.
>>>>
>>>>
>>>
>>> Hi Mehdi,
>>>   Can you advise me?
>>> Is it OK to not error upon the exact same module being added twice?
(and
>>> thus your patch would be good)
>>>
>>>
>> It is a matter of API contract, so I'm not sure I perceive a clear
>> right/wrong here. If we want to enforce an API contract on the
caller's
>> responsibility to provide a unique ID, then the assert may be OK
(however
>> we disable assertions in production and it means random crashes which
isn't
>> super user-friendly).
>> On the other hand my patch here was trying to be resilient to API
misuse
>> for "little cost".
>>
>> Note that the plan was that this API (and the ThinLTOCodegenerator.cpp)
>> should just die, but unfortunately I haven't had time to work on
the
>> replacement (a C binding for the new LTO API).
>>
>> --
>> Mehdi
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170918/c5554afb/attachment.html>

It is expected and not unusual to need to update the lit test in such case.
I'd need to see exactly which test breaks and how to know though.

Best,

-- 
Mehdi

2017-09-18 13:17 GMT-07:00 Johan Engelen <jbc.engelen at gmail.com>:
> The fix (https://reviews.llvm.org/D37961) does not work.  From what I
> have learned thusfar, the module identifier is used as filename sometimes
> (I think when writing an intermediate module index summary), and so a bunch
> of lit tests fail with the "fix".
>
> I'll look further into fixing this, any help is appreciated.
>
> ( One thing that may be important is to have a deterministic suffix. The
> counter solution of https://reviews.llvm.org/D37961 would name the same
> module differently depending on order, which may happen when doing thinlto
> in several different steps. Adding the size of the module (or its offset)
> would be the same in each invocation. Regardless, changing the suffix to a
> size suffix does not fix the lit test problems. )
>
> regards,
>   Johan
>
>
>
> On Sun, Sep 17, 2017 at 6:32 PM, Johan Engelen <jbc.engelen at
gmail.com>
> wrote:
>
>> I've created a review for your patch Mehdi: https://reviews.llvm.or
>> g/D37961
>>
>> First time using `arc`, so hope things went well.
>>
>> - Johan
>>
>>
>> On Tue, Sep 12, 2017 at 5:25 AM, Mehdi AMINI <joker.eph at
gmail.com> wrote:
>>
>>> Hi Johan,
>>>
>>> 2017-09-11 14:21 GMT-07:00 Johan Engelen <jbc.engelen at
gmail.com>:
>>>
>>>> On Fri, Sep 8, 2017 at 9:04 PM, Johan Engelen <jbc.engelen
at gmail.com>
>>>> wrote:
>>>>
>>>>>
>>>>> On Thu, Sep 7, 2017 at 5:44 PM, Mehdi AMINI <joker.eph
at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hi Johan,
>>>>>>
>>>>>> ld64 only calls functions from
llvm/include/llvm-c/lto.h  (defined
>>>>>> in llvm/tools/lto/lto.cpp)
>>>>>>
>>>>>> For instance ThinLTOCodeGenerator::addModule is called
>>>>>> through thinlto_codegen_add_module().
>>>>>>
>>>>>> Apple hasn't released the code for ld64 in Xcode 9
yet, did you check
>>>>>> if it is fixed in Xcode 9?
>>>>>> (I think I remember fixing it in ld64 but I'm not
totally sure...).
>>>>>>
>>>>>
>>>>> I haven't tried with Xcode 9.
>>>>>
>>>>>
>>>>>>
>>>>>> From what I can see with Xcode 8.2, the linker just
passes the file
>>>>>> name (prefixed with the archive name):
https://github.com/michaelweis
>>>>>> er/ld64/blob/master/src/ld/parsers/lto_file.cpp#L546
>>>>>> (original here: https://opensource.apple
>>>>>>
.com/source/ld64/ld64-274.2/src/ld/parsers/lto_file.cpp.auto.html )
>>>>>>
>>>>>> We could workaround this in ThinLTOCodeGenerator by
adding a
>>>>>> incremental suffix to every new buffer. Something like
this diff:
>>>>>>
>>>>>
>>>>> I was assuming that we do want to assert/error on calling
addModule
>>>>> with the exact same module twice? Otherwise your diff is
nice, thanks.
>>>>>
>>>>>
>>>>
>>>> Hi Mehdi,
>>>>   Can you advise me?
>>>> Is it OK to not error upon the exact same module being added
twice?
>>>> (and thus your patch would be good)
>>>>
>>>>
>>> It is a matter of API contract, so I'm not sure I perceive a
clear
>>> right/wrong here. If we want to enforce an API contract on the
caller's
>>> responsibility to provide a unique ID, then the assert may be OK
(however
>>> we disable assertions in production and it means random crashes
which isn't
>>> super user-friendly).
>>> On the other hand my patch here was trying to be resilient to API
misuse
>>> for "little cost".
>>>
>>> Note that the plan was that this API (and the
ThinLTOCodegenerator.cpp)
>>> should just die, but unfortunately I haven't had time to work
on the
>>> replacement (a C binding for the new LTO API).
>>>
>>> --
>>> Mehdi
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170918/6358431b/attachment.html>