diff mbox series

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

Message ID 20231005212233.22ae4e13@rorschach.local.home (mailing list archive)
State Under Review
Headers show
Series [v2] libtraceeval: Have TRACEEVAL_ARRAY_SIZE() handle NULL pointer | expand

Commit Message

Steven Rostedt Oct. 6, 2023, 1:22 a.m. UTC 
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.

			})))

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

Comments

Ross Zwisler Oct. 11, 2023, 10:33 p.m. UTC | #1 
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
>
Steven Rostedt Oct. 11, 2023, 11:14 p.m. UTC | #2 
On Wed, 11 Oct 2023 16:33:48 -0600
Ross Zwisler <zwisler@google.com> wrote:

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

Unfortunately what happens is that the compiler still checks the above. So
if we have just:

 	(int)(sizeof(struct {						\
 		int:(-!!(__builtin_types_compatible_p(typeof(data),	\
 						      typeof(&((data)[0])))));


Then the with NULL turns into:

	struct { int: -1; }

and fails the compile because:

 		__builtin_types_compatible_p(typeof(NULL), typeof(&((NULL)[0])))

Returns true.

So if we pair that with (void *)(data) != NULL, it will then return false
and turns into:

	struct { int: 0; }

Which is valid.

> 
> > 
> > 			})))  
> 
> A few random parens in the changelog. :)
> 

Yeah, the code had it too and I had to fix it. But didn't update the change log. :-p

-- Steve
Ross Zwisler Oct. 17, 2023, 2:47 p.m. UTC | #3 
On Wed, Oct 11, 2023 at 07:14:06PM -0400, Steven Rostedt wrote:
> On Wed, 11 Oct 2023 16:33:48 -0600
> Ross Zwisler <zwisler@google.com> wrote:
> 
> > 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?
> 
> Unfortunately what happens is that the compiler still checks the above. So
> if we have just:
> 
>  	(int)(sizeof(struct {						\
>  		int:(-!!(__builtin_types_compatible_p(typeof(data),	\
>  						      typeof(&((data)[0])))));
> 
> 
> Then the with NULL turns into:
> 
> 	struct { int: -1; }
> 
> and fails the compile because:
> 
>  		__builtin_types_compatible_p(typeof(NULL), typeof(&((NULL)[0])))
> 
> Returns true.
> 
> So if we pair that with (void *)(data) != NULL, it will then return false
> and turns into:
> 
> 	struct { int: 0; }
> 
> Which is valid.

Sounds good. If you haven't landed this already you can add:

Reviewed-by: Ross Zwisler <zwisler@google.com>
diff mbox series

Patch

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