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

[samsonov]

Summary

One more attempt to fix UBSan reports: make sure DenseMapInfo::getEmptyKey()
and DenseMapInfo::getTombstoneKey() doesn't do any upcasts/downcasts to/from Value*.

[chandlerc]

include/llvm/IR/ValueHandle.h

239–248

I think you need to use DenseMapInfo<Value *> here?

[samsonov]

Address Chandler's comment.

include/llvm/IR/ValueHandle.h

239–248

Sure. Fixed.

[chandlerc]

include/llvm/IR/ValueHandle.h

192–203

I would define helpers here for getRawValPtr and setRawValPtr which do the indirection between NDEBUG and debug builds. Then use them below (see comments).

240–275

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 *>.

[samsonov]

Address comments.

include/llvm/IR/ValueHandle.h

192–203

Done

240–275

Done. However, what if some subclass MyValue of Value provides its own DenseMapInfo<MyValue*>::isEqual or DenseMapInfo<MyValue*>::getHashValue ?

[chandlerc]

LGTM as well, thanks!!! Sorry for the circles this round around in!

include/llvm/IR/ValueHandle.h

240–275

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*>.

[samsonov]

Thanks, submitting this now!

include/llvm/IR/ValueHandle.h

240–275

Right, I agree with that, just wanted to double-check my understanding.

include/llvm/IR/ValueHandle.h

 template <typename ValueTy>
 class AssertingVH
 #ifndef NDEBUG
   : public ValueHandleBase
 #endif
   {
   friend struct DenseMapInfo<AssertingVH<ValueTy> >;
 
 #ifndef NDEBUG
   ValueTy *getValPtr() const {
     return static_cast<ValueTy*>(ValueHandleBase::getValPtr());
   }
   void setValPtr(ValueTy *P) {
     ValueHandleBase::operator=(GetAsValue(P));
   }
 #else
-  ValueTy *ThePtr;
-  ValueTy *getValPtr() const { return ThePtr; }
-  void setValPtr(ValueTy *P) { ThePtr = P; }
+  Value *ThePtr;
+  ValueTy *getValPtr() const { return static_cast<ValueTy *>(ThePtr); }
+  void setValPtr(ValueTy *P) { ThePtr = GetAsValue(P); }
 #endif
 
-  // Convert a ValueTy*, which may be const, to the type the base
-  // class expects.
+  // Convert a ValueTy*, which may be const, to the raw Value*.
   static Value *GetAsValue(Value *V) { return V; }
   static Value *GetAsValue(const Value *V) { return const_cast<Value*>(V); }
 
 public:
 #ifndef NDEBUG
   AssertingVH() : ValueHandleBase(Assert) {}
   AssertingVH(ValueTy *P) : ValueHandleBase(Assert, GetAsValue(P)) {}
   AssertingVH(const AssertingVH &RHS) : ValueHandleBase(Assert, RHS) {}
 #else
   AssertingVH() : ThePtr(nullptr) {}
-  AssertingVH(ValueTy *P) : ThePtr(P) {}
+  AssertingVH(ValueTy *P) : ThePtr(GetAsValue(P)) {}
 #endif
 
   operator ValueTy*() const {
     return getValPtr();
   }
 
   ValueTy *operator=(ValueTy *RHS) {
     setValPtr(RHS);
     return getValPtr();
   }
   ValueTy *operator=(const AssertingVH<ValueTy> &RHS) {
     setValPtr(RHS.getValPtr());
     return getValPtr();
   }
 
   ValueTy *operator->() const { return getValPtr(); }
   ValueTy &operator*() const { return *getValPtr(); }
 };
 
 // Specialize DenseMapInfo to allow AssertingVH to participate in DenseMap.
 template<typename T>
 struct DenseMapInfo<AssertingVH<T> > {
   typedef DenseMapInfo<T*> PointerInfo;
   static inline AssertingVH<T> getEmptyKey() {
-    return AssertingVH<T>(PointerInfo::getEmptyKey());
+    AssertingVH<T> Res;
+    Value *V = const_cast<Value *>(
+        reinterpret_cast<const Value *>(PointerInfo::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 = const_cast<Value *>(
+        reinterpret_cast<const Value *>(PointerInfo::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);
   }
 #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
 };
 
 template <typename T>
 struct isPodLike<AssertingVH<T> > {
 #ifdef NDEBUG
   static const bool value = true;
 #else
   static const bool value = false;

include/llvm/IR/ValueMap.h

class ValueMapCallbackVH : public CallbackVH {
   typedef typename std::remove_pointer<KeyT>::type KeySansPointerT;
 
   ValueMapT *Map;
 
   ValueMapCallbackVH(KeyT Key, ValueMapT *Map)
       : CallbackVH(const_cast<Value*>(static_cast<const Value*>(Key))),
         Map(Map) {}
 
+  // Private constructor used to create empty/tombstone DenseMap keys.
+  ValueMapCallbackVH(Value *V) : CallbackVH(V), Map(nullptr) {}
+
 public:
   KeyT Unwrap() const { return cast_or_null<KeySansPointerT>(getValPtr()); }
 
   void deleted() override {
     // Make a copy that won't get changed even when *this is destroyed.
     ValueMapCallbackVH Copy(*this);
     typename Config::mutex_type *M = Config::getMutex(Copy.Map->Data);
     unique_lock<typename Config::mutex_type> Guard;

 };
 
 template<typename KeyT, typename ValueT, typename Config>
 struct DenseMapInfo<ValueMapCallbackVH<KeyT, ValueT, Config> > {
   typedef ValueMapCallbackVH<KeyT, ValueT, Config> VH;
   typedef DenseMapInfo<KeyT> PointerInfo;
 
   static inline VH getEmptyKey() {
-    return VH(PointerInfo::getEmptyKey(), nullptr);
+    return VH(const_cast<Value *>(
+        reinterpret_cast<const Value *>(PointerInfo::getEmptyKey())));
   }
   static inline VH getTombstoneKey() {
-    return VH(PointerInfo::getTombstoneKey(), nullptr);
+    return VH(const_cast<Value *>(
+        reinterpret_cast<const Value *>(PointerInfo::getTombstoneKey())));
   }
   static unsigned getHashValue(const VH &Val) {
     return PointerInfo::getHashValue(Val.Unwrap());
   }
   static unsigned getHashValue(const KeyT &Val) {
     return PointerInfo::getHashValue(Val);
   }
   static bool isEqual(const VH &LHS, const VH &RHS) {