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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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