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

[Alexey Samsonov]

On Wed, Aug 27, 2014 at 4:15 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
>
> On Wed, Aug 27, 2014 at 4:09 PM, Alexey Samsonov <vonosmas at gmail.com>
> wrote:
>
>>
>> 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.
>>>
>>
>> OK, I was confused =/ PointerLikeTypeTraits<T*>::NumLowBitsAvailable
>> provides information
>> about the contents of T*, so what actually matters is alignOf<T>, not
>> alignOf<T*>. However,
>> alignOf<T> requires that T is defined, which is probably a restriction we
>> don't want to impose.
>>
>
> Do we ever provide PointerLikeTypeTraits<T*> for T somewhere other than
> where T is defined? That seems subtle/easily surprising.
>

Well, yes, you can declare a class member of type DenseMap<T*, Foo> if T is
forward-declared (which is convenient).

>
> I suppose there's the case where PointerLikeTypeTraits<T*> is the default
> implementation - and in that case we don't want to impose the limitation
> that T is defined? Is there any way we can do that conditionally?
>
> - David
>
>
>>
>>
>>>
>>> - 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
>>
>
>


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