[PATCH] Use properly aligned pointers for empty/tombstone DenseMap keys.

[Alexey Samsonov]

On Tue, Aug 19, 2014 at 4:20 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Aug 19, 2014 at 2:59 PM, Alexey Samsonov <vonosmas at gmail.com>
> wrote:
> > PointerLikeTypeTraits::NumLowBitsAvailable have different values for
> > different kinds of pointers (e.g. certain pointers can be cache-line
> > aligned). Do you suggest to bump the default value of NumLowBitsAvailable
> > for generic T* to return the natural alignment of T*?
>
> I /think/ that'd be a reasonable thing to do. If anyone cares about
> the number of low bits available, presumably they care even if those
> bits are just provided by the natural alignment.
>

Yes, I'm inclined to do this change. Though, there are pretty confusing
overrides of NumLowBitsAvailable in the
codebase. For instance, PointerLikeTypeTraits<clang::IdentifierInfo*> with
NumLowBitsAvailable set to 1,
with a comment:
// Provide PointerLikeTypeTraits for IdentifierInfo pointers, which
// are not guaranteed to be 8-byte aligned.
Er, why?



>
> - David
>
> >
> >
> > On Tue, Aug 19, 2014 at 2:09 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>
> >> On Tue, Aug 19, 2014 at 1:30 PM, Alexey Samsonov <vonosmas at gmail.com>
> >> wrote:
> >> > Hi rsmith, chandlerc,
> >> >
> >> > We should respect alignOf<T*>() when creating empty/tombstone keys
> >> > for DenseMap<T*, foo>. Otherwise this can lead to casts on misaligned
> >> > addresses, which is undefined behavior, as reported by UBSan.
> >> >
> >> > http://reviews.llvm.org/D4976
> >> >
> >> > Files:
> >> >   include/llvm/ADT/DenseMapInfo.h
> >> >
> >> > Index: include/llvm/ADT/DenseMapInfo.h
> >> > ===================================================================
> >> > --- include/llvm/ADT/DenseMapInfo.h
> >> > +++ include/llvm/ADT/DenseMapInfo.h
> >> > @@ -14,6 +14,7 @@
> >> >  #ifndef LLVM_ADT_DENSEMAPINFO_H
> >> >  #define LLVM_ADT_DENSEMAPINFO_H
> >> >
> >> > +#include "llvm/Support/AlignOf.h"
> >> >  #include "llvm/Support/PointerLikeTypeTraits.h"
> >> >  #include "llvm/Support/type_traits.h"
> >> >
> >> > @@ -32,12 +33,20 @@
> >> >  struct DenseMapInfo<T*> {
> >> >    static inline T* getEmptyKey() {
> >> >      uintptr_t Val = static_cast<uintptr_t>(-1);
> >> > -    Val <<= PointerLikeTypeTraits<T*>::NumLowBitsAvailable;
> >> > +    const unsigned Alignment =
> >> > +        PointerLikeTypeTraits<T*>::NumLowBitsAvailable >
> alignOf<T*>()
> >> > +            ?
> (unsigned)PointerLikeTypeTraits<T*>::NumLowBitsAvailable
> >> > +            : alignOf<T*>();
> >>
> >> Should this be built into PointerLikeTypeTraits instead?
> >>
> >> > +    Val <<= Alignment;
> >> >      return reinterpret_cast<T*>(Val);
> >> >    }
> >> >    static inline T* getTombstoneKey() {
> >> >      uintptr_t Val = static_cast<uintptr_t>(-2);
> >> > -    Val <<= PointerLikeTypeTraits<T*>::NumLowBitsAvailable;
> >> > +    const unsigned Alignment =
> >> > +        PointerLikeTypeTraits<T*>::NumLowBitsAvailable >
> alignOf<T*>()
> >> > +            ?
> (unsigned)PointerLikeTypeTraits<T*>::NumLowBitsAvailable
> >> > +            : alignOf<T*>();
> >> > +    Val <<= Alignment;
> >> >      return reinterpret_cast<T*>(Val);
> >> >    }
> >> >    static unsigned getHashValue(const T *PtrVal) {
> >> >
> >> > _______________________________________________
> >> > llvm-commits mailing list
> >> > llvm-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >> >
> >
> >
> >
> >
> > --
> > Alexey Samsonov
> > vonosmas at gmail.com
>



-- 
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140819/7e208c17/attachment.html>