Message-ID: <202209251032.71251A8@keescook>
Date:   Sun, 25 Sep 2022 17:37:10 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
Cc:     intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, mchehab@...nel.org,
        chris@...is-wilson.co.uk, matthew.auld@...el.com,
        thomas.hellstrom@...ux.intel.com, jani.nikula@...el.com,
        nirmoy.das@...el.com, airlied@...hat.com, daniel@...ll.ch,
        andi.shyti@...ux.intel.com, andrzej.hajda@...el.com,
        mauro.chehab@...ux.intel.com, linux@...musvillemoes.dk,
        vitor@...saru.org, dlatypov@...gle.com, ndesaulniers@...gle.com
Subject: Re: [PATCH v11 3/9] compiler_types.h: Add assert_same_type to catch
 type mis-match while compiling

On Fri, Sep 23, 2022 at 11:26:22AM +0300, Gwan-gyeong Mun wrote:
> Adds assert_same_type and assert_same_typable macros to catch type
> mis-match while compiling. The existing typecheck() macro outputs build
> warnings, but the newly added assert_same_type() macro uses the
> static_assert macro (which uses _Static_assert keyword and it 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_same_type() macro,
> assert_same_typable() macro allows a constant value as the second argument.
> Since static_assert is used at compile time and it requires
> constant-expression as an argument [1][2], overflows_type_ret_const_expr()
> is newly added. There is overflows_type() that has the same behavior, but
> the macro uses __builtin_add_overflow() internally, and
> __builtin_add_overflows returns a bool type [3], so it is difficult to use
> as an argument of _Static_assert. The assert_same_type and
> assert_same_typable macros have been added to compiler_types.h, but the
> overflows_type_ret_const_expr macro has been added to overflow.h
> So, overflow.h has to be included to use assert_same_typable which
> internally uses overflows_type_ret_const_expr.
> And it adds unit tests for overflows_type, overflows_type_ret_const_expr,
> assert_same_type and assert_same_typable. The overflows_type has been added
> as well to compare whether the overflows_type_ret_const_expr unit test has
> the same as the result.

I spent some time rewriting the code in this patch. I think it's really
close, but I wanted to tweak how things were being defined, naming, etc.

Notes below, and I'll send my proposed patch separately...

> [...]
> +#define overflows_type_ret_const_expr(x,T) (			\

For the "overflows_type" defines, I think this reads a bit better:

#define __overflows_type_constexpr(x, T) (                      \
        is_unsigned_type(typeof(x)) ?                           \
                (x) > type_max(typeof(T)) ? 1 : 0               \
        : is_unsigned_type(typeof(T)) ?                         \
                (x) < 0 || (x) > type_max(typeof(T)) ? 1 : 0    \
                : (x) < type_min(typeof(T)) ||                  \
                  (x) > type_max(typeof(T)) ? 1 : 0 )

#define __overflows_type(x, T)          ({      \
        typeof(T) v = 0;                        \
        check_add_overflow((x), v, &v);         \
})

#define overflows_type(n, T)                                    \
        __builtin_choose_expr(__is_constexpr(n),                \
                              __overflows_type_constexpr(n, T), \
                              __overflows_type(n, T))

> [...]
> +/**
> + * assert_same_type - abort compilation if the first argument's data type and
> + *                    the second argument's data type are not the same
> + * @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 aborts compilation
> + * if the two types are not the same.
> + * See also assert_same_typable().
> + */
> +#define assert_same_type(t1, t2) static_assert(__same_type(t1, t2))

I still think I'd rather avoid a define for this. It doesn't seem worth
4 characters of savings to just have to type it out:

	static_assert(__same_type(a, b))

> [...]
> +#define assert_same_typable(t, n) static_assert(			       \
> +		__builtin_choose_expr(__builtin_constant_p(n),		       \
> +				      overflows_type_ret_const_expr(n,t) == 0, \
> +				      __same_type(t, n)))

This one I'd like to convert into something closer in naming convention to
"__same_type". Also note that "__builtin_constant_p()" doesn't actually
work here: it needs to be __is_constexpr(). So, I propose:

#define __castable_to_type(n, T)				\
		__builtin_choose_expr(__is_constexpr(n),	\
			__overflows_type_constexpr(n, T),	\
			__same_type(n, T))

Then we can do:

	static_assert(__castable_to_type(INT_MAX, size_t));

> [...[
> +static void overflows_type_test(struct kunit *test)
> +{
> +/* Args are: first type, secound type, value, overflow expected */
> +#define TEST_OVERFLOWS_TYPE(t1, t2, v, of) do {				\
> +	t1 __t1 = v;							\
> +	t2 __t2;							\
> +	bool __of;							\
> +	__of = overflows_type(v, t2);					\
> +	if (__of != of) {						\
> +		KUNIT_EXPECT_EQ_MSG(test, __of, of,			\
> +			"expected overflows_type(%s, %s) to%s overflow\n", \
> +			#v, #t2, of ? "" : " not");			\
> +	}								\
> [...]
> +	__of = overflows_type_ret_const_expr(__t1, __t2) ? true : false;\
> +	if (__of != of) {						\
> +		KUNIT_EXPECT_EQ_MSG(test, __of, of,			\
> +			"expected overflows_type_ret_const_expr(%s, %s) to%s overflow\n", \
> +			#t1" __t1 = "#v, #t2" __t2", of ? "" : " not");	\
> +	}								\

These tests are excellent! I've adapted them a little bit to avoid some
of their internal redundancy. (i.e. the above blocks are basically
almost entire the same, etc).

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists