On Wed, Oct 04, 2023 at 06:52:25PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
>
> Some functions use the TRACEEVAL_ARRAY_SIZE() macro to determine the size
> of the array passed to it. But this will not work if the developer uses a
> pointer to the array. gcc may give a warning, but it will still happily
> compile it leaving the developer wondering why their code does not work.
>
> Use a little trick that checks and tests if the array is static, and will
> fail the build if it is not.
>
> #define TRACEEVAL_ARRAY_SIZE(data) \
> ((sizeof(data) / sizeof((data)[0])) + \
> (int)(sizeof(struct { \
> int:(-!!(__builtin_types_compatible_p(typeof(data), \
> typeof(&((data)[0]))))); \
> })))
>
> Going backwards to explain the above, we have:
>
> __builtin_types_compatible_p(typeof(data), typeof(&((data)[0]))
>
> Which is a gcc/clang compiler directive that returns true if the two
> pointers are compatible [ (a, b) where a = b is valid ]. For a static
> array, we would have
>
> struct traceeval_data data[5]
>
> Where typeof(data) is "struct traceeval_data []" and the type of
> &data[0] is a pointer to "struct traceeval_data", and the above
> would return false (zero) [ data = &data[0] is invalid ].
>
> For pointers:
>
> struct traceeval_data *data;
>
> Then type of data is the same as &data[0] and it would return true (1).
> [ data = &data[0] is valid ]
>
> Now we have a structure defined:
>
> struct {
> int: (-!!(<result>));
> }
>
> Which if <result> of the __builtin_types_compatible_p() returned false
> (zero), then it would be:
>
> struct {
> int: 0; // structure with int size of 0 bits
> }
>
> Which is perfectly valid. But if <result> returned true (as it would if it
> was a pointer and not a static array), then it would be:
>
> struct {
> int: -1; // structure with int size of -1 bits!
> }
>
> Which is not valid to compile, and the build will fail.
>
> But in order to make sure this is in the code that uses
> TRACEEVAL_ARRAY_SIZE(), it needs to be part of the computation. To do
> that, as "struct { int:0; }" is of size zero, we can simply add a sizeof()
> around it, and attach the above with an addition "+".
>
> ... + sizeof((int)(sizeof(struct { int:0;}))) is the same as:
>
> ... + 0
>
> Now with this logic, if a pointer is passed to something like
> traceeval_init(), it will fail to build, and not cause hours of scratching
> head debugging for the developer at runtime.
>
> Of course, the developer will likely now scratch their head on why it
> doesn't build, but that's because they didn't RTFM!
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
That's some scary deep magic. :)
We may want to add a comment above this to briefly explain, i.e. "This macro
will fail to build if 'data' is a pointer and not a static array" and refer
them to the commit history? That way they have some way to proceed if/when
their build fails.
Otherwise looks good:
Reviewed-by: Ross Zwisler <zwisler@xxxxxxxxxx>
> ---
> include/traceeval.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/traceeval.h b/include/traceeval.h
> index 4cc5eb6ef3de..6c6e09c53129 100644
> --- a/include/traceeval.h
> +++ b/include/traceeval.h
> @@ -17,7 +17,12 @@
> /* Field name/descriptor for number of hits */
> #define TRACEEVAL_VAL_HITS ((const char *)(-1UL))
>
> -#define TRACEEVAL_ARRAY_SIZE(data) (sizeof(data) / sizeof((data)[0]))
> +#define TRACEEVAL_ARRAY_SIZE(data) \
> + ((sizeof(data) / sizeof((data)[0])) + \
> + (int)(sizeof(struct { \
> + int:(-!!(__builtin_types_compatible_p(typeof(data), \
> + typeof(&((data)[0]))))); \
> + })))
>
> /* Data type distinguishers */
> enum traceeval_data_type {
> --
> 2.40.1
>
|