[PATCH] ADT: Remove misaligned pointeres from DenseMapInfo

[Duncan P. N. Exon Smith]

> On 2015-Jan-08, at 15:39, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> On 2015-Jan-08, at 15:15, Richard Smith <richard at metafoo.co.uk> wrote:
>> 
>> On Thu, Jan 8, 2015 at 3:04 PM, Chandler Carruth <chandlerc at google.com> wrote:
>> 
>> On Thu, Jan 8, 2015 at 2:59 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
>>  This is where UBSan reports an error - P is not a pointer to an actual object, it's a fake value (0xfffff....), which is not even
>>  properly aligned.
>> 
>> As I've said before, I believe UBSan is incorrect to report an error at this point. I don't believe the cast to a pointer is UB, I believe using that pointer for anything other than casting back to an integer is UB. We can't even compare it to another pointer (such a compare would also produce 'false', even for comparison with itself).
>> 
>> It looks like what's happening is that this invalid pointer value is being upcast, and that is what UBSan is complaining about. If so, I think UBSan is right, but the bug is in DenseMapInfo<AssertingVH<T>>: it shouldn't be assuming that DenseMapInfo<T*>'s getEmptyKey produces a valid pointer to a T; it obviously cannot do so.
> 
> Okay, this makes sense.  Thanks Alexey (and Richard) for clarifying.
> 
> Interestingly, there's only undefined behaviour in asserts builds,
> since `AssertingVH` only inherits from `ValueHandleBase` (and upcasts)
> in asserts builds.
> 
> This sort of thing should work:
> 
>    private:
>    #ifndef NDEBUG
>      AssertingVH(ValueTy *Raw, bool)
>        : ValueHandleBase(Assert, reinterpret_cast<Value *>(Raw)) {}
>    #endif
> 
>    public:
>      static AssertingVH<T> getDenseMapKey(ValueTy *Raw) {
>    #ifndef NDEBUG
>        assert(Raw == DenseMapInfo<ValueTy *>::getEmptyKey() ||
>               Raw == DenseMapInfo<ValueTy *>::getTombstoneKey());
>        return AssertingVH(Raw, true);

Or this might need to be:

    private:
      AssertingVH(Value *Key, bool) : ValueHandleBase(Assert, Key) {}

    public:
      static AssertingVH<T> getDenseMapKey(ValueTy *Key) {
    #ifndef NDEBUG
        assert(Raw == DenseMapInfo<ValueTy *>::getEmptyKey() ||
               Raw == DenseMapInfo<ValueTy *>::getTombstoneKey());
        if (Raw == DenseMapInfo<ValueTy *>::getEmptyKey())
          return AssertingVH(DenseMapInfo<Value *>::getEmptyKey(), bool());
        return AssertingVH(DenseMapInfo<Value *>::getTombstoneKey(), bool());
    #else
        return AssertingVH(Raw);
      }

>    #else
>        return AssertingVH(Raw);
>    #endif
>      }
> 
> Then we can have:
> 
>     template <class T> struct DenseMapInfo<AssertingVH<T>> {
>       static AssertingVH<T> getEmptyKey() {
>         return AssertingVH<T>::getDenseMapKey(DenseMapInfo<T *>::getEmptyKey());
>       }
> 
>       static AssertingVH<T> getTombstoneKey() {
>         return AssertingVH<T>::getDenseMapKey(DenseMapInfo<T *>::getTombstoneKey());
>       }
> 
>       // ...
>     };