[PATCH] D14682: Fix mapping of unmaterialized global values during metadata linking

[Teresa Johnson via llvm-commits]

tejohnson created this revision.
tejohnson added reviewers: dexonsmith, joker.eph.
tejohnson added subscribers: llvm-commits, pcc.

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.

http://reviews.llvm.org/D14682

Files:
  include/llvm/Transforms/Utils/ValueMapper.h
  lib/Linker/LinkModules.cpp
  lib/Transforms/Utils/ValueMapper.cpp

Index: lib/Transforms/Utils/ValueMapper.cpp
===================================================================
--- lib/Transforms/Utils/ValueMapper.cpp
+++ lib/Transforms/Utils/ValueMapper.cpp
@@ -42,9 +42,14 @@
 
   // 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));
+      return nullptr;
+    }
     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();
@@ -74,7 +79,8 @@
     // 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);
   }
 
@@ -184,7 +190,8 @@
   // 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;
 }
 
@@ -305,7 +312,8 @@
     // 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;
Index: lib/Linker/LinkModules.cpp
===================================================================
--- lib/Linker/LinkModules.cpp
+++ lib/Linker/LinkModules.cpp
@@ -1628,8 +1628,9 @@
     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));
   }
 }
 
Index: include/llvm/Transforms/Utils/ValueMapper.h
===================================================================
--- include/llvm/Transforms/Utils/ValueMapper.h
+++ include/llvm/Transforms/Utils/ValueMapper.h
@@ -69,6 +69,11 @@
     /// Instruct the remapper to move distinct metadata instead of duplicating
     /// it when there are module-level changes.
     RF_MoveDistinctMDs = 4,
+
+    /// RF_NullMapMissingGlobalValues - 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) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D14682.40215.patch
Type: text/x-patch
Size: 3244 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151114/cfc5f358/attachment.bin>