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

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 30 Jun 2025 16:20:21 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

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

The long term goal here is to build with LTO during review.
Best if there are no outstanding warnings in that case.
LTO has found some pre-existing bugs because it has wider visibility across file boundaries.