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 ↗

(On Diff #17941)

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

[samsonov]

Address Chandler's comment.

include/llvm/IR/ValueHandle.h

239–248 ↗

(On Diff #17941)

Sure. Fixed.

[chandlerc]

include/llvm/IR/ValueHandle.h

192–203 ↗

(On Diff #17945)

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

239–272 ↗

(On Diff #17945)

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 ↗

(On Diff #17945)

Done

239–272 ↗

(On Diff #17945)

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

239–272 ↗

(On Diff #17945)

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

239–272 ↗

(On Diff #17945)

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

llvm/trunk/include/llvm/IR/ValueHandle.h

 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));
-  }
+  Value *getRawValPtr() const { return ValueHandleBase::getValPtr(); }
+  void setRawValPtr(Value *P) { ValueHandleBase::operator=(P); }
 #else
-  ValueTy *ThePtr;
-  ValueTy *getValPtr() const { return ThePtr; }
-  void setValPtr(ValueTy *P) { ThePtr = P; }
+  Value *ThePtr;
+  Value *getRawValPtr() const { return ThePtr; }
+  void setRawValPtr(Value *P) { ThePtr = 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); }
 
+  ValueTy *getValPtr() const { return static_cast<ValueTy *>(getRawValPtr()); }
+  void setValPtr(ValueTy *P) { setRawValPtr(GetAsValue(P)); }
+
 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());
-  }
-  static inline T* getTombstoneKey() {
-    return AssertingVH<T>(PointerInfo::getTombstoneKey());
+    AssertingVH<T> Res;
+    Res.setRawValPtr(DenseMapInfo<Value *>::getEmptyKey());
+    return Res;
+  }
+  static inline AssertingVH<T> getTombstoneKey() {
+    AssertingVH<T> Res;
+    Res.setRawValPtr(DenseMapInfo<Value *>::getTombstoneKey());
+    return Res;
   }
   static unsigned getHashValue(const AssertingVH<T> &Val) {
-    return PointerInfo::getHashValue(Val);
+    return DenseMapInfo<Value *>::getHashValue(Val.getRawValPtr());
   }
-#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();
+    return DenseMapInfo<Value *>::isEqual(LHS.getRawValPtr(),
+                                          RHS.getRawValPtr());
   }
-#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;

llvm/trunk/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;
if (Config::FollowRAUW) {
       }
     }
   }
 };
 
 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(DenseMapInfo<Value *>::getEmptyKey());
   }
   static inline VH getTombstoneKey() {
-    return VH(PointerInfo::getTombstoneKey(), nullptr);
+    return VH(DenseMapInfo<Value *>::getTombstoneKey());
   }
   static unsigned getHashValue(const VH &Val) {
-    return PointerInfo::getHashValue(Val.Unwrap());
+    return DenseMapInfo<KeyT>::getHashValue(Val.Unwrap());
   }
   static unsigned getHashValue(const KeyT &Val) {
-    return PointerInfo::getHashValue(Val);
+    return DenseMapInfo<KeyT>::getHashValue(Val);
   }
   static bool isEqual(const VH &LHS, const VH &RHS) {
     return LHS == RHS;
   }
   static bool isEqual(const KeyT &LHS, const VH &RHS) {
     return LHS == RHS.getValPtr();
   }
 };