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

Fri Jul 29 07:05:50 PDT 2016

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160729/a23fc3a2/attachment.html>