On Mon, 30 Jun 2025 16:20:21 +0100
Bruce Richardson <bruce.richard...@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: fengcheng...@huawei.com
> >
> > Signed-off-by: Stephen Hemminger <step...@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.richard...@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.
