[llvm-link] Fix crash when materializing appending global

[sdmitriev]

Summary

This patch fixes llvm-link crash when materializing global variable
with appending linkage and initializer that depends on another
global with appending linkage.

[tra]

llvm/lib/Transforms/Utils/ValueMapper.cpp

822–824

Does the crash happen because mapAppendingVariable attempts to iterate over an array that may change by the loop?
If that's the case, the fix (making a temp copy with relevant items) should probably be done there.

[sdmitriev]

llvm/lib/Transforms/Utils/ValueMapper.cpp

822–824

For the IR from test mapAppendingVariable call causes a new item to be added to the AppendingInits vector, but this item gets incorrectly cleaned up from it by the resize call which follows mapAppendingVariable. As a result we are getting PrefixSize == -1 while processing the second appending global which leads to a crash.

[tra]

llvm/lib/Transforms/Utils/ValueMapper.cpp

822–824

I see. I'd rename Inits -> NewInits or UnmappedInits to make it more clear that we only want to process new entries.
Perhaps PrefixSize could also use a more descriptive name, too. NumMappedInits ?

825

What's the typical number of inits do you expect to see in practice?
Presumably, it's smaller than that of AppendingInits itself, which is 16.

[sdmitriev]

Addressed review comments.

[sdmitriev]

llvm/lib/Transforms/Utils/ValueMapper.cpp

825

I have renamed Inits to NewInits in the updated patch and reduced its size to 8.

llvm/lib/Transforms/Utils/ValueMapper.cpp

while (!Worklist.empty()) {
     CurrentMCID = E.MCID;
     switch (E.Kind) {
     case WorklistEntry::MapGlobalInit:
       E.Data.GVInit.GV->setInitializer(mapConstant(E.Data.GVInit.Init));
       remapGlobalObjectMetadata(*E.Data.GVInit.GV);
       break;
     case WorklistEntry::MapAppendingVar: {
       unsigned PrefixSize = AppendingInits.size() - E.AppendingGVNumNewMembers;
+      // mapAppendingVariable call can change AppendingInits if initalizer for
+      // the variable depends on another appending global, because of that inits
+      // need to be extracted and updated before the call.
+      SmallVector<Constant *, 8> NewInits(
+          drop_begin(AppendingInits, PrefixSize));
+      AppendingInits.resize(PrefixSize);
       mapAppendingVariable(*E.Data.AppendingGV.GV,
                            E.Data.AppendingGV.InitPrefix,
-                           E.AppendingGVIsOldCtorDtor,
-                           makeArrayRef(AppendingInits).slice(PrefixSize));
-      AppendingInits.resize(PrefixSize);
+                           E.AppendingGVIsOldCtorDtor, makeArrayRef(NewInits));
       break;
     }
     case WorklistEntry::MapGlobalIndirectSymbol:
       E.Data.GlobalIndirectSymbol.GIS->setIndirectSymbol(
           mapConstant(E.Data.GlobalIndirectSymbol.Target));
       break;
     case WorklistEntry::RemapFunction:
       remapFunction(*E.Data.RemapF);

llvm/test/Linker/appending-global-crash.ll

+; RUN: llvm-link %s -S -o - | FileCheck %s
+
+; Check that llvm-link does not crash when materializing appending global with
+; initializer depending on another appending global.
+
+; CHECK-DAG: @use = appending global [1 x i8*] [i8* bitcast ([1 x i8*]* @var to i8*)]
+; CHECK-DAG: @var = appending global [1 x i8*] undef
+
+@use = appending global [1 x i8*] [i8* bitcast ([1 x i8*]* @var to i8*)]
+@var = appending global [1 x i8*] undef