[Intel-gfx] [PATCH v10 3/9] compiler_types.h: Add assert_type to catch type mis-match while compiling

[Rasmus Villemoes]

On 11/09/2022 13.04, Andi Shyti wrote:
> Hi Gwan-gyeong,
> 
> On Fri, Sep 09, 2022 at 07:59:07PM +0900, Gwan-gyeong Mun wrote:
>> It adds assert_type and assert_typable macros to catch type mis-match while
> 
> /Add/It adds/, please use the imperative form.
> 
>> compiling. The existing typecheck() macro outputs build warnings, but the
>> newly added assert_type() macro uses the _Static_assert() keyword (which is
>> introduced in C11) to generate a build break when the types are different
>> and can be used to detect explicit build errors.
>> Unlike the assert_type() macro, assert_typable() macro allows a constant
>> value as the second argument.
>>
>> Suggested-by: Kees Cook <keescook at chromium.org>
>> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> Cc: Matthew Auld <matthew.auld at intel.com>
>> Cc: Nirmoy Das <nirmoy.das at intel.com>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: Andi Shyti <andi.shyti at linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab at kernel.org>
>> Cc: Andrzej Hajda <andrzej.hajda at intel.com>
>> Cc: Kees Cook <keescook at chromium.org>
>> ---
>>  include/linux/compiler_types.h | 39 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
>> index 4f2a819fd60a..19cc125918bb 100644
>> --- a/include/linux/compiler_types.h
>> +++ b/include/linux/compiler_types.h
>> @@ -294,6 +294,45 @@ struct ftrace_likely_data {
>>  /* Are two types/vars the same type (ignoring qualifiers)? */
>>  #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>>  
>> +/**
>> + * assert_type - break compile if the first argument's data type and the second
>> + *               argument's data type are not the same
> 
> I would use /aborts compilation/break compile/
> 
>> + *
> 
> nowhere is written that this extra blank line is not needed, but
> just checking the style in compiler_types.h it is not used.
> 
> I personally like the blank line, but standing to the general
> taste, it should be removed also for keeping a coherent style.
> 
>> + * @t1: data type or variable
>> + * @t2: data type or variable
>> + *
>> + * The first and second arguments can be data types or variables or mixed (the
>> + * first argument is the data type and the second argument is variable or vice
>> + * versa). It determines whether the first argument's data type and the second
>> + * argument's data type are the same while compiling, and it breaks compile if
>> + * the two types are not the same.
>> + * See also assert_typable().
>> + */
>> +#define assert_type(t1, t2) _Static_assert(__same_type(t1, t2))
> 
> In C11 _Static_assert is defined as:
> 
>   _Static_assert ( constant-expression , string-literal ) ;
> 
> While
> 
>   _Static_assert ( constant-expression ) ;
> 
> is defined in C17 along with the previous. I think you should add
> the error message as a 'string-literal'.

See how static_assert() is defined in linux/build_bug.h, and let's avoid
using _Static_assert directly. So this should IMO just be

#define assert_same_type(t1, t2) static_assert(__same_type(t1, t2))

(including the naming of the macro; I don't think "assert_type" is a
good name). No need to add an explicit string literal, the name of the
macro and the constant expression itself are plenty to explain what is
being asserted (with the latter being the reason the string was made
optional).

Rasmus