[PATCH] ADT: Remove misaligned pointeres from DenseMapInfo

[Duncan P. N. Exon Smith]

> On 2015-Jan-08, at 15:56, Chandler Carruth <chandlerc at google.com> wrote:
> 
> After arguing about this for like an hour with Richard, David, and Alexey, I think we have a better path forward...
> 
> Much as you suggest, the key is nuking the upcast in the asserts build. But we can fix a number of other things in the process. For example, it is really annoying that we construct ValueHandle objects for the empty and tombstone keys. Instead, it would be better if we could not construct an object at all, and it turns out there is a nice way to do this.

Colour me interested...

> Unfortunately, it requires some refactoring to DenseMap and DenseMapInfo to expose this freedom. I'm going to do that refactoring (which seems like generally good cleanup), and then we can look at a more direct way of fixing the bug here.

SGTM!

> On Thu, Jan 8, 2015 at 3:45 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > 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());
> >       }
> >
> >       // ...
> >     };
> 
>