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

[Alexey Samsonov]

On Wed, Aug 27, 2014 at 5:36 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:

>
> On Wed, Aug 27, 2014 at 4:59 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>>
>> On Wed, Aug 27, 2014 at 4:46 PM, Alexey Samsonov <vonosmas at gmail.com>
>> wrote:
>>
>>>
>>> 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).
>>>
>>
>> (wait, did you mean DenseMap<T, Foo>? Because DenseMap<T*, Foo> needs T*
>> to be defined, and T* is defined even if T is only declared)
>>
>
> No, I mean DenseMap<T*, Foo>.
> We can forward-declare type T and define a variable of type DenseMap<T*,
> Foo>. This definition uses DenseMapInfo<T*>, which uses
> PointerLikeTypeTraits<T*>. That is, we
> may want to know PointerLikeTypeTraits<T*>::NumLowBitsAvailable even if T
> is unknown. However, the actual number of available low bits of pointer T*
> depends on the alignment of
> type T, and we need the definition of T to calculate the latter.
>

OK, so we can fix the immediate problems with UBSan by a smaller change to
DenseMapInfo<AssertingVH<T>>. I've updated http://reviews.llvm.org/D4976.
Does it look reasonable?


>
>
>>
>> Sure - but I mean we provide specializations of PointerLikeTypeTraits<T*>
>> separate from the definition of T? For the default PointerLikeTypeTraits
>> (presumably with a NumLowBitsAvailable of zero) would allow the above. But
>> it seems a tricksy to have unrelated bits of code declaring that pointers
>> to certain objects have a certain non-zero number of low bits available.
>>
>> For example PointerLikeTypeTraits<Value*> is in Value.h, Use** is in
>> Use.h, etc. So it wouldn't be bad for those to also use alignof<T>.
>>
>> Hmm, maybe I've lost the plot somewhere along here...
>>
>>
>>
>>>
>>>> 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
>>>
>>
>>
>
>
> --
> 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/20140828/dd22a137/attachment.html>