Re: [PATCH v2] test/argparse: change initialization to workaround LTO

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Jun 30, 2025 at 07:58:49AM -0700, Stephen Hemminger wrote:
> When compiled with Link Time Optimization, the existing code
> generated an error, because the compiler was unable to intuit
> that there was space in the flexible array.
> 
> In function ‘test_argparse_copy’,
>     inlined from ‘test_argparse_init_obj’ at ../app/test/test_argparse.c:108:2,
>     inlined from ‘test_argparse_opt_callback_parse_int_of_no_val’ at ../app/test/test_argparse.c:490:8:
> ../app/test/test_argparse.c:96:17: warning: ‘memcpy’ writing 56 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
>    96 |                 memcpy(&dst->args[i], &src->args[i], sizeof(src->args[i]));
> 
> Initialiizing a structure with flexible array is special case
> and compiler expands the structure to fit. But inside the copy
> function it no longer knew that.
> 
> The workaround is to put the copy inside the same function
> and use structure assignment. Also macro should be uppper case.
> 
> Fixes: 6c5c6571601c ("argparse: verify argument config")
> Cc: fengchengwen@huawei.com
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - simpler fix is to just inline the copy
> 
>  app/test/test_argparse.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 

LGTM. One suggestion inline, in case you feel like adjusting things
further.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
> index 0a229752fa..d5b777e321 100644
> --- a/app/test/test_argparse.c
> +++ b/app/test/test_argparse.c
> @@ -71,7 +71,7 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
>  }
>  
>  /* valid templater, must contain at least two args. */
> -#define argparse_templater() { \
> +#define ARGPARSE_TEMPLATE { \
>  	.prog_name = "test_argparse", \
>  	.usage = "-a xx -b yy", \
>  	.descriptor = NULL, \
> @@ -87,25 +87,24 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
>  	}, \
>  }
>  
> -static void
> -test_argparse_copy(struct rte_argparse *dst, struct rte_argparse *src)
> -{
> -	uint32_t i;
> -	memcpy(dst, src, sizeof(*src));
> -	for (i = 0; /* NULL */; i++) {
> -		memcpy(&dst->args[i], &src->args[i], sizeof(src->args[i]));
> -		if (src->args[i].name_long == NULL)
> -			break;
> -	}
> -}
>  
>  static struct rte_argparse *
>  test_argparse_init_obj(void)
>  {
> -	static struct rte_argparse backup = argparse_templater();
> -	static struct rte_argparse obj = argparse_templater();
> -	/* Because obj may be overwritten, do a deep copy. */
> -	test_argparse_copy(&obj, &backup);
> +	/* Note: initialization of structure with flexible arrary
> +	 * increases the size of the variable to match.
> +	 */
> +	static const struct rte_argparse backup = ARGPARSE_TEMPLATE;
> +	static struct rte_argparse obj = ARGPARSE_TEMPLATE;
> +	unsigned int i;
> +
> +	obj = backup;
> +	for (i = 0; ; i++) {
> +		obj.args[i] = backup.args[i];
> +		if (backup.args[i].name_long == NULL)
> +			break;
> +	}

We should consider either making this a "do { } while" loop or adding the
termination condition to the "for" loop statement as normal. For example:

	unsigned int i = 0;

	obj = backup;
	do {
		obj.args[i] = backup.args[i];
	} while (backup.args[++i].name_long != NULL);

or else:

	obj = backup;
	for (i = 0; backup.args[i].name_long != NULL; i++)
		obj.args[i] = backup.args[i];
	obj.args[i] = ARGPARSE_ARG_END();

I'd tend toward the second, myself, but what is in your patch above is fine
as-is too.

> +
>  	return &obj;
>  }
>  
> -- 
> 2.47.2
>