[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

[Aaron Ballman via cfe-commits]

On Tue, Sep 12, 2017 at 1:02 PM, Roman Lebedev via Phabricator
<reviews at reviews.llvm.org> wrote:
> lebedev.ri added a comment.
>
> In https://reviews.llvm.org/D33826#868185, @aaron.ballman wrote:
>
>> In https://reviews.llvm.org/D33826#868167, @lebedev.ri wrote:
>>
>> > In https://reviews.llvm.org/D33826#868085, @aaron.ballman wrote:
>> >
>> > > In https://reviews.llvm.org/D33826#866170, @lebedev.ri wrote:
>> > >
>> > > > In https://reviews.llvm.org/D33826#866161, @JonasToth wrote:
>> > > >
>> > > > > There is an exception to the general rule (EXP36-C-EX2), stating that the result of `malloc` and friends is allowed to be casted to stricter alignments, since the pointer is known to be of correct alignment.
>> > > >
>> > > >
>> > > > Quote for the reference:
>> > > >
>> > > > > EXP36-C-EX2: If a pointer is known to be correctly aligned to the target type, then a cast to that type is permitted. There are several cases where a pointer is known to be correctly aligned to the target type. The pointer could point to an object declared with a suitable alignment specifier. It could point to an object returned by aligned_alloc(), calloc(), malloc(), or realloc(), as per the C standard, section 7.22.3, paragraph 1  [ISO/IEC 9899:2011].
>> > > >
>> > > > For plain `calloc(), malloc(), or realloc()`, i would guess it's related to `max_align_t` / `std::max_align_t` / `__STDCPP_DEFAULT_NEW_ALIGNMENT__`, which is generally just `16` bytes.
>> > >
>> > >
>> > > It's a requirement from the C standard that malloc, calloc, and realloc return a suitably-aligned pointer for *any* type.
>> >
>> >
>> > We are probably arguing about the details, or i'm just misguided, but you mean any *standard* type, right?
>> >
>> >   MALLOC(3)
>> >    ...
>> >    The  malloc()  and calloc() functions return a pointer to the allocated memory, which is suitably aligned for any built-in type.
>> >
>> >
>> > E.g. `__m256` needs to be 32-byte aligned, and  user type, with the help of `alignas()`, can have different alignment requirements
>>
>>
>> C11 7.22.3p1 (in part):
>>
>>   The pointer returned if the allocation succeeds is suitably aligned so that it may be assigned to
>>    a pointer to any type of object with a fundamental alignment requirement and then used
>>    to access such an object or an array of such objects in the space allocated (until the space
>>    is explicitly deallocated).
>>
>> C11 6.2.8p2 defines fundamental alignment:
>>
>>   A fundamental alignment is represented by an alignment less than or equal to the greatest
>>    alignment supported by the implementation in all contexts, which is equal to
>>    _Alignof (max_align_t).
>>
>> So I don't think this applies to only builtin types. Which makes sense, otherwise you couldn't rely on malloc to do the correct thing with `struct S { char c; int i; }; struct S s = (struct S *)malloc(sizeof(struct S));` However, you are correct that it doesn't need to return a suitably aligned pointer for *any* type.
>
>
> I agree!
> Thus, my point being, *if* the cast in question is not from `void*` (if it is, i don't think it is possible to warn, or prevent the warning for that matter),
> shouldn't the alignment requirements of the target type be considered, even if the pointer was just returned by `malloc` and friends?

Ah, I think I'm catching on to the point you're raising (thank you for
the patience). If the return type is void *, we don't have enough
information to sensibly diagnose (not without data flow analysis to
determine where the original value came from, anyway), and if the
return type is not void *, we should be checking the alignment anyway.

That suggests we don't need the exception in this check at all, unless
we find extant implementations that return something other than void
*.

~Aaron

>
>>>
>>>
>>>>>> Could you add a testcase for this case, i think there would currenlty be a false positive.
>>>>>>
>>>>>> And is there a general way of knowing when the pointer is of correct alignment, or is it necessary to keep a list of functions like `malloc` that are just known?
>>>>>>  If yes, i think it would be nice if this list is configurable (maybe like in cppcoreguidelines-no-malloc, where that functionality could be refactored out).
>>>>
>>>> Agreed, this should be a configurable list, but is should be pre-populated with the listed functions from the CERT standard.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D33826
>
>
>