lkml.org 
[lkml]   [2022]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2022-09-26 02:37    [W:4.621 / U:0.500 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site