thr3ads.net - llvm dev - [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in FunctionImport.cpp [Jul 2016]

If this information is useful, please help other people find it:
Share via:

Taewook Oh via llvm-dev

2016-Jul-29 22:40 UTC

[llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in FunctionImport.cpp

It was r274523. I’m not sure it was the same module. By mistake I restarted the
build with the previous version without backing backing up the build artifacts
:(

Thanks,
Taewook

From: Teresa Johnson <tejohnson at google.com>
Date: Friday, July 29, 2016 at 3:30 PM
To: Taewook Oh <twoh at fb.com>
Cc: via llvm-dev <llvm-dev at lists.llvm.org>
Subject: Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in
FunctionImport.cpp

On Fri, Jul 29, 2016 at 2:25 PM, Taewook Oh <twoh at fb.com<mailto:twoh at
fb.com>> wrote:

Hello Teresa,

Thank you for your analysis. One thing to note is that the global materializer
materializes the value as a function declaration, not a function definition. As
I pasted on my first email,

; Materializable

; Function Attrs: nounwind uwtable

define weak_odr void @foo(%1*) unnamed_addr #7 comdat($comdat1) align 2
personality i32 (...)* @__gxx_personality_v0 {}

is materialized to

; Function Attrs: nounwind uwtable

declare void @foo(%"type1"*) unnamed_addr #2 align 2

Inside IRLinker::linkGlobalValueProto, the materialized value is returned from
getLinkedToGlobal(SGV) and assigned to DGV. However, as ForAlias is true and
ShouldLink is false, DGV becomes nullptr later, and NewGV is created from
copyGlobalValueProto(SGV, ShouldLink) call. Therefore, returned value from
linkGlobalValueProto is different from the value obtained by
ValueMap.lookup(SGV) in IRLinker::materialize.

Ah, sorry, somehow I completely missed the fact that it was brought in as a decl
when I looked this morning.

BTW, I updated LLVM to the latest version in SVN this morning, and the assertion
failure is gone. Instead, it ends up with broken module found errors:

Global is external, but doesn't have external or weak linkage!

void (%2*, %2*, i32)* @"_foo.llvm.E5F1952C"

function declaration may not have a !dbg attachment

void (%2*, %2*, i32)* @"_foo.llvm.E5F1952C"

LLVM ERROR: Broken module found, compilation aborted!

This looks like a promoted local from the name. What is the linkage type? Is
this the verifier run right after the importing pass?

I'm guessing the first problem went away due to luck, as I can't think
of any specific change that would have affected it. Is this from the same
module?

Do you know what SVN revision were you at before, even roughly?

Teresa

Hope this provides additional clues to fix the issue.

Thanks,

Taewook

From: Teresa Johnson <tejohnson at google.com<mailto:tejohnson at
google.com>>
Date: Friday, July 29, 2016 at 7:05 AM

To: Taewook Oh <twoh at fb.com<mailto:twoh at fb.com>>
Cc: via llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at
lists.llvm.org>>
Subject: Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in
FunctionImport.cpp

On Thu, Jul 28, 2016 at 5:18 PM, Taewook Oh <twoh at fb.com<mailto:twoh at
fb.com>> wrote:

Hello Teresa,

Thank you for your reply. I’m trying to create a small repro but find it hard to
nail down because originally it is a big build. This happens with gold linker.

I think I need to see a smaller test case, looking through the code I'm not
sure how we ended up in this situation. See analysis below.

Thanks,

Taewook

From: Teresa Johnson <tejohnson at google.com<mailto:tejohnson at
google.com>>
Date: Thursday, July 28, 2016 at 5:08 PM
To: Taewook Oh <twoh at fb.com<mailto:twoh at fb.com>>
Cc: via llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at
lists.llvm.org>>
Subject: Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in
FunctionImport.cpp

Hi Taewook,

On Thu, Jul 28, 2016 at 4:38 PM, Taewook Oh via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:

Encountered “assert(GS != DefinedGlobals.end())” failure while running ThinLTO.
The assertion statement is in MustPreserveGV lambda function in
llvm::thinLTOInternalizeModule (lib/Transforms/IPO/FunctionImport.cpp).

It seems that the assertion fails because it fails to recover the "original
name" of the global value. ModuleSummaryIndex::getOriginalNameBeforePromote
attempts to get the original name by stripping .llvm.{HASH}, but what I observe
is that ".1" is still appended to the expected original name.

Then where this extra ".1" comes from? It is appended when the global
value is materialized. IRLinker::materialize function calls
IRLinker::linkGlobalValueProto function, and inside that function if DGV is
nullptr or ShouldLink is true then IRLinker::copyGlobalValueProto function is
called to create a global variable in the destination module that corresponds to
SGV. I found that newly created global variable has the extra ".1"
added to the name of the SGV.

When this happens? I don't have a complete understanding but I observed that
the same global value is materialized twice during the single invocation of
IRLinker::run, once with GlobalValueMaterializer and once with
LocalValueMaterializer. First, the global value

Looking at the IRLinker, it appears this second (local) copy should only be
created if there was no copy already linked in from the same source module:

In IRLinker::linkGlobalValueProto, we should find the DGV created by the
GlobalValueMaterializer for the same SGV (first find it by name, then if
ShouldLink==true, we should find it in ValueMap and return that entry, or if
ShouldLink==false, since ForAlias=true we should return null.

Then when linkGlobalValueProto returns, in IRLinker::materialize there is
specific handling for this case:

  // When linking a global for an alias, it will always be linked. However we
  // need to check if it was not already scheduled to satify a reference from a
  // regular global value initializer. We know if it has been schedule if the
  // "New" GlobalValue that is mapped here for the alias is the same
as the one
  // already mapped. If there is an entry in the ValueMap but the value is
  // different, it means that the value already had a definition in the
  // destination module (linkonce for instance), but we need a new definition
  // for the alias ("New" will be different.
  if (ForAlias && ValueMap.lookup(SGV) == New)
    return New;

AFAICT the only way we should be creating a new local copy is if foo() already
had a copy in the dest module. But when we call ThinLTO internalization we are
doing so before any function importing. The gold-plugin should have linked the
module being compiled in the ThinLTO backend into an empty module from
thinLTOBackendTask, so we should not already have a copy of foo() in the dest
module.

I must be missing something...in lieu of a smaller test case, can you trace
through what happens when we call linkGlobalValueProto for the local
materializer, and see why we don't find the copy of SGV already materialized
by the global materializer which should be in the ValueMap?

Thanks,

Teresa

Yes, the IRLinker will append an integer if it encounters a naming conflict.
Normally this would happen in full LTO, but I guess is happening here since we
are linking twice due to the alias.

; Materializable

; Function Attrs: nounwind uwtable

define weak_odr void @foo(%1*) unnamed_addr #7 comdat($comdat1) align 2
personality i32 (...)* @__gxx_personality_v0 {}

(I renamed the function and comdat)

is materialized with GlobalValueMaterializer, (so the IRLinker::materialize
function is called with ForAlias == false), and the materialized value is

; Function Attrs: nounwind uwtable

declare void @foo(%"type1"*) unnamed_addr #2 align 2

Then, later, the same value is materialized again with LocalValueMaterializer
(so ForAlias == true for IRLinker::materialize), and the materialized value is

; Function Attrs: nounwind uwtable

define internal void @foo.1(%"type 0x7efb6ee89d80"*) unnamed_addr #2
comdat($comdat1) align 2 personality i32 (...)* @__gxx_personality_v0 !dbg
!12345 {

  // function definition

}

(here, “type 0x7efb6ee89d80” is not “type1” )

So for me it seems that we need a mechanism to retrieve the original name of the
global value even when it is renamed during the materialization. I submitted the
bug report as well
(https://llvm.org/bugs/show_bug.cgi?id=28760)<https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D28760-29&d=CwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=kOsLCgQzH7N8ptZ7diJD9g&m=XtlNH01OW0mwOi0no2wur-HO6RY5szj-dgWaIcCki-k&s=9pGLGnxeOI3J3lvx9-ayiZJUImAefSzcGGJN5xo9_Kc&e=>.

Interestingly, we are already trying to handle a similar case in that lambda
(see the comment about weak values linked in as a local copy due to an alias).
However, we weren't anticipating the original weak value being linked in as
well, which is causing the rename.

Do you have a small reproducer? Is this with gold? I need to think about the
best way to handle this case. One way of course is to conservatively return true
if we can't find it in the map, but I don't love that since we may miss
other cases that need to be handled.

Thanks,

Teresa

Thanks,

Taewook

_______________________________________________
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<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=CwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=kOsLCgQzH7N8ptZ7diJD9g&m=XtlNH01OW0mwOi0no2wur-HO6RY5szj-dgWaIcCki-k&s=eA2ODuBrwjiWfuW-_sPfCVr1H774iHS5j89ydb7KY2E&e=>

--

Teresa Johnson |

 Software Engineer |

 tejohnson at google.com<mailto:tejohnson at google.com> |

 408-460-2413<tel:408-460-2413>

--

Teresa Johnson |

 Software Engineer |

 tejohnson at google.com<mailto:tejohnson at google.com> |

 408-460-2413<tel:408-460-2413>

--
Teresa Johnson |

 Software Engineer |

 tejohnson at google.com<mailto:tejohnson at google.com> |

 408-460-2413<tel:408-460-2413>

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160729/02121c20/attachment.html>

Teresa Johnson via llvm-dev

2016-Jul-29 22:52 UTC

head link

[llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in FunctionImport.cpp

On Fri, Jul 29, 2016 at 3:40 PM, Taewook Oh <twoh at fb.com> wrote:
> It was r274523. I’m not sure it was the same module. By mistake I
> restarted the build with the previous version without backing backing up
> the build artifacts :(
>
So a couple things were added to gold since then, index-based linkonce/weak
resolution and some more aggressive internalization. I don't think either
of these would have made the original problem go away, so I'm guessing that
the latter provoked the new problem (in combination with pcc's earlier
change to allow metadata attachments on decls that added that verifier
assert). Presumably we started internalizing the promoted global, and
something (presumably a later pass) then dropped the symbol. Let me see if
I can provoke that particular issue with a debug ThinLTO build of SPEC
(recently I have been testing non-debug). In the meantime, can you drop
your debug flag and see if the first problem comes back?

Thanks,
Teresa

>
> Thanks,
>
> Taewook
>
>
>
> *From: *Teresa Johnson <tejohnson at google.com>
> *Date: *Friday, July 29, 2016 at 3:30 PM
>
> *To: *Taewook Oh <twoh at fb.com>
> *Cc: *via llvm-dev <llvm-dev at lists.llvm.org>
> *Subject: *Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end())
> failed in FunctionImport.cpp
>
>
>
>
>
>
>
> On Fri, Jul 29, 2016 at 2:25 PM, Taewook Oh <twoh at fb.com> wrote:
>
> Hello Teresa,
>
>
>
> Thank you for your analysis. One thing to note is that the global
> materializer materializes the value as a function declaration, not a
> function definition. As I pasted on my first email,
>
>
>
> ; Materializable
>
> ; Function Attrs: nounwind uwtable
>
> define weak_odr void @foo(%1*) unnamed_addr #7 comdat($comdat1) align 2
> personality i32 (...)* @__gxx_personality_v0 {}
>
>
>
> is materialized to
>
>
>
> ; Function Attrs: nounwind uwtable
>
> declare void @foo(%"type1"*) unnamed_addr #2 align 2
>
>
>
> Inside IRLinker::linkGlobalValueProto, the materialized value is returned
> from getLinkedToGlobal(SGV) and assigned to DGV. However, as ForAlias is
> true and ShouldLink is false, DGV becomes nullptr later, and NewGV is
> created from copyGlobalValueProto(SGV, ShouldLink) call. Therefore,
> returned value from linkGlobalValueProto is different from the value
> obtained by ValueMap.lookup(SGV) in IRLinker::materialize.
>
>
>
> Ah, sorry, somehow I completely missed the fact that it was brought in as
> a decl when I looked this morning.
>
>
>
>
>
> BTW, I updated LLVM to the latest version in SVN this morning, and the
> assertion failure is gone. Instead, it ends up with broken module found
> errors:
>
>
>
> Global is external, but doesn't have external or weak linkage!
>
> void (%2*, %2*, i32)* @"_foo.llvm.E5F1952C"
>
> function declaration may not have a !dbg attachment
>
> void (%2*, %2*, i32)* @"_foo.llvm.E5F1952C"
>
> LLVM ERROR: Broken module found, compilation aborted!
>
>
>
> This looks like a promoted local from the name. What is the linkage type?
> Is this the verifier run right after the importing pass?
>
>
>
> I'm guessing the first problem went away due to luck, as I can't
think of
> any specific change that would have affected it. Is this from the same
> module?
>
>
>
> Do you know what SVN revision were you at before, even roughly?
>
>
>
> Teresa
>
>
>
> Hope this provides additional clues to fix the issue.
>
>
>
> Thanks,
>
> Taewook
>
>
>
> *From: *Teresa Johnson <tejohnson at google.com>
> *Date: *Friday, July 29, 2016 at 7:05 AM
>
>
> *To: *Taewook Oh <twoh at fb.com>
> *Cc: *via llvm-dev <llvm-dev at lists.llvm.org>
> *Subject: *Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end())
> failed in FunctionImport.cpp
>
>
>
>
>
>
>
> On Thu, Jul 28, 2016 at 5:18 PM, Taewook Oh <twoh at fb.com> wrote:
>
> Hello Teresa,
>
>
>
> Thank you for your reply. I’m trying to create a small repro but find it
> hard to nail down because originally it is a big build. This happens with
> gold linker.
>
>
>
> I think I need to see a smaller test case, looking through the code I'm
> not sure how we ended up in this situation. See analysis below.
>
>
>
> Thanks,
>
> Taewook
>
>
>
> *From: *Teresa Johnson <tejohnson at google.com>
> *Date: *Thursday, July 28, 2016 at 5:08 PM
> *To: *Taewook Oh <twoh at fb.com>
> *Cc: *via llvm-dev <llvm-dev at lists.llvm.org>
> *Subject: *Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end())
> failed in FunctionImport.cpp
>
>
>
> Hi Taewook,
>
>
>
>
>
>
>
> On Thu, Jul 28, 2016 at 4:38 PM, Taewook Oh via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Encountered “assert(GS != DefinedGlobals.end())” failure while running
> ThinLTO. The assertion statement is in MustPreserveGV lambda function in
> llvm::thinLTOInternalizeModule (lib/Transforms/IPO/FunctionImport.cpp).
>
>
>
> It seems that the assertion fails because it fails to recover the
> "original name" of the global value.
> ModuleSummaryIndex::getOriginalNameBeforePromote attempts to get the
> original name by stripping .llvm.{HASH}, but what I observe is that
".1" is
> still appended to the expected original name.
>
>
>
> Then where this extra ".1" comes from? It is appended when the
global
> value is materialized. IRLinker::materialize function calls
> IRLinker::linkGlobalValueProto function, and inside that function if DGV is
> nullptr or ShouldLink is true then IRLinker::copyGlobalValueProto function
> is called to create a global variable in the destination module that
> corresponds to SGV. I found that newly created global variable has the
> extra ".1" added to the name of the SGV.
>
>
>
> When this happens? I don't have a complete understanding but I observed
> that the same global value is materialized twice during the single
> invocation of IRLinker::run, once with GlobalValueMaterializer and once
> with LocalValueMaterializer. First, the global value
>
> Looking at the IRLinker, it appears this second (local) copy should only
> be created if there was no copy already linked in from the same source
> module:
>
>
>
> In IRLinker::linkGlobalValueProto, we should find the DGV created by the
> GlobalValueMaterializer for the same SGV (first find it by name, then if
> ShouldLink==true, we should find it in ValueMap and return that entry, or
> if ShouldLink==false, since ForAlias=true we should return null.
>
>
>
> Then when linkGlobalValueProto returns, in IRLinker::materialize there is
> specific handling for this case:
>
>   // When linking a global for an alias, it will always be linked. However
> we
>   // need to check if it was not already scheduled to satify a reference
> from a
>   // regular global value initializer. We know if it has been schedule if
> the
>   // "New" GlobalValue that is mapped here for the alias is the
same as
> the one
>   // already mapped. If there is an entry in the ValueMap but the value is
>   // different, it means that the value already had a definition in the
>   // destination module (linkonce for instance), but we need a new
> definition
>   // for the alias ("New" will be different.
>   if (ForAlias && ValueMap.lookup(SGV) == New)
>     return New;
>
>
>
>
>
> AFAICT the only way we should be creating a new local copy is if foo()
> already had a copy in the dest module. But when we call ThinLTO
> internalization we are doing so before any function importing. The
> gold-plugin should have linked the module being compiled in the ThinLTO
> backend into an empty module from thinLTOBackendTask, so we should not
> already have a copy of foo() in the dest module.
>
>
>
> I must be missing something...in lieu of a smaller test case, can you
> trace through what happens when we call linkGlobalValueProto for the local
> materializer, and see why we don't find the copy of SGV already
> materialized by the global materializer which should be in the ValueMap?
>
>
>
> Thanks,
>
> Teresa
>
>
>
> Yes, the IRLinker will append an integer if it encounters a naming
> conflict. Normally this would happen in full LTO, but I guess is happening
> here since we are linking twice due to the alias.
>
>
>
>
>
> ; Materializable
>
> ; Function Attrs: nounwind uwtable
>
> define weak_odr void @foo(%1*) unnamed_addr #7 comdat($comdat1) align 2
> personality i32 (...)* @__gxx_personality_v0 {}
>
> (I renamed the function and comdat)
>
>
>
> is materialized with GlobalValueMaterializer, (so the
> IRLinker::materialize function is called with ForAlias == false), and the
> materialized value is
>
>
>
> ; Function Attrs: nounwind uwtable
>
> declare void @foo(%"type1"*) unnamed_addr #2 align 2
>
>
>
> Then, later, the same value is materialized again with
> LocalValueMaterializer (so ForAlias == true for IRLinker::materialize), and
> the materialized value is
>
>
>
> ; Function Attrs: nounwind uwtable
>
> define internal void @foo.1(%"type 0x7efb6ee89d80"*) unnamed_addr
#2
> comdat($comdat1) align 2 personality i32 (...)* @__gxx_personality_v0 !dbg
> !12345 {
>
>   // function definition
>
> }
>
> (here, “type 0x7efb6ee89d80” is not “type1” )
>
>
>
> So for me it seems that we need a mechanism to retrieve the original name
> of the global value even when it is renamed during the materialization. I
> submitted the bug report as well (
> https://llvm.org/bugs/show_bug.cgi?id=28760)
>
<https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D28760-29&d=CwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=kOsLCgQzH7N8ptZ7diJD9g&m=XtlNH01OW0mwOi0no2wur-HO6RY5szj-dgWaIcCki-k&s=9pGLGnxeOI3J3lvx9-ayiZJUImAefSzcGGJN5xo9_Kc&e=>
> .
>
>
>
> Interestingly, we are already trying to handle a similar case in that
> lambda (see the comment about weak values linked in as a local copy due to
> an alias). However, we weren't anticipating the original weak value
being
> linked in as well, which is causing the rename.
>
>
>
> Do you have a small reproducer? Is this with gold? I need to think about
> the best way to handle this case. One way of course is to conservatively
> return true if we can't find it in the map, but I don't love that
since we
> may miss other cases that need to be handled.
>
>
>
> Thanks,
>
> Teresa
>
>
>
> Thanks,
>
> Taewook
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=CwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=kOsLCgQzH7N8ptZ7diJD9g&m=XtlNH01OW0mwOi0no2wur-HO6RY5szj-dgWaIcCki-k&s=eA2ODuBrwjiWfuW-_sPfCVr1H774iHS5j89ydb7KY2E&e=>
>
>
>
>
>
> --
>
> Teresa Johnson |
>
>  Software Engineer |
>
>  tejohnson at google.com |
>
>  408-460-2413
>
>
>
>
>
>
>
> --
>
> Teresa Johnson |
>
>  Software Engineer |
>
>  tejohnson at google.com |
>
>  408-460-2413
>
>
>
>
>
>
>
> --
>
> Teresa Johnson |
>
>  Software Engineer |
>
>  tejohnson at google.com |
>
>  408-460-2413
>
>
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160729/899d2db4/attachment-0001.html>

Taewook Oh via llvm-dev

2016-Jul-30 06:37 UTC

head link

[llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in FunctionImport.cpp

Yes, if I drop the debug flag then the original problem (assertion failure)
comes back.

Thanks,
Taewook

From: Teresa Johnson <tejohnson at google.com>
Date: Friday, July 29, 2016 at 3:52 PM
To: Taewook Oh <twoh at fb.com>
Cc: via llvm-dev <llvm-dev at lists.llvm.org>
Subject: Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in
FunctionImport.cpp

On Fri, Jul 29, 2016 at 3:40 PM, Taewook Oh <twoh at fb.com<mailto:twoh at
fb.com>> wrote:
It was r274523. I’m not sure it was the same module. By mistake I restarted the
build with the previous version without backing backing up the build artifacts
:(

So a couple things were added to gold since then, index-based linkonce/weak
resolution and some more aggressive internalization. I don't think either of
these would have made the original problem go away, so I'm guessing that the
latter provoked the new problem (in combination with pcc's earlier change to
allow metadata attachments on decls that added that verifier assert). Presumably
we started internalizing the promoted global, and something (presumably a later
pass) then dropped the symbol. Let me see if I can provoke that particular issue
with a debug ThinLTO build of SPEC (recently I have been testing non-debug). In
the meantime, can you drop your debug flag and see if the first problem comes
back?

Thanks,
Teresa

Thanks,
Taewook

From: Teresa Johnson <tejohnson at google.com<mailto:tejohnson at
google.com>>
Date: Friday, July 29, 2016 at 3:30 PM

To: Taewook Oh <twoh at fb.com<mailto:twoh at fb.com>>
Cc: via llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at
lists.llvm.org>>
Subject: Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in
FunctionImport.cpp

On Fri, Jul 29, 2016 at 2:25 PM, Taewook Oh <twoh at fb.com<mailto:twoh at
fb.com>> wrote:

Hello Teresa,

Thank you for your analysis. One thing to note is that the global materializer
materializes the value as a function declaration, not a function definition. As
I pasted on my first email,

; Materializable

; Function Attrs: nounwind uwtable

define weak_odr void @foo(%1*) unnamed_addr #7 comdat($comdat1) align 2
personality i32 (...)* @__gxx_personality_v0 {}

is materialized to

; Function Attrs: nounwind uwtable

declare void @foo(%"type1"*) unnamed_addr #2 align 2

Inside IRLinker::linkGlobalValueProto, the materialized value is returned from
getLinkedToGlobal(SGV) and assigned to DGV. However, as ForAlias is true and
ShouldLink is false, DGV becomes nullptr later, and NewGV is created from
copyGlobalValueProto(SGV, ShouldLink) call. Therefore, returned value from
linkGlobalValueProto is different from the value obtained by
ValueMap.lookup(SGV) in IRLinker::materialize.

Ah, sorry, somehow I completely missed the fact that it was brought in as a decl
when I looked this morning.

BTW, I updated LLVM to the latest version in SVN this morning, and the assertion
failure is gone. Instead, it ends up with broken module found errors:

Global is external, but doesn't have external or weak linkage!

void (%2*, %2*, i32)* @"_foo.llvm.E5F1952C"

function declaration may not have a !dbg attachment

void (%2*, %2*, i32)* @"_foo.llvm.E5F1952C"

LLVM ERROR: Broken module found, compilation aborted!

This looks like a promoted local from the name. What is the linkage type? Is
this the verifier run right after the importing pass?

I'm guessing the first problem went away due to luck, as I can't think
of any specific change that would have affected it. Is this from the same
module?

Do you know what SVN revision were you at before, even roughly?

Teresa

Hope this provides additional clues to fix the issue.

Thanks,

Taewook

From: Teresa Johnson <tejohnson at google.com<mailto:tejohnson at
google.com>>
Date: Friday, July 29, 2016 at 7:05 AM

To: Taewook Oh <twoh at fb.com<mailto:twoh at fb.com>>
Cc: via llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at
lists.llvm.org>>
Subject: Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in
FunctionImport.cpp

On Thu, Jul 28, 2016 at 5:18 PM, Taewook Oh <twoh at fb.com<mailto:twoh at
fb.com>> wrote:

Hello Teresa,

Thank you for your reply. I’m trying to create a small repro but find it hard to
nail down because originally it is a big build. This happens with gold linker.

I think I need to see a smaller test case, looking through the code I'm not
sure how we ended up in this situation. See analysis below.

Thanks,

Taewook

From: Teresa Johnson <tejohnson at google.com<mailto:tejohnson at
google.com>>
Date: Thursday, July 28, 2016 at 5:08 PM
To: Taewook Oh <twoh at fb.com<mailto:twoh at fb.com>>
Cc: via llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at
lists.llvm.org>>
Subject: Re: [llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in
FunctionImport.cpp

Hi Taewook,

On Thu, Jul 28, 2016 at 4:38 PM, Taewook Oh via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:

Encountered “assert(GS != DefinedGlobals.end())” failure while running ThinLTO.
The assertion statement is in MustPreserveGV lambda function in
llvm::thinLTOInternalizeModule (lib/Transforms/IPO/FunctionImport.cpp).

It seems that the assertion fails because it fails to recover the "original
name" of the global value. ModuleSummaryIndex::getOriginalNameBeforePromote
attempts to get the original name by stripping .llvm.{HASH}, but what I observe
is that ".1" is still appended to the expected original name.

Then where this extra ".1" comes from? It is appended when the global
value is materialized. IRLinker::materialize function calls
IRLinker::linkGlobalValueProto function, and inside that function if DGV is
nullptr or ShouldLink is true then IRLinker::copyGlobalValueProto function is
called to create a global variable in the destination module that corresponds to
SGV. I found that newly created global variable has the extra ".1"
added to the name of the SGV.

When this happens? I don't have a complete understanding but I observed that
the same global value is materialized twice during the single invocation of
IRLinker::run, once with GlobalValueMaterializer and once with
LocalValueMaterializer. First, the global value

Looking at the IRLinker, it appears this second (local) copy should only be
created if there was no copy already linked in from the same source module:

In IRLinker::linkGlobalValueProto, we should find the DGV created by the
GlobalValueMaterializer for the same SGV (first find it by name, then if
ShouldLink==true, we should find it in ValueMap and return that entry, or if
ShouldLink==false, since ForAlias=true we should return null.

Then when linkGlobalValueProto returns, in IRLinker::materialize there is
specific handling for this case:

  // When linking a global for an alias, it will always be linked. However we
  // need to check if it was not already scheduled to satify a reference from a
  // regular global value initializer. We know if it has been schedule if the
  // "New" GlobalValue that is mapped here for the alias is the same
as the one
  // already mapped. If there is an entry in the ValueMap but the value is
  // different, it means that the value already had a definition in the
  // destination module (linkonce for instance), but we need a new definition
  // for the alias ("New" will be different.
  if (ForAlias && ValueMap.lookup(SGV) == New)
    return New;

AFAICT the only way we should be creating a new local copy is if foo() already
had a copy in the dest module. But when we call ThinLTO internalization we are
doing so before any function importing. The gold-plugin should have linked the
module being compiled in the ThinLTO backend into an empty module from
thinLTOBackendTask, so we should not already have a copy of foo() in the dest
module.

I must be missing something...in lieu of a smaller test case, can you trace
through what happens when we call linkGlobalValueProto for the local
materializer, and see why we don't find the copy of SGV already materialized
by the global materializer which should be in the ValueMap?

Thanks,

Teresa

Yes, the IRLinker will append an integer if it encounters a naming conflict.
Normally this would happen in full LTO, but I guess is happening here since we
are linking twice due to the alias.

; Materializable

; Function Attrs: nounwind uwtable

define weak_odr void @foo(%1*) unnamed_addr #7 comdat($comdat1) align 2
personality i32 (...)* @__gxx_personality_v0 {}

(I renamed the function and comdat)

is materialized with GlobalValueMaterializer, (so the IRLinker::materialize
function is called with ForAlias == false), and the materialized value is

; Function Attrs: nounwind uwtable

declare void @foo(%"type1"*) unnamed_addr #2 align 2

Then, later, the same value is materialized again with LocalValueMaterializer
(so ForAlias == true for IRLinker::materialize), and the materialized value is

; Function Attrs: nounwind uwtable

define internal void @foo.1(%"type 0x7efb6ee89d80"*) unnamed_addr #2
comdat($comdat1) align 2 personality i32 (...)* @__gxx_personality_v0 !dbg
!12345 {

  // function definition

}

(here, “type 0x7efb6ee89d80” is not “type1” )

So for me it seems that we need a mechanism to retrieve the original name of the
global value even when it is renamed during the materialization. I submitted the
bug report as well
(https://llvm.org/bugs/show_bug.cgi?id=28760)<https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D28760-29&d=CwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=kOsLCgQzH7N8ptZ7diJD9g&m=XtlNH01OW0mwOi0no2wur-HO6RY5szj-dgWaIcCki-k&s=9pGLGnxeOI3J3lvx9-ayiZJUImAefSzcGGJN5xo9_Kc&e=>.

Interestingly, we are already trying to handle a similar case in that lambda
(see the comment about weak values linked in as a local copy due to an alias).
However, we weren't anticipating the original weak value being linked in as
well, which is causing the rename.

Do you have a small reproducer? Is this with gold? I need to think about the
best way to handle this case. One way of course is to conservatively return true
if we can't find it in the map, but I don't love that since we may miss
other cases that need to be handled.

Thanks,

Teresa

Thanks,

Taewook

_______________________________________________
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<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=CwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=kOsLCgQzH7N8ptZ7diJD9g&m=XtlNH01OW0mwOi0no2wur-HO6RY5szj-dgWaIcCki-k&s=eA2ODuBrwjiWfuW-_sPfCVr1H774iHS5j89ydb7KY2E&e=>

--

Teresa Johnson |

 Software Engineer |

 tejohnson at google.com<mailto:tejohnson at google.com> |

 408-460-2413<tel:408-460-2413>

--

Teresa Johnson |

 Software Engineer |

 tejohnson at google.com<mailto:tejohnson at google.com> |

 408-460-2413<tel:408-460-2413>

--
Teresa Johnson |

 Software Engineer |

 tejohnson at google.com<mailto:tejohnson at google.com> |

 408-460-2413<tel:408-460-2413>

--
Teresa Johnson |

 Software Engineer |

 tejohnson at google.com<mailto:tejohnson at google.com> |

 408-460-2413

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160730/f69081f7/attachment-0001.html>

Possibly Parallel Threads

Search for more apparently analagous threads

llvm dev - Jul 2016 - [ThinLTO] assert(GS != DefinedGlobals.end()) failed in FunctionImport.cpp

[llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in FunctionImport.cpp

[llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in FunctionImport.cpp

[llvm-dev] [ThinLTO] assert(GS != DefinedGlobals.end()) failed in FunctionImport.cpp

Possibly Parallel Threads