Re: [PATCH 3/4] string: Suppress -Wmaybe-unitialized for wordcopy [BZ #19444]

[Carlos O'Donell <carlos@redhat.com>]

On 12/29/22 07:58, Adhemerval Zanella via Libc-alpha wrote:

I think this particular change needs a v2 and some more detailed analysis.

> With GCC 6+ when compiling with -O1 warns that some MERGE macro usage
> might be used uninitialized.  The issue is calling the function with
> len equal to 0 is undefined since the first 'switch' will not trigger
> any case and then subsequent loop will potentially use uninitialized
> variables.

The comment does not seem to match what the code does.

Calling the function with len equal to 0 results in 'case 0' executing in all of these
cases.

> However all usages on mem routines always called the function for
> sizes larger than OP_T_THRES.

This isn't always the case, you could have a call to copy just 8 bytes, which is
below the 16-byt OP_T_THRES.

> ---
>  string/wordcopy.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/string/wordcopy.c b/string/wordcopy.c
> index d05718322c..3b6344115d 100644
> --- a/string/wordcopy.c
> +++ b/string/wordcopy.c
> @@ -18,8 +18,19 @@
>  
>  /* BE VERY CAREFUL IF YOU CHANGE THIS CODE...!  */
>  
> +#include <assert.h>
>  #include <stddef.h>
> +#include <libc-diag.h>
> +/* With GCC 6 when compiling with -O1 warns that some MERGE macro usage might
> +   be used uninitialized.  The issue is calling the function with len equal to
> +   0 is undefined since the first 'switch' will not trigger any case and then
> +   subsequent loop will potentially use uninitialized variables.  However all
> +   usages on mem routines always called the function for sizes larger than

This comment does not match what the code does. The switch 'case 0:' case should execute.

> +   OP_T_THRES.  */
> +DIAG_PUSH_NEEDS_COMMENT;
> +DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized");
>  #include <memcopy.h>
> +DIAG_POP_NEEDS_COMMENT;
>  
>  /* _wordcopy_fwd_aligned -- Copy block beginning at SRCP to
>     block beginning at DSTP with LEN `op_t' words (not LEN bytes!).
> @@ -94,7 +105,11 @@ WORDCOPY_FWD_ALIGNED (long int dstp, long int srcp, size_t len)
>      {
>      do8:
>        a0 = ((op_t *) srcp)[0];
> +      /* Check the comment on memcopy.h inclusion.  */
> +      DIAG_PUSH_NEEDS_COMMENT;
> +      DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized");
>        ((op_t *) dstp)[0] = a1;
> +      DIAG_POP_NEEDS_COMMENT;

Why is a1 considered uninitialized?

The switch has case statements for every possible value.

In case 1 we unconditionally set a1.

We can't enter case 1 unless len was exactly 1 or any value of 1+8*n for valid n.

It seems like the compiler can't see that 'OP_T_THRES <= 3 * OPSIZ' is always a true.

>      do7:
>        a1 = ((op_t *) srcp)[1];
>        ((op_t *) dstp)[1] = a0;
> @@ -291,7 +306,11 @@ WORDCOPY_BWD_ALIGNED (long int dstp, long int srcp, size_t len)
>      {
>      do8:
>        a0 = ((op_t *) srcp)[7];
> +      /* Check the comment on memcopy.h inclusion.  */
> +      DIAG_PUSH_NEEDS_COMMENT;
> +      DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized");
>        ((op_t *) dstp)[7] = a1;
> +      DIAG_POP_NEEDS_COMMENT;

Likewise.

>      do7:
>        a1 = ((op_t *) srcp)[6];
>        ((op_t *) dstp)[6] = a0;

-- 
Cheers,
Carlos.