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

[Duncan P. N. Exon Smith]

> On 2015-Jan-09, at 13:52, Chandler Carruth <chandlerc at gmail.com> wrote:
> 
> ================
> Comment at: include/llvm/IR/ValueHandle.h:192-203
> @@ -191,14 +191,14 @@
> 
> #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
> 
> ----------------
> I would define helpers here for getRawValPtr and setRawValPtr which do the indirection between NDEBUG and debug builds. Then use them below (see comments).
> 
> ================
> 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
> };
> ----------------
> 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 *>.
> 

Just in case you're waiting for something from me, this LGTM (once
Chandler's happy).  Thanks for doing this.

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