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

Teresa Johnson via llvm-dev llvm-dev at lists.llvm.org
Mon Aug 1 06:57:32 PDT 2016 

On Sat, Jul 30, 2016 at 6:45 AM, Teresa Johnson <tejohnson at google.com>
wrote:

> Ok, good to know.
>
> Any luck on a smaller test case for either of the problems?
>

> I tried building all the C/C++ SPEC cpu2006 benchmarks with -g -flto=thin
> at head and didn't get the failure. Let me try to look into how the debug
> metadata is normally dropped from the imported decl. In the meantime, could
> you find out the answers to the questions I had (see below) about the
> linkage type of the _foo symbol and when the verifier was being run when it
> hit the failures? Also, was _foo imported into the module?
>
> Also, I am going to try to see if I can construct a test case to trigger
> the original problem. But do you want to try to make forward progress by
> changing your local version of LLVM so that instead of asserting in
> MustPreserveGV, we instead conservatively return true if it isn't found in
> the map?
>

I have a small test case to repro the original problem.

I think that the conservative fix I describe above is probably the right
approach - we could also try stripping off any .\d+ suffix and querying the
module index with that. It works but I am a little wary as the variable
could originally be named with a .\d+ suffix as well and we could
inadvertently use the index for a different variable. Although they should
both have index entries, but I'd rather do something conservatively correct
rather than cause a subtle bug due to using the wrong index entry.

Will update the bugzilla entry and send a patch.

I may still need help getting a test case for the second problem (the debug
metadata on the decl), although I might not be able to look at that for a
couple days.

Teresa


> Teresa
>
> On Fri, Jul 29, 2016 at 11:37 PM, Taewook Oh <twoh at fb.com> wrote:
>
>> 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> 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
>>
>>
>>
>
>
>
> --
> 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/20160801/c6762071/attachment-0001.html>

More information about the llvm-dev mailing list