Re: [PATCH] libtraceeval: Use trick to force static array usage where needed

[Ross Zwisler <zwisler@google.com>]

On Wed, Oct 04, 2023 at 06:52:25PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> 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@goodmis.org>

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@google.com>

> ---
>  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
>