libtraceeval: Use trick to force static array usage where needed

Commit Message

Steven Rostedt Oct. 4, 2023, 10:52 p.m. UTC

From: "Steven Rostedt (Google)" <[e-mail]>

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) <[e-mail]>
---
 include/traceeval.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Ross Zwisler Oct. 5, 2023, 9:25 p.m. UTC | #1

On Wed, Oct 04, 2023 at 06:52:25PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <[e-mail]>
> 
> 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) <[e-mail]>

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 <[e-mail]>

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

Steven Rostedt Oct. 5, 2023, 9:59 p.m. UTC | #2

On Thu, 5 Oct 2023 15:25:21 -0600
Ross Zwisler <[e-mail]> wrote:

> That's some scary deep magic.  :)

Yeah it is. Hence the detailed change log, and not just:

  "The macro prevents static traceeval_data *keys from being used"

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

Well, I don't think they need to understand how the macro work. But just
why it failed. Even though new compilers also warn about why it doesn't
work, I can still add:

/*
 * If your compile failed due to the below macro, it is likely you used
 * a pointer to struct traceeval_data, and not a static array.
 *
 * That is:
 *
 *    struct traceeval_data *keys;
 *
 * and not:
 *
 *    struct traceeval_data keys[] = { ... };
 */

I'll send a v2

> 
> Otherwise looks good:
> Reviewed-by: Ross Zwisler <[e-mail]>

Thanks!

-- Steve

Patch

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 {