llvm dev - Jul 2016 - [ThinLTO] Using two different IRMovers for the same composite module? (related to PR28180)

It seems that the patch works for me as well, though the linker crashes with
another error after that. Thanks!

Mehdi, I couldn’t quite understand what do you mean by you don’t have a repro so
you couldn’t upstream the patch. Aren’t .ll files you attached sufficient to
submit along with the patch? If there is anything I can help you to upstream it,
please let me know.

-- Taewook

On 7/22/16, 2:52 PM, "mehdi.amini at apple.com on behalf of Mehdi
Amini" <mehdi.amini at apple.com> wrote:

> On Jul 22, 2016, at 2:47 PM, Davide Italiano <davide at freebsd.org>
wrote:
> 
> On Fri, Jul 22, 2016 at 2:44 PM, Davide Italiano <davide at
freebsd.org> wrote:
>> On Fri, Jul 22, 2016 at 2:34 PM, Taewook Oh via llvm-dev
>> <llvm-dev at lists.llvm.org> wrote:
>>> Yes, I have the repro, though I can’t publish it externally. It
would be
>>> great if you can upstream the patch so I can try it. Thank you for
your
>>> explanation as well!
>> 
>> There's a reproducer attached (obtained via lld --reproduce
option).
>> If that doesn't work, you can checkout mozjs and try to reduce from
>> there.
>> It happens while doing an LTO build with lld.
>> I have a fix for that (Mehdi has one as well, apparently) in my local
>> tree, but I don't have time to reduce. If you can take care of
that,
>> chances are that an upstream fix will be committed shortly after.
>> 
> 
> Just to clarify, I'm talking about the issue in PR28180. I can't
> comment about the ThinLTO one, sorry.
If lld is setting enableDebugTypeODRUniquing(); on the context and isn’t using
the IRMover to target an empty module, it can be the same bug.
I mentioned that it should touch only ThinLTO but I had ld64 in mind.

— 
Mehdi

> On Jul 22, 2016, at 4:42 PM, Taewook Oh <twoh at fb.com> wrote:
> 
> It seems that the patch works for me as well, though the linker crashes
with another error after that. Thanks!
> 
> Mehdi, I couldn’t quite understand what do you mean by you don’t have a
repro so you couldn’t upstream the patch. Aren’t .ll files you attached
sufficient to submit along with the patch? If there is anything I can help you
to upstream it, please let me know.
I could submit the patch as-is but it is not in my habit to do that without a
total understanding of the situation, i.e. I’m not convinced the test case is
totally reduced and I need to “reverse engineer” the debug metadata to get a
source-code construct that would trigger this bug.

— 
Mehdi




> 
> -- Taewook
> 
> On 7/22/16, 2:52 PM, "mehdi.amini at apple.com on behalf of Mehdi
Amini" <mehdi.amini at apple.com> wrote:
> 
> 
>> On Jul 22, 2016, at 2:47 PM, Davide Italiano <davide at
freebsd.org> wrote:
>> 
>> On Fri, Jul 22, 2016 at 2:44 PM, Davide Italiano <davide at
freebsd.org> wrote:
>>> On Fri, Jul 22, 2016 at 2:34 PM, Taewook Oh via llvm-dev
>>> <llvm-dev at lists.llvm.org> wrote:
>>>> Yes, I have the repro, though I can’t publish it externally. It
would be
>>>> great if you can upstream the patch so I can try it. Thank you
for your
>>>> explanation as well!
>>> 
>>> There's a reproducer attached (obtained via lld --reproduce
option).
>>> If that doesn't work, you can checkout mozjs and try to reduce
from
>>> there.
>>> It happens while doing an LTO build with lld.
>>> I have a fix for that (Mehdi has one as well, apparently) in my
local
>>> tree, but I don't have time to reduce. If you can take care of
that,
>>> chances are that an upstream fix will be committed shortly after.
>>> 
>> 
>> Just to clarify, I'm talking about the issue in PR28180. I
can't
>> comment about the ThinLTO one, sorry.
> 
> If lld is setting enableDebugTypeODRUniquing(); on the context and isn’t
using the IRMover to target an empty module, it can be the same bug.
> I mentioned that it should touch only ThinLTO but I had ld64 in mind.
> 
> — 
> Mehdi
> 
>

Makes sense. I’ll try to get the code as well. 

Thanks,
Taewook

On 7/22/16, 6:13 PM, "mehdi.amini at apple.com on behalf of Mehdi
Amini" <mehdi.amini at apple.com> wrote:

> On Jul 22, 2016, at 4:42 PM, Taewook Oh <twoh at fb.com> wrote:
> 
> It seems that the patch works for me as well, though the linker crashes
with another error after that. Thanks!
> 
> Mehdi, I couldn’t quite understand what do you mean by you don’t have a
repro so you couldn’t upstream the patch. Aren’t .ll files you attached
sufficient to submit along with the patch? If there is anything I can help you
to upstream it, please let me know.
I could submit the patch as-is but it is not in my habit to do that without a
total understanding of the situation, i.e. I’m not convinced the test case is
totally reduced and I need to “reverse engineer” the debug metadata to get a
source-code construct that would trigger this bug.

— 
Mehdi




> 
> -- Taewook
> 
> On 7/22/16, 2:52 PM, "mehdi.amini at apple.com on behalf of Mehdi
Amini" <mehdi.amini at apple.com> wrote:
> 
> 
>> On Jul 22, 2016, at 2:47 PM, Davide Italiano <davide at
freebsd.org> wrote:
>> 
>> On Fri, Jul 22, 2016 at 2:44 PM, Davide Italiano <davide at
freebsd.org> wrote:
>>> On Fri, Jul 22, 2016 at 2:34 PM, Taewook Oh via llvm-dev
>>> <llvm-dev at lists.llvm.org> wrote:
>>>> Yes, I have the repro, though I can’t publish it externally. It
would be
>>>> great if you can upstream the patch so I can try it. Thank you
for your
>>>> explanation as well!
>>> 
>>> There's a reproducer attached (obtained via lld --reproduce
option).
>>> If that doesn't work, you can checkout mozjs and try to reduce
from
>>> there.
>>> It happens while doing an LTO build with lld.
>>> I have a fix for that (Mehdi has one as well, apparently) in my
local
>>> tree, but I don't have time to reduce. If you can take care of
that,
>>> chances are that an upstream fix will be committed shortly after.
>>> 
>> 
>> Just to clarify, I'm talking about the issue in PR28180. I
can't
>> comment about the ThinLTO one, sorry.
> 
> If lld is setting enableDebugTypeODRUniquing(); on the context and isn’t
using the IRMover to target an empty module, it can be the same bug.
> I mentioned that it should touch only ThinLTO but I had ld64 in mind.
> 
> — 
> Mehdi
> 
>

I just filed a bug (https://llvm.org/bugs/show_bug.cgi?id=30248), which is
different from the one discussed in this thread but seems to have a same root. I
attached a small repro as well.

As you said, assertion fails if the mapper tries to map a metadata that already
in the destination module, when the same metadata is reached from the source
module. More specifically, it is possible that a function parameter V of
Mapper::mapValue in lib/Transforms/utils/ValueMapper.cpp is already in the
destination module. Still, V is forced to be (re)materialized into the
destination module, and during that process the original V can be erase from the
module while NewV is created (last if statement of
IRLinker::linkGlobalValueProto in lib/Linker/IRMover.cpp). For such case ),
“getVM()[V] = NewV” statement in Mapper::mapValue is invalid.

I confirmed that the bug can be fixed by your internal fix that you’ve attached
to this thread.

Thanks,
Taewook  

On 7/23/16, 10:13 AM, "mehdi.amini at apple.com on behalf of Mehdi
Amini" <mehdi.amini at apple.com> wrote:

    
    > On Jul 22, 2016, at 4:42 PM, Taewook Oh <twoh at fb.com> wrote:
    > 
    > It seems that the patch works for me as well, though the linker crashes
with another error after that. Thanks!
    > 
    > Mehdi, I couldn’t quite understand what do you mean by you don’t have a
repro so you couldn’t upstream the patch. Aren’t .ll files you attached
sufficient to submit along with the patch? If there is anything I can help you
to upstream it, please let me know.
    
    I could submit the patch as-is but it is not in my habit to do that without
a total understanding of the situation, i.e. I’m not convinced the test case is
totally reduced and I need to “reverse engineer” the debug metadata to get a
source-code construct that would trigger this bug.
    
    — 
    Mehdi
    
    
    
    
    
    > 
    > -- Taewook
    > 
    > On 7/22/16, 2:52 PM, "mehdi.amini at apple.com on behalf of Mehdi
Amini" <mehdi.amini at apple.com> wrote:
    > 
    > 
    >> On Jul 22, 2016, at 2:47 PM, Davide Italiano <davide at
freebsd.org> wrote:
    >> 
    >> On Fri, Jul 22, 2016 at 2:44 PM, Davide Italiano <davide at
freebsd.org> wrote:
    >>> On Fri, Jul 22, 2016 at 2:34 PM, Taewook Oh via llvm-dev
    >>> <llvm-dev at lists.llvm.org> wrote:
    >>>> Yes, I have the repro, though I can’t publish it
externally. It would be
    >>>> great if you can upstream the patch so I can try it. Thank
you for your
    >>>> explanation as well!
    >>> 
    >>> There's a reproducer attached (obtained via lld --reproduce
option).
    >>> If that doesn't work, you can checkout mozjs and try to
reduce from
    >>> there.
    >>> It happens while doing an LTO build with lld.
    >>> I have a fix for that (Mehdi has one as well, apparently) in my
local
    >>> tree, but I don't have time to reduce. If you can take care
of that,
    >>> chances are that an upstream fix will be committed shortly
after.
    >>> 
    >> 
    >> Just to clarify, I'm talking about the issue in PR28180. I
can't
    >> comment about the ThinLTO one, sorry.
    > 
    > If lld is setting enableDebugTypeODRUniquing(); on the context and
isn’t using the IRMover to target an empty module, it can be the same bug.
    > I mentioned that it should touch only ThinLTO but I had ld64 in mind.
    > 
    > — 
    > Mehdi
    > 
    >