[PATCH 0/4] Fix remaining -Os/-O1 compile issues [BZ #19444]

[PATCH 0/4] Fix remaining -Os/-O1 compile issues [BZ #19444]

[Adhemerval Zanella]

All targets should not build for -Os and/or -O1 without the need of
disable werrors.

Adhemerval Zanella (4):
  locale: Use correct buffer size for utf8_sequence_error [BZ #19444]
  sunrpc: Suppress GCC -O1 warning on user2netname [BZ #19444]
  string: Suppress -Wmaybe-unitialized for wordcopy [BZ #19444]
  math: Suppress -O0 warnings for soft-fp fsqrt [BZ #19444]

 locale/programs/linereader.c      |  2 +-
 string/wordcopy.c                 | 19 +++++++++++++++++++
 sunrpc/netname.c                  |  7 ++++---
 sysdeps/ieee754/soft-fp/s_fsqrt.c | 11 +++++++++++
 4 files changed, 35 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH 1/4] locale: Use correct buffer size for utf8_sequence_error [BZ #19444]

[Adhemerval Zanella]

The buffer used by snprintf might not be large enough for all possible
inputs, as indicated by gcc with -O1:

../locale/programs/linereader.c: In function ‘utf8_sequence_error’:
../locale/programs/linereader.c:713:58: error: ‘%02x’ directive output
may be truncated writing between 2 and 8 bytes into a region of size
between 1 and 13 [-Werror=format-truncation=]
  713 |     snprintf (buf, sizeof (buf), "0x%02x 0x%02x 0x%02x 0x%02x",
      |                                                          ^~~~
../locale/programs/linereader.c:713:34: note: directive argument in the
range [0, 2147483647]
  713 |     snprintf (buf, sizeof (buf), "0x%02x 0x%02x 0x%02x 0x%02x",
      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../locale/programs/linereader.c:713:5: note: ‘snprintf’ output between
20 and 38 bytes into a destination of size 30
  713 |     snprintf (buf, sizeof (buf), "0x%02x 0x%02x 0x%02x 0x%02x",
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  714 |               ch1, ch2, ch3, ch4);
      |               ~~~~~~~~~~~~~~~~~~~

Checked on x86_64-linux-gnu.
---
 locale/programs/linereader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/locale/programs/linereader.c b/locale/programs/linereader.c
index 0460074a0c..794f25a6e4 100644
--- a/locale/programs/linereader.c
+++ b/locale/programs/linereader.c
@@ -701,7 +701,7 @@ static bool
 utf8_sequence_error (struct linereader *lr, uint8_t ch1, int ch2, int ch3,
 		     int ch4)
 {
-  char buf[30];
+  char buf[38];
 
   if (ch2 < 0)
     snprintf (buf, sizeof (buf), "0x%02x", ch1);
-- 
2.34.1

Re: [PATCH 1/4] locale: Use correct buffer size for utf8_sequence_error [BZ #19444]

[Carlos O'Donell]

On 12/29/22 07:57, Adhemerval Zanella via Libc-alpha wrote:
> The buffer used by snprintf might not be large enough for all possible
> inputs, as indicated by gcc with -O1:
> 
> ../locale/programs/linereader.c: In function ‘utf8_sequence_error’:
> ../locale/programs/linereader.c:713:58: error: ‘%02x’ directive output
> may be truncated writing between 2 and 8 bytes into a region of size
> between 1 and 13 [-Werror=format-truncation=]
>   713 |     snprintf (buf, sizeof (buf), "0x%02x 0x%02x 0x%02x 0x%02x",
>       |                                                          ^~~~
> ../locale/programs/linereader.c:713:34: note: directive argument in the
> range [0, 2147483647]
>   713 |     snprintf (buf, sizeof (buf), "0x%02x 0x%02x 0x%02x 0x%02x",
>       |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../locale/programs/linereader.c:713:5: note: ‘snprintf’ output between
> 20 and 38 bytes into a destination of size 30
>   713 |     snprintf (buf, sizeof (buf), "0x%02x 0x%02x 0x%02x 0x%02x",
>       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   714 |               ch1, ch2, ch3, ch4);
>       |               ~~~~~~~~~~~~~~~~~~~
> 
> Checked on x86_64-linux-gnu.

LGTM. Took me a minute to work out the 38 bytes value though.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  locale/programs/linereader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/locale/programs/linereader.c b/locale/programs/linereader.c
> index 0460074a0c..794f25a6e4 100644
> --- a/locale/programs/linereader.c
> +++ b/locale/programs/linereader.c
> @@ -701,7 +701,7 @@ static bool
>  utf8_sequence_error (struct linereader *lr, uint8_t ch1, int ch2, int ch3,
>  		     int ch4)
>  {
> -  char buf[30];
> +  char buf[38];

OK.

"0x7f 0x7fffffff 0x7fffffff 0x7fffffff\0" 
= 4 + 1 + 10 + 1 + 10 + 1 + 10 + 1 = 38 bytes including null terminator.

I expect that at -O2 that VRP can see the values propagated from utf8_decode
and determine the buffer will be smaller by a lot given the range checks.

The ch2, ch3, and ch4 values shall not exceed 0xff in all cases since they
are all distinct bytes in the UTF-8 sequence. I expect that the worst case
is actually "0xff 0xff 0xff 0xff\0" or 20 bytes, but if the checks in
utf8_decode are changed then the worst case could be 38 bytes.

>  
>    if (ch2 < 0)
>      snprintf (buf, sizeof (buf), "0x%02x", ch1);

-- 
Cheers,
Carlos.

[PATCH 2/4] sunrpc: Suppress GCC -O1 warning on user2netname [BZ #19444]

[Adhemerval Zanella]

The same issue described by 6128e82ebe973163d2dd614d31753c88c0c4d645
also happend with -O1.

Checked on x86_64-linux-gnu.
---
 sunrpc/netname.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sunrpc/netname.c b/sunrpc/netname.c
index c1d1c43e50..a10591c071 100644
--- a/sunrpc/netname.c
+++ b/sunrpc/netname.c
@@ -49,10 +49,11 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid,
   if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN)
     return 0;
 
-  /* GCC with -Os warns that sprint might overflow while handling dfltdom,
-     however the above test does check if an overflow would happen.  */
+  /* GCC with -Os or -O1 warns that sprint might overflow while handling
+     dfltdom, however the above test does check if an overflow would
+     happen.  */
   DIAG_PUSH_NEEDS_COMMENT;
-  DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wformat-overflow");
+  DIAG_IGNORE_NEEDS_COMMENT (8, "-Wformat-overflow");
   sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
   DIAG_POP_NEEDS_COMMENT;
   i = strlen (netname);
-- 
2.34.1

Re: [PATCH 2/4] sunrpc: Suppress GCC -O1 warning on user2netname [BZ #19444]

[Carlos O'Donell]

On 12/29/22 07:58, Adhemerval Zanella via Libc-alpha wrote:
> The same issue described by 6128e82ebe973163d2dd614d31753c88c0c4d645
> also happend with -O1.
> 
> Checked on x86_64-linux-gnu.
> ---
>  sunrpc/netname.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 
> diff --git a/sunrpc/netname.c b/sunrpc/netname.c
> index c1d1c43e50..a10591c071 100644
> --- a/sunrpc/netname.c
> +++ b/sunrpc/netname.c
> @@ -49,10 +49,11 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid,
>    if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN)
>      return 0;
>  
> -  /* GCC with -Os warns that sprint might overflow while handling dfltdom,
> -     however the above test does check if an overflow would happen.  */
> +  /* GCC with -Os or -O1 warns that sprint might overflow while handling
> +     dfltdom, however the above test does check if an overflow would
> +     happen.  */
>    DIAG_PUSH_NEEDS_COMMENT;
> -  DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wformat-overflow");
> +  DIAG_IGNORE_NEEDS_COMMENT (8, "-Wformat-overflow");
>    sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
>    DIAG_POP_NEEDS_COMMENT;
>    i = strlen (netname);

-- 
Cheers,
Carlos.

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

[Adhemerval Zanella]

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 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
+   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;
     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;
     do7:
       a1 = ((op_t *) srcp)[6];
       ((op_t *) dstp)[6] = a0;
-- 
2.34.1

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

[Carlos O'Donell]

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.

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

[Adhemerval Zanella Netto]

[-- Attachment #1: Type: text/plain, Size: 4902 bytes --]



On 10/01/23 19:47, Carlos O'Donell wrote:
> 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.

The usage are in fact all done on generic mem functions.  The _wordcopy_fwd_aligned
and/or _wordcopy_fwd_dest_aligned are called by the WORD_COPY_FWD which, used by both
generic memcpy and memmove.  And both implementations only call the macro for length 
larger than OP_T_THRES.

This is similar for WORD_COPY_BWD, which is only used for memmove.c.

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

Right, reading again the code I think it might be indeed an issue with the
sparc backend since it is the only arch that triggers it, even though powerpc
also build the same code for both 64 and 32 bits.  What about:


  /* Compiling with -O1 might warn that 'a2' and 'a3' may be used
     uninitialized.  However, 'do3' (which uses 'a3') is only reachable either
     by 'case 0' or 'case 1', which initializes 'a3' (and it is also set at
     'do1' for subsequent loop iterations).  This is similar for 'do4'
     (which uses 'a2') that is only reachable by 'case 1' which initializes
     'a2').
     Since the usage is within the MERGE macro, we need to disable the warning
     on its definition.  */
> 
>> +   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.

I think it is unlikely because both OP_T_THRES and OPSIZ are constant macros,
although it might be case that the sparc backend is doing something fuzzy that
is maybe confusing some other passes.  I changed to:

      /* Compiling with -O1 might warn that 'a1' in 'do8' switch may be used
         uninitialized.  However, 'do8' is only reachable through 'case 1', 
         since all possible modulo value are handling in the initial switch).
         */

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

I am attaching the fixed patch.

[-- Attachment #2: 0003-string-Suppress-Wmaybe-unitialized-for-wordcopy-BZ-1.patch --]
[-- Type: text/plain, Size: 2869 bytes --]

From 86b83da8c7836484654fbacaf54f764df378a31e Mon Sep 17 00:00:00 2001
From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date: Thu, 29 Dec 2022 09:15:49 -0300
Subject: [PATCH 3/4] string: Suppress -Wmaybe-unitialized for wordcopy [BZ
 #19444]

The GCC 6+ when compiling for sparc warns that some variables might be
used uninitialized.  However it does not seem the fact, since the
variables are really initialized (and also other targets that use the
same code, like powerpc, do not warn about it).

So suppress the warning for now.
---
 string/wordcopy.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/string/wordcopy.c b/string/wordcopy.c
index ae5ccd793c..d0cfc22082 100644
--- a/string/wordcopy.c
+++ b/string/wordcopy.c
@@ -19,7 +19,12 @@
 /* BE VERY CAREFUL IF YOU CHANGE THIS CODE...!  */
 
 #include <stddef.h>
+#include <libc-diag.h>
+/* Check comment of WORDCOPY_FWD_DEST_ALIGNED.  */
+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 +99,14 @@ WORDCOPY_FWD_ALIGNED (long int dstp, long int srcp, size_t len)
     {
     do8:
       a0 = ((op_t *) srcp)[0];
+      /* Compiling with -O1 may warn that 'a1' in 'do8' switch may be used
+	 uninitialized.  However, 'do8' is only reachable through 'case 1',
+	 since all possible modulo values are handling in the initial
+	 switch).  */
+      DIAG_PUSH_NEEDS_COMMENT;
+      DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized");
       ((op_t *) dstp)[0] = a1;
+      DIAG_POP_NEEDS_COMMENT;
     do7:
       a1 = ((op_t *) srcp)[1];
       ((op_t *) dstp)[1] = a0;
@@ -190,6 +202,14 @@ WORDCOPY_FWD_DEST_ALIGNED (long int dstp, long int srcp, size_t len)
       goto do4;			/* No-op.  */
     }
 
+  /* Compiling with -O1 might warn that 'a2' and 'a3' may be used
+     uninitialized.  However, 'do3' (which uses 'a3') is only reachable either
+     by 'case 0' or 'case 1', which initializes 'a3' (and it is also set at
+     'do1' for subsequent loop iterations).  This is similar for 'do4'
+     (which uses 'a2') that is only reachable by 'case 1' which initializes
+     'a2').
+     Since the usage is within the MERGE macro, we need to disable the warning
+     on its definition.  */
   do
     {
     do4:
@@ -291,7 +311,11 @@ WORDCOPY_BWD_ALIGNED (long int dstp, long int srcp, size_t len)
     {
     do8:
       a0 = ((op_t *) srcp)[7];
+      /* Check the comment on WORDCOPY_FWD_ALIGNED.  */
+      DIAG_PUSH_NEEDS_COMMENT;
+      DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized");
       ((op_t *) dstp)[7] = a1;
+      DIAG_POP_NEEDS_COMMENT;
     do7:
       a1 = ((op_t *) srcp)[6];
       ((op_t *) dstp)[6] = a0;
-- 
2.34.1

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

[Carlos O'Donell]

On 1/11/23 08:14, Adhemerval Zanella Netto wrote:
> I am attaching the fixed patch.

Could you please submit this in a new thread as a distinct patch from the series?

I think all the other patches can go in immediately.

This also helps me track in patchwork and for CI to re-test your patch.

-- 
Cheers,
Carlos.

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

[Carlos O'Donell]

On 1/11/23 08:14, Adhemerval Zanella Netto wrote:
> On 10/01/23 19:47, Carlos O'Donell wrote:
>> 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.

Downthread I've asked you to repost in a new thread, but I'll carry out a review
here to reduce the turnaround time.

>>> 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.
> 
> The usage are in fact all done on generic mem functions.  The _wordcopy_fwd_aligned
> and/or _wordcopy_fwd_dest_aligned are called by the WORD_COPY_FWD which, used by both
> generic memcpy and memmove.  And both implementations only call the macro for length 
> larger than OP_T_THRES.

We are talking about examples like those in string/memmove.c:

 58       if (len >= OP_T_THRES)
 59         {
 60           /* Copy just a few bytes to make DSTP aligned.  */
 61           len -= (-dstp) % OPSIZ;
 62           BYTE_COPY_FWD (dstp, srcp, (-dstp) % OPSIZ);
 63 
 64           /* Copy whole pages from SRCP to DSTP by virtual address
 65              manipulation, as much as possible.  */
 66 
 67           PAGE_COPY_FWD_MAYBE (dstp, srcp, len, len);
 68 
 69           /* Copy from SRCP to DSTP taking advantage of the known
 70              alignment of DSTP.  Number of bytes remaining is put
 71              in the third argument, i.e. in LEN.  This number may
 72              vary from machine to machine.  */
 73 
 74           WORD_COPY_FWD (dstp, srcp, len, len);
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Only called with len >= OP_T_THRES
 75 
 76           /* Fall out and copy the tail.  */
 77         }

I agree completely with you here.

My point earlier was that the macros themselves have no 'assert(len > OP_T_THRES);'
or any indication that they should not be called with len < OP_T_THRES.

> This is similar for WORD_COPY_BWD, which is only used for memmove.c.

Agreed.

>>
>>> ---
>>>  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.
> 
> Right, reading again the code I think it might be indeed an issue with the
> sparc backend since it is the only arch that triggers it, even though powerpc
> also build the same code for both 64 and 32 bits.  What about:

Right.
 
>   /* Compiling with -O1 might warn that 'a2' and 'a3' may be used
>      uninitialized.  However, 'do3' (which uses 'a3') is only reachable either
>      by 'case 0' or 'case 1', which initializes 'a3' (and it is also set at
>      'do1' for subsequent loop iterations).  This is similar for 'do4'
>      (which uses 'a2') that is only reachable by 'case 1' which initializes
>      'a2').
>      Since the usage is within the MERGE macro, we need to disable the warning
>      on its definition.  */

OK.

>>
>>> +   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.
> 
> I think it is unlikely because both OP_T_THRES and OPSIZ are constant macros,
> although it might be case that the sparc backend is doing something fuzzy that
> is maybe confusing some other passes.  I changed to:
> 
>       /* Compiling with -O1 might warn that 'a1' in 'do8' switch may be used
>          uninitialized.  However, 'do8' is only reachable through 'case 1', 
>          since all possible modulo value are handling in the initial switch).
>          */

OK.

> 
>>
>>>      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;
>>
> 
> I am attaching the fixed patch.

> From 86b83da8c7836484654fbacaf54f764df378a31e Mon Sep 17 00:00:00 2001
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Date: Thu, 29 Dec 2022 09:15:49 -0300
> Subject: [PATCH 3/4] string: Suppress -Wmaybe-unitialized for wordcopy [BZ
>  #19444]
> 
> The GCC 6+ when compiling for sparc warns that some variables might be
> used uninitialized.  However it does not seem the fact, since the
> variables are really initialized (and also other targets that use the
> same code, like powerpc, do not warn about it).

Suggest:

When compiling with GCC 6+ the sparc build warns that some variables might
be used uninitialized. 


> 
> So suppress the warning for now.
> ---
>  string/wordcopy.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/string/wordcopy.c b/string/wordcopy.c
> index ae5ccd793c..d0cfc22082 100644
> --- a/string/wordcopy.c
> +++ b/string/wordcopy.c
> @@ -19,7 +19,12 @@
>  /* BE VERY CAREFUL IF YOU CHANGE THIS CODE...!  */
>  
>  #include <stddef.h>
> +#include <libc-diag.h>
> +/* Check comment of WORDCOPY_FWD_DEST_ALIGNED.  */

Please move the comment up to here.

> +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 +99,14 @@ WORDCOPY_FWD_ALIGNED (long int dstp, long int srcp, size_t len)
>      {
>      do8:
>        a0 = ((op_t *) srcp)[0];
> +      /* Compiling with -O1 may warn that 'a1' in 'do8' switch may be used
> +	 uninitialized.  However, 'do8' is only reachable through 'case 1',
> +	 since all possible modulo values are handling in the initial
> +	 switch).  */

Please move the comment to just above the store of a1 into dstp.

Suggest:

/* Compiling with -O1 may warn that 'a1' may be used uninitialized. There are only
   two ways to arrive at label 'do8' and they are via a do-while loop iteration
   or directly via the earlier switch 'case 1:' case. The switch case always sets
   'a1' and all previous loop iterations will also have set 'a1' before the use.  */



> +      DIAG_PUSH_NEEDS_COMMENT;
> +      DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized");
>        ((op_t *) dstp)[0] = a1;
> +      DIAG_POP_NEEDS_COMMENT;
>      do7:
>        a1 = ((op_t *) srcp)[1];
>        ((op_t *) dstp)[1] = a0;
> @@ -190,6 +202,14 @@ WORDCOPY_FWD_DEST_ALIGNED (long int dstp, long int srcp, size_t len)
>        goto do4;			/* No-op.  */
>      }
>  

Please move this comment up to the point at which you have the DIAG* since
it should apply to both FWD and BWD uses that use the MERGE macros.

> +  /* Compiling with -O1 might warn that 'a2' and 'a3' may be used
> +     uninitialized.  However, 'do3' (which uses 'a3') is only reachable either
> +     by 'case 0' or 'case 1', which initializes 'a3' (and it is also set at
> +     'do1' for subsequent loop iterations).  This is similar for 'do4'
> +     (which uses 'a2') that is only reachable by 'case 1' which initializes
> +     'a2').
> +     Since the usage is within the MERGE macro, we need to disable the warning
> +     on its definition.  */

Suggest:

/* Compiling with -O1 might warn that 'a2' and 'a3' may be used
   uninitialized.  There are only two ways to arrive at labels
   'do4', 'do3' or 'do1', all of which use 'a2' or 'a3' in the
   MERGE macro: either from the earlier switch case statement
   or via a loop iteration.  In all cases the switch statement
   or previous loop sets both 'a2' and 'a3'.

   Since the usage is within the MERGE macro we disable the
   warning in the definition, but only in this file.  */

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

-- 
Cheers,
Carlos.

[PATCH 4/4] math: Suppress -O0 warnings for soft-fp fsqrt [BZ #19444]

[Adhemerval Zanella]

The patch suppress the same warnings from 87c266d758d29e52bfb717f90,
that shows issues for microblaze, mips soft-fp, nios2, and or1k.
---
 sysdeps/ieee754/soft-fp/s_fsqrt.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sysdeps/ieee754/soft-fp/s_fsqrt.c b/sysdeps/ieee754/soft-fp/s_fsqrt.c
index ef631bc6c2..d5c7904118 100644
--- a/sysdeps/ieee754/soft-fp/s_fsqrt.c
+++ b/sysdeps/ieee754/soft-fp/s_fsqrt.c
@@ -25,6 +25,15 @@
 #undef fsqrtl
 
 #include <math-narrow.h>
+#include <libc-diag.h>
+/* R_f[01] are not set in cases where they are not used in packing,
+   but the compiler does not see that they are set in all cases where
+   they are used, resulting in warnings that they may be used
+   uninitialized.  The location of the warning differs in different
+   versions of GCC, it may be where R is defined using a macro or it
+   may be where the macro is defined.  This happens only with -O1.  */
+DIAG_PUSH_NEEDS_COMMENT;
+DIAG_IGNORE_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
 #include <soft-fp.h>
 #include <single.h>
 #include <double.h>
@@ -51,4 +60,6 @@ __fsqrt (double x)
   CHECK_NARROW_SQRT (ret, x);
   return ret;
 }
+DIAG_POP_NEEDS_COMMENT;
+
 libm_alias_float_double (sqrt)
-- 
2.34.1

Re: [PATCH 4/4] math: Suppress -O0 warnings for soft-fp fsqrt [BZ #19444]

[Carlos O'Donell]

On 12/29/22 07:58, Adhemerval Zanella via Libc-alpha wrote:
> The patch suppress the same warnings from 87c266d758d29e52bfb717f90,
> that shows issues for microblaze, mips soft-fp, nios2, and or1k.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  sysdeps/ieee754/soft-fp/s_fsqrt.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sysdeps/ieee754/soft-fp/s_fsqrt.c b/sysdeps/ieee754/soft-fp/s_fsqrt.c
> index ef631bc6c2..d5c7904118 100644
> --- a/sysdeps/ieee754/soft-fp/s_fsqrt.c
> +++ b/sysdeps/ieee754/soft-fp/s_fsqrt.c
> @@ -25,6 +25,15 @@
>  #undef fsqrtl
>  
>  #include <math-narrow.h>
> +#include <libc-diag.h>
> +/* R_f[01] are not set in cases where they are not used in packing,
> +   but the compiler does not see that they are set in all cases where
> +   they are used, resulting in warnings that they may be used
> +   uninitialized.  The location of the warning differs in different
> +   versions of GCC, it may be where R is defined using a macro or it
> +   may be where the macro is defined.  This happens only with -O1.  */
> +DIAG_PUSH_NEEDS_COMMENT;
> +DIAG_IGNORE_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");

OK. Agreed, R's usage in _FP_SQRT is conditional.

>  #include <soft-fp.h>
>  #include <single.h>
>  #include <double.h>
> @@ -51,4 +60,6 @@ __fsqrt (double x)
>    CHECK_NARROW_SQRT (ret, x);
>    return ret;
>  }
> +DIAG_POP_NEEDS_COMMENT;
> +
>  libm_alias_float_double (sqrt)

-- 
Cheers,
Carlos.