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
>
