[PATCH] Fix UBSan error reports in ValueMapCallbackVH and AssertingVH<T> empty/tombstone keys generation.

[Alexey Samsonov]

Thanks, submitting this now!


================
Comment at: include/llvm/IR/ValueHandle.h:239-272
@@ -239,23 +238,36 @@
 struct DenseMapInfo<AssertingVH<T> > {
-  typedef DenseMapInfo<T*> PointerInfo;
   static inline AssertingVH<T> getEmptyKey() {
-    return AssertingVH<T>(PointerInfo::getEmptyKey());
+    AssertingVH<T> Res;
+    Value *V = DenseMapInfo<Value *>::getEmptyKey();
+#ifndef NDEBUG
+    Res.ValueHandleBase::operator=(V);
+#else
+    Res.ThePtr = V;
+#endif
+    return Res;
   }
-  static inline T* getTombstoneKey() {
-    return AssertingVH<T>(PointerInfo::getTombstoneKey());
+  static inline AssertingVH<T> getTombstoneKey() {
+    AssertingVH<T> Res;
+    Value *V = DenseMapInfo<Value *>::getTombstoneKey();
+#ifndef NDEBUG
+    Res.ValueHandleBase::operator=(V);
+#else
+    Res.ThePtr = V;
+#endif
+    return Res;
   }
   static unsigned getHashValue(const AssertingVH<T> &Val) {
-    return PointerInfo::getHashValue(Val);
+    return DenseMapInfo<T *>::getHashValue(Val);
   }
 #ifndef NDEBUG
   static bool isEqual(const AssertingVH<T> &LHS, const AssertingVH<T> &RHS) {
     // Avoid downcasting AssertingVH<T> to T*, as empty/tombstone keys may not
     // be properly aligned pointers to T*.
     return LHS.ValueHandleBase::getValPtr() == RHS.ValueHandleBase::getValPtr();
   }
 #else
   static bool isEqual(const AssertingVH<T> &LHS, const AssertingVH<T> &RHS) {
     return LHS == RHS;
   }
 #endif
 };
----------------
chandlerc wrote:
> samsonov wrote:
> > chandlerc wrote:
> > > All of these NDEBUG vs. not can go away by using helpers defined once in the class above to get the raw Value* and set the raw Value*.
> > > 
> > > I also think you want to have both isEqual and getHashValue use the raw Value* and delegate to DenseMapInfo<Value *>.
> > Done. However, what if some subclass MyValue of Value provides its own DenseMapInfo<MyValue*>::isEqual or DenseMapInfo<MyValue*>::getHashValue ?
> As Duncan said, I think that would be weird. I might go further and say I think using such an AssertingVH<MyValue> would be a bug. We can't both test for equality using their delegated routine *and* not use their type's pointers. I think it is reasonable to insist for simplicity that Value subclasses that want DenseMaps of AssertingVHs must be happy with DenseMapInfo<Value*> or generally DenseMapInfo<T*>.
Right, I agree with that, just wanted to double-check my understanding.

http://reviews.llvm.org/D6903

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/