Fix mapping of unmaterialized global values during metadata linking

Summary:
The patch to move metadata linking after global value linking didn't
correctly map unmaterialized global values to null as desired. They
were in fact mapped to the source copy. It largely worked by accident
since most module linker clients destroyed the source module which
caused the source GVs to be replaced by null, but caused a failure with
LTO linking on Windows:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20151109/312869.html

The problem is that a null return value from materializeValueFor is
handled by mapping the value to self. This is the desired behavior when
materializeValueFor is passed a non-GlobalValue. The problem is how to
distinguish that case from the case where we really do want to map to
null.

This patch addresses this by passing in a new flag to the value mapper
indicating that unmapped global values should be mapped to null. Other
Value types are handled as before.

Note that the documented behavior of asserting on unmapped values when
the flag RF_IgnoreMissingValues isn't set is currently disabled with
FIXME notes due to bootstrap failures. I modified these disabled asserts
so when they are eventually enabled again it won't assert for the
unmapped values when the new RF_NullMapMissingGlobalValues flag is set.

I also considered using a callback into the value materializer, but a
flag seemed cleaner given that there are already existing flags.
I also considered modifying materializeValueFor to return the input
value when we want to map to source and then treat a null return
to mean map to null. However, there are other value materializer
subclasses that implement materializeValueFor, and they would all need
to be audited and the return values possibly changed, which seemed
error-prone.

Reviewers: dexonsmith, joker.eph

Subscribers: pcc, llvm-commits

Differential Revision: http://reviews.llvm.org/D14682

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@253170 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/Transforms/Utils/ValueMapper.h b/include/llvm/Transforms/Utils/ValueMapper.h
index 6f4cd24c849ffa2ae9600851fd038387bde74971..17ce4f4ab73dda73284d7ecf19bc86da5f26ee4d 100644 (file)
--- a/include/llvm/Transforms/Utils/ValueMapper.h
+++ b/include/llvm/Transforms/Utils/ValueMapper.h
@@ -69,6 +69,10 @@ namespace llvm {
     /// Instruct the remapper to move distinct metadata instead of duplicating
     /// it when there are module-level changes.
     RF_MoveDistinctMDs = 4,
     /// Instruct the remapper to move distinct metadata instead of duplicating
     /// it when there are module-level changes.
     RF_MoveDistinctMDs = 4,

+
+    /// Any global values not in value map are mapped to null instead of
+    /// mapping to self. Illegal if RF_IgnoreMissingEntries is also set.
+    RF_NullMapMissingGlobalValues = 8,

   };
 
   static inline RemapFlags operator|(RemapFlags LHS, RemapFlags RHS) {
   };
 
   static inline RemapFlags operator|(RemapFlags LHS, RemapFlags RHS) {

diff --git a/lib/Linker/LinkModules.cpp b/lib/Linker/LinkModules.cpp
index ee41d25ed706118849346b268a8f1c0beace21ca..5419c22fbf7b69dea916910eccf7cb98eb8bc4fb 100644 (file)
--- a/lib/Linker/LinkModules.cpp
+++ b/lib/Linker/LinkModules.cpp
@@ -1628,8 +1628,9 @@ void ModuleLinker::linkNamedMDNodes() {
     NamedMDNode *DestNMD = DstM->getOrInsertNamedMetadata(NMD.getName());
     // Add Src elements into Dest node.
     for (const MDNode *op : NMD.operands())
     NamedMDNode *DestNMD = DstM->getOrInsertNamedMetadata(NMD.getName());
     // Add Src elements into Dest node.
     for (const MDNode *op : NMD.operands())

-      DestNMD->addOperand(MapMetadata(op, ValueMap, RF_MoveDistinctMDs,
-                                      &TypeMap, &ValMaterializer));
+      DestNMD->addOperand(MapMetadata(
+          op, ValueMap, RF_MoveDistinctMDs | RF_NullMapMissingGlobalValues,
+          &TypeMap, &ValMaterializer));

   }
 }
 
   }
 }
 

diff --git a/lib/Transforms/Utils/ValueMapper.cpp b/lib/Transforms/Utils/ValueMapper.cpp
index 414d4eaa68dedeb499c25e76336a244e3d2fab3a..c8d13385b1d491456851ec2e9e94c3b8232c0611 100644 (file)
--- a/lib/Transforms/Utils/ValueMapper.cpp
+++ b/lib/Transforms/Utils/ValueMapper.cpp
@@ -42,9 +42,16 @@ Value *llvm::MapValue(const Value *V, ValueToValueMapTy &VM, RemapFlags Flags,
 
   // Global values do not need to be seeded into the VM if they
   // are using the identity mapping.
 
   // Global values do not need to be seeded into the VM if they
   // are using the identity mapping.

-  if (isa<GlobalValue>(V))
+  if (isa<GlobalValue>(V)) {
+    if (Flags & RF_NullMapMissingGlobalValues) {
+      assert(!(Flags & RF_IgnoreMissingEntries) &&
+             "Illegal to specify both RF_NullMapMissingGlobalValues and "
+             "RF_IgnoreMissingEntries");
+      return nullptr;
+    }

     return VM[V] = const_cast<Value*>(V);
     return VM[V] = const_cast<Value*>(V);

-  
+  }
+

   if (const InlineAsm *IA = dyn_cast<InlineAsm>(V)) {
     // Inline asm may need *type* remapping.
     FunctionType *NewTy = IA->getFunctionType();
   if (const InlineAsm *IA = dyn_cast<InlineAsm>(V)) {
     // Inline asm may need *type* remapping.
     FunctionType *NewTy = IA->getFunctionType();


@@ -74,7 +81,8 @@ Value *llvm::MapValue(const Value *V, ValueToValueMapTy &VM, RemapFlags Flags,
     // correct.  For now, just match behaviour from before the metadata/value
     // split.
     //
     // correct.  For now, just match behaviour from before the metadata/value
     // split.
     //

-    //    assert(MappedMD && "Referenced metadata value not in value map");
+    //    assert((MappedMD || (Flags & RF_NullMapMissingGlobalValues)) &&
+    //           "Referenced metadata value not in value map");

     return VM[V] = MetadataAsValue::get(V->getContext(), MappedMD);
   }
 
     return VM[V] = MetadataAsValue::get(V->getContext(), MappedMD);
   }
 


@@ -184,7 +192,8 @@ static Metadata *mapMetadataOp(Metadata *Op,
   // correct.  For now, just match behaviour from before the metadata/value
   // split.
   //
   // correct.  For now, just match behaviour from before the metadata/value
   // split.
   //

-  //    llvm_unreachable("Referenced metadata not in value map!");
+  //    assert((Flags & RF_NullMapMissingGlobalValues) &&
+  //           "Referenced metadata not in value map!");

   return nullptr;
 }
 
   return nullptr;
 }
 


@@ -305,7 +314,8 @@ static Metadata *MapMetadataImpl(const Metadata *MD,
     // correct.  For now, just match behaviour from before the metadata/value
     // split.
     //
     // correct.  For now, just match behaviour from before the metadata/value
     // split.
     //

-    //    assert(MappedV && "Referenced metadata not in value map!");
+    //    assert((MappedV || (Flags & RF_NullMapMissingGlobalValues)) &&
+    //           "Referenced metadata not in value map!");

     if (MappedV)
       return mapToMetadata(VM, MD, ValueAsMetadata::get(MappedV));
     return nullptr;
     if (MappedV)
       return mapToMetadata(VM, MD, ValueAsMetadata::get(MappedV));
     return nullptr;