Re: [PATCH v2] libtraceeval: Have TRACEEVAL_ARRAY_SIZE() handle NULL pointer

[Ross Zwisler <zwisler@google.com>]

On Thu, Oct 05, 2023 at 09:22:33PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> In the new addition to make sure that pointers passed to traceeval_init()
> and other functions that require a static array and not a dynamic one will
> cause the build to fail, this causes NULL pointers to fail the build too.
> 
> Although keys must be filled, vals are allowed to be NULL. It was assumed
> that:
> 
>    (void *)vals == NULL ? TRACEEVAL_ARRAY_SIZE(vals))
> 
> Would solve this, but it gcc was actually still giving a warning about
> 
>   warning: division 'sizeof (void *) / sizeof (void)' does not compute the number of array elements
> 
> But now it actually fails to build with the magic check.
> 
> Change TRACEEVAL_ARRAY_SIZE() to handle NULL for both keys and vals, by
> not only having:
> 
>  #define TRACEEVAL_ARRAY_SIZE(data) \
> 	((void *)(data) == NULL ?  0 : __TRACEEVAL_ARRAY_SIZE(data))
> 
> But that is not enough, as gcc still evaluates the second part, and it
> will fail to build. To handle this, change that to:
> 
>  #define __TRACEEVAL_ARRAY_SIZE(data)					\
> 	((sizeof(data) / (sizeof((data)[0])) + 0) +			\
> 
> The above adds " + 0" to the "sizeof((data)[0])" which quiets the warning
> mentioned above (the addition of zero breaks the normal pattern of finding
> an array size).
> 
> 	(int)(sizeof(struct {						\
> 		int:(-!!(__builtin_types_compatible_p(typeof(data),	\
> 						      typeof(&((data)[0]))) && \
> 			 (void *)(data) != NULL));			\
> 
> Added "&& (void *)(data) != NULL" that makes the above return false (zero)
> for a static array and NULL, which is exactly what we want.

Don't we already know it's not NULL because of the check in
TRACEEVAL_ARRAY_SIZE()?  Or do we really need to check for NULL in both
macros?

> 
> 			})))

A few random parens in the changelog. :)

> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v1: https://lore.kernel.org/all/20231005205129.6d6cbfad@gandalf.local.home/
> 
> - Fixed error in placement of parenthesis.
> 
>  include/traceeval.h | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/traceeval.h b/include/traceeval.h
> index bbf8f6ac7dd1..d8095ba5c1a1 100644
> --- a/include/traceeval.h
> +++ b/include/traceeval.h
> @@ -29,13 +29,17 @@
>   *
>   *    struct traceeval_data keys[] = { ... };
>   */
> -#define TRACEEVAL_ARRAY_SIZE(data)					\
> -	((sizeof(data) / sizeof((data)[0])) +				\
> +#define __TRACEEVAL_ARRAY_SIZE(data)					\
> +	((sizeof(data) / (sizeof((data)[0]) + 0)) +			\
>  	(int)(sizeof(struct {						\
>  		int:(-!!(__builtin_types_compatible_p(typeof(data),	\
> -						      typeof(&((data)[0]))))); \
> +						      typeof(&((data)[0]))) && \
> +			 (void *)(data) != NULL));			\
>  			})))
>  
> +#define TRACEEVAL_ARRAY_SIZE(data) \
> +	((void *)(data) == NULL ?  0 : __TRACEEVAL_ARRAY_SIZE(data))
> +
>  /* Data type distinguishers */
>  enum traceeval_data_type {
>  	TRACEEVAL_TYPE_NONE,
> @@ -191,7 +195,7 @@ struct traceeval;
>  #define traceeval_init(keys, vals)					\
>  	traceeval_init_size(keys, vals,					\
>  			    TRACEEVAL_ARRAY_SIZE(keys),			\
> -			    (void *)vals == NULL ?  0 : TRACEEVAL_ARRAY_SIZE(vals))
> +			    TRACEEVAL_ARRAY_SIZE(vals))
>  
>  #define traceeval_init_size(keys, vals, nr_keys, nr_vals)		\
>  	traceeval_init_data_size(keys, vals, nr_keys, nr_vals,		\
> @@ -211,7 +215,7 @@ int traceeval_insert_size(struct traceeval *teval,
>  
>  #define traceeval_insert(teval, keys, vals)				\
>  	traceeval_insert_size(teval, keys, TRACEEVAL_ARRAY_SIZE(keys), \
> -			      vals, (void *)vals == NULL ? 0 : TRACEEVAL_ARRAY_SIZE(vals))
> +			      vals, TRACEEVAL_ARRAY_SIZE(vals))
>  
>  int traceeval_remove_size(struct traceeval *teval,
>  			  const struct traceeval_data *keys, size_t nr_keys);
> -- 
> 2.40.1
>