llvm dev - Jan 2017 - ThinLTO promotion is ending up with "invalid" IR before IR-Linking

> On Dec 23, 2016, at 9:34 AM, Teresa Johnson <tejohnson at google.com>
wrote:
> 
> 
> 
> On Thu, Dec 22, 2016 at 8:55 PM, Mehdi Amini <mehdi.amini at apple.com
<mailto:mehdi.amini at apple.com>> wrote:
> Hey,
> 
> As I’m playing with Metadata lazy-loading, I added a verifier right before
running the IRLinker in FunctionImport.cpp, and it does not pass (on current
trunk) in a few cases. One that I looked at ended up with aliases pointing to
available_externally functions for instance.
> 
> How do these look after IRLinking (in the dest module)? I looked at the
logic in renameModuleForThinLTO and all the conversions to available_externally
are predicated on it being in the provided GlobalsToImport set. But in
FunctionImport.cpp selectCallee() we specifically prevent importing of aliases
that would result in the aliasee becoming available_externally. Presumably the
resulting IRLinked dest module looks legit, otherwise we would have later
verifier failures.
So the source module is:

@weakalias = weak alias void (...), bitcast (void ()* @globalfunc1 to void
(...)*)
define void @globalfunc1() #0 {
entry:
  ret void
}

But we turn globalfunc1 into available_externally in renameModuleForThinLTO(),
which make the alias invalid.

We don’t import the IR so the destination module is OK.
36

> 
> 
> I’m hesitant about breaking the IR verifier like that before calling the
IRLinker. The alternative I can see now would be:
> - to perform any handling that requires breaking the verifier as part of
the IRLinker process,
> - or to perform the verifier-breaking changes that we do as a pre-process
step to the IRLinker as a post-process step on the linked module.
> 
> Wouldn't this second option result in verification errors on the
resulting dest module?
Not sure why? Since currently the destination module is valid, the post-process
should be OK as well?

— 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161223/87abc32b/attachment.html>

I just hit another issue: before of ODR type uniquing on bitcode loading, we can
end-up with a metadata graph that include metadata from the destination module,
including a DICompileUnit.
Unfortunately this compile-unit is not listed in llvm.dbg.cu and this does not
pass the verifier.

CC a few folks more aware of debug info that I am: is there a plan to make
llvm.dbg.cu disappear?

— 
Mehdi
> On Dec 23, 2016, at 11:04 AM, Mehdi Amini via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
>> On Dec 23, 2016, at 9:34 AM, Teresa Johnson <tejohnson at google.com
<mailto:tejohnson at google.com>> wrote:
>> 
>> 
>> 
>> On Thu, Dec 22, 2016 at 8:55 PM, Mehdi Amini <mehdi.amini at
apple.com <mailto:mehdi.amini at apple.com>> wrote:
>> Hey,
>> 
>> As I’m playing with Metadata lazy-loading, I added a verifier right
before running the IRLinker in FunctionImport.cpp, and it does not pass (on
current trunk) in a few cases. One that I looked at ended up with aliases
pointing to available_externally functions for instance.
>> 
>> How do these look after IRLinking (in the dest module)? I looked at the
logic in renameModuleForThinLTO and all the conversions to available_externally
are predicated on it being in the provided GlobalsToImport set. But in
FunctionImport.cpp selectCallee() we specifically prevent importing of aliases
that would result in the aliasee becoming available_externally. Presumably the
resulting IRLinked dest module looks legit, otherwise we would have later
verifier failures.
> 
> So the source module is:
> 
> @weakalias = weak alias void (...), bitcast (void ()* @globalfunc1 to void
(...)*)
> define void @globalfunc1() #0 {
> entry:
>   ret void
> }
> 
> But we turn globalfunc1 into available_externally in
renameModuleForThinLTO(), which make the alias invalid.
> 
> We don’t import the IR so the destination module is OK.
> 36
> 
> 
>> 
>> 
>> I’m hesitant about breaking the IR verifier like that before calling
the IRLinker. The alternative I can see now would be:
>> - to perform any handling that requires breaking the verifier as part
of the IRLinker process,
>> - or to perform the verifier-breaking changes that we do as a
pre-process step to the IRLinker as a post-process step on the linked module.
>> 
>> Wouldn't this second option result in verification errors on the
resulting dest module?
> 
> Not sure why? Since currently the destination module is valid, the
post-process should be OK as well?
> 
> — 
> Mehdi
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161223/428a2b57/attachment.html>

> On 2016-Dec-23, at 11:33, Mehdi Amini <mehdi.amini at apple.com>
wrote:
> 
> I just hit another issue: before of ODR type uniquing on bitcode loading,
we can end-up with a metadata graph that include metadata from the destination
module, including a DICompileUnit.
> Unfortunately this compile-unit is not listed in llvm.dbg.cu and this does
not pass the verifier.
The verifier won't pass until linking is complete, IIRC.  I had to turn off
per-module verification when I added the ODR-type uniquing logic. 
DICompositeTypes will, when created, "magically" coalesce with
DICompositeTypes in already-loaded modules in the same context that share a UUID
("identifier:").  That means they reference things in that other
module.

You need to finish linking before running the verifier.
> CC a few folks more aware of debug info that I am: is there a plan to make
llvm.dbg.cu disappear?
> 
> — 
> Mehdi
> 
>> On Dec 23, 2016, at 11:04 AM, Mehdi Amini via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>> 
>> 
>>> On Dec 23, 2016, at 9:34 AM, Teresa Johnson <tejohnson at
google.com> wrote:
>>> 
>>> 
>>> 
>>> On Thu, Dec 22, 2016 at 8:55 PM, Mehdi Amini <mehdi.amini at
apple.com> wrote:
>>> Hey,
>>> 
>>> As I’m playing with Metadata lazy-loading, I added a verifier right
before running the IRLinker in FunctionImport.cpp, and it does not pass (on
current trunk) in a few cases. One that I looked at ended up with aliases
pointing to available_externally functions for instance.
>>> 
>>> How do these look after IRLinking (in the dest module)? I looked at
the logic in renameModuleForThinLTO and all the conversions to
available_externally are predicated on it being in the provided GlobalsToImport
set. But in FunctionImport.cpp selectCallee() we specifically prevent importing
of aliases that would result in the aliasee becoming available_externally.
Presumably the resulting IRLinked dest module looks legit, otherwise we would
have later verifier failures.
>> 
>> So the source module is:
>> 
>> @weakalias = weak alias void (...), bitcast (void ()* @globalfunc1 to
void (...)*)
>> define void @globalfunc1() #0 {
>> entry:
>>   ret void
>> }
>> 
>> But we turn globalfunc1 into available_externally in
renameModuleForThinLTO(), which make the alias invalid.
>> 
>> We don’t import the IR so the destination module is OK.
>> 36
>> 
>> 
>>> 
>>> 
>>> I’m hesitant about breaking the IR verifier like that before
calling the IRLinker. The alternative I can see now would be:
>>> - to perform any handling that requires breaking the verifier as
part of the IRLinker process,
>>> - or to perform the verifier-breaking changes that we do as a
pre-process step to the IRLinker as a post-process step on the linked module.
>>> 
>>> Wouldn't this second option result in verification errors on
the resulting dest module?
>> 
>> Not sure why? Since currently the destination module is valid, the
post-process should be OK as well?
>> 
>> — 
>> Mehdi
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

We had the same problem with verification when merging/linking 2 modules: a
compile unit is not listed in named metadata (llvm.dbg.cu),

We worked around this problem by using the DebugInfoFinder processModule
function, which identifies all the compile units in the new module, by visiting
all the subprogram scopes and the already available compile units. However, this
succeeds verification only if the unnamed/missing compile units have children
sub programs. This is not a strong guarantee.


Anna


On Dec 23, 2016, at 2:33 PM, Mehdi Amini via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:

I just hit another issue: before of ODR type uniquing on bitcode loading, we can
end-up with a metadata graph that include metadata from the destination module,
including a DICompileUnit.
Unfortunately this compile-unit is not listed in llvm.dbg.cu and this does not
pass the verifier.

CC a few folks more aware of debug info that I am: is there a plan to make
llvm.dbg.cu disappear?

—
Mehdi

On Dec 23, 2016, at 11:04 AM, Mehdi Amini via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:


On Dec 23, 2016, at 9:34 AM, Teresa Johnson <tejohnson at
google.com<mailto:tejohnson at google.com>> wrote:



On Thu, Dec 22, 2016 at 8:55 PM, Mehdi Amini <mehdi.amini at
apple.com<mailto:mehdi.amini at apple.com>> wrote:
Hey,

As I’m playing with Metadata lazy-loading, I added a verifier right before
running the IRLinker in FunctionImport.cpp, and it does not pass (on current
trunk) in a few cases. One that I looked at ended up with aliases pointing to
available_externally functions for instance.

How do these look after IRLinking (in the dest module)? I looked at the logic in
renameModuleForThinLTO and all the conversions to available_externally are
predicated on it being in the provided GlobalsToImport set. But in
FunctionImport.cpp selectCallee() we specifically prevent importing of aliases
that would result in the aliasee becoming available_externally. Presumably the
resulting IRLinked dest module looks legit, otherwise we would have later
verifier failures.

So the source module is:

@weakalias = weak alias void (...), bitcast (void ()* @globalfunc1 to void
(...)*)
define void @globalfunc1() #0 {
entry:
  ret void
}

But we turn globalfunc1 into available_externally in renameModuleForThinLTO(),
which make the alias invalid.

We don’t import the IR so the destination module is OK.
36




I’m hesitant about breaking the IR verifier like that before calling the
IRLinker. The alternative I can see now would be:
- to perform any handling that requires breaking the verifier as part of the
IRLinker process,
- or to perform the verifier-breaking changes that we do as a pre-process step
to the IRLinker as a post-process step on the linked module.

Wouldn't this second option result in verification errors on the resulting
dest module?

Not sure why? Since currently the destination module is valid, the post-process
should be OK as well?

—
Mehdi
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161223/4f0a9220/attachment.html>

On Fri, Dec 23, 2016 at 11:04 AM, Mehdi Amini <mehdi.amini at apple.com>
wrote:
>
> On Dec 23, 2016, at 9:34 AM, Teresa Johnson <tejohnson at google.com>
wrote:
>
>
>
> On Thu, Dec 22, 2016 at 8:55 PM, Mehdi Amini <mehdi.amini at
apple.com>
> wrote:
>
>> Hey,
>>
>> As I’m playing with Metadata lazy-loading, I added a verifier right
>> before running the IRLinker in FunctionImport.cpp, and it does not pass
(on
>> current trunk) in a few cases. One that I looked at ended up with
aliases
>> pointing to available_externally functions for instance.
>>
>
> How do these look after IRLinking (in the dest module)? I looked at the
> logic in renameModuleForThinLTO and all the conversions to
> available_externally are predicated on it being in the provided
> GlobalsToImport set. But in FunctionImport.cpp selectCallee() we
> specifically prevent importing of aliases that would result in the aliasee
> becoming available_externally. Presumably the resulting IRLinked dest
> module looks legit, otherwise we would have later verifier failures.
>
>
> So the source module is:
>
> @weakalias = weak alias void (...), bitcast (void ()* @globalfunc1 to void
> (...)*)
> define void @globalfunc1() #0 {
> entry:
>   ret void
> }
>
> But we turn globalfunc1 into available_externally in
> renameModuleForThinLTO(), which make the alias invalid.
>
> We don’t import the IR so the destination module is OK.
>
Presumably we have decided to import globalfunc1 (otherwise
renameModuleForThinLTO would not convert its linkage type), but not
weakalias (since selectcallee should block it because aliases can't be
imported unless the aliasee is linkonceodr). And the dest module is ok
because we only import globalfunc1 and not weakalias. In that case, yes, I
guess the source module can't be verified after
doing renameModuleForThinLTO. Why not just verify before we
do renameModuleForThinLTO, and then the dest module again after IR linking?
Not sure it is worth verifying between those two points.

Except that as noted in the follow-on emails, type ODRing which kicks in
when the source module is loaded, will cause other issues.

Ah, see that you added the verification before renameModuleForThinLTO, then
removed it again due to the typeODR issue.

36
>
>
>
>
>> I’m hesitant about breaking the IR verifier like that before calling
the
>> IRLinker. The alternative I can see now would be:
>> - to perform any handling that requires breaking the verifier as part
of
>> the IRLinker process,
>> - or to perform the verifier-breaking changes that we do as a
pre-process
>> step to the IRLinker as a post-process step on the linked module.
>>
>
> Wouldn't this second option result in verification errors on the
resulting
> dest module?
>
>
> Not sure why? Since currently the destination module is valid, the
> post-process should be OK as well?
>
I misunderstood what you were saying - you mentioned applying
"verifier-breaking changes" as a post-process, but the key difference
is
that they are not verifier-breaking at that point.

I'm not sure it is worth making these changes, especially since there are
other reasons (type ODR) that prevent verification until that point.

Teresa

> —
> Mehdi
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
<(408)%20460-2413>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161227/1bf19d48/attachment.html>

> On Dec 27, 2016, at 7:29 AM, Teresa Johnson <tejohnson at google.com>
wrote:
> 
> 
> 
> On Fri, Dec 23, 2016 at 11:04 AM, Mehdi Amini <mehdi.amini at apple.com
<mailto:mehdi.amini at apple.com>> wrote:
> 
>> On Dec 23, 2016, at 9:34 AM, Teresa Johnson <tejohnson at google.com
<mailto:tejohnson at google.com>> wrote:
>> 
>> 
>> 
>> On Thu, Dec 22, 2016 at 8:55 PM, Mehdi Amini <mehdi.amini at
apple.com <mailto:mehdi.amini at apple.com>> wrote:
>> Hey,
>> 
>> As I’m playing with Metadata lazy-loading, I added a verifier right
before running the IRLinker in FunctionImport.cpp, and it does not pass (on
current trunk) in a few cases. One that I looked at ended up with aliases
pointing to available_externally functions for instance.
>> 
>> How do these look after IRLinking (in the dest module)? I looked at the
logic in renameModuleForThinLTO and all the conversions to available_externally
are predicated on it being in the provided GlobalsToImport set. But in
FunctionImport.cpp selectCallee() we specifically prevent importing of aliases
that would result in the aliasee becoming available_externally. Presumably the
resulting IRLinked dest module looks legit, otherwise we would have later
verifier failures.
> 
> So the source module is:
> 
> @weakalias = weak alias void (...), bitcast (void ()* @globalfunc1 to void
(...)*)
> define void @globalfunc1() #0 {
> entry:
>   ret void
> }
> 
> But we turn globalfunc1 into available_externally in
renameModuleForThinLTO(), which make the alias invalid.
> 
> We don’t import the IR so the destination module is OK.
> 
> Presumably we have decided to import globalfunc1 (otherwise
renameModuleForThinLTO would not convert its linkage type), but not weakalias
(since selectcallee should block it because aliases can't be imported unless
the aliasee is linkonceodr). And the dest module is ok because we only import
globalfunc1 and not weakalias. In that case, yes, I guess the source module
can't be verified after doing renameModuleForThinLTO. Why not just verify
before we do renameModuleForThinLTO, and then the dest module again after IR
linking? Not sure it is worth verifying between those two points.
> 
> Except that as noted in the follow-on emails, type ODRing which kicks in
when the source module is loaded, will cause other issues.
> 
> Ah, see that you added the verification before renameModuleForThinLTO, then
removed it again due to the typeODR issue.
Right, this was extremely annoying was bringing up the on-demand loading of
metadata. Being able to validate the IR after loading is really important in
this case.

>> I’m hesitant about breaking the IR verifier like that before calling
the IRLinker. The alternative I can see now would be:
>> - to perform any handling that requires breaking the verifier as part
of the IRLinker process,
>> - or to perform the verifier-breaking changes that we do as a
pre-process step to the IRLinker as a post-process step on the linked module.
>> 
>> Wouldn't this second option result in verification errors on the
resulting dest module?
> 
> Not sure why? Since currently the destination module is valid, the
post-process should be OK as well?
> 
> I misunderstood what you were saying - you mentioned applying
"verifier-breaking changes" as a post-process, but the key difference
is that they are not verifier-breaking at that point.
> 
> I'm not sure it is worth making these changes, especially since there
are other reasons (type ODR) that prevent verification until that point.
I don’t have an immediate need / issue, but since it is not usual to manipulate
“incorrect IR” in a pass or a transformation utility “by design", I figured
I’ll bring up this issue.

— 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161228/8c26dc2f/attachment.html>

> On Dec 23, 2016, at 11:33 AM, Mehdi Amini <mehdi.amini at apple.com>
wrote:
> 
> CC a few folks more aware of debug info that I am: is there a plan to make
llvm.dbg.cu disappear?
No. The llvm.dbg.cu named MDNode is the one IR node that (via their owning
DICompileUnits) points to debug info for entities that have been optimized away.

-- adrian