diff mbox

powerpc: Fix unitialized variable

Message ID 5489B83F.6070007@linux.vnet.ibm.com
State Superseded
Delegated to: Adhemerval Zanella Netto
Headers

Commit Message

Adhemerval Zanella Netto Dec. 11, 2014, 3:29 p.m. UTC 
  On 11-12-2014 12:40, Joseph Myers wrote:
> On Thu, 11 Dec 2014, Adhemerval Zanella wrote:
>
>> +       int truncating, connreset, n;
>> +       /* There is the following warning on some architectures:
>> +          'resplen' may be used uninitialized in this function
>> +          [-Wmaybe-uninitialized]
>> +          This is a false positive according to:
>> +          https://www.sourceware.org/ml/libc-alpha/2014-12/msg00323.html
>> +        */
>> +       DIAG_PUSH_NEEDS_COMMENT;
>> +       DIAG_IGNORE_NEEDS_COMMENT (4.7, "-Wmaybe-uninitialized");
>> +       int resplen;
>> +       DIAG_POP_NEEDS_COMMENT;
> * Do you actually need this here, or only around the use of the variable?

Yes, in my testing we need on both places to silent the compiler.

>
> * An actual analysis of why the variable can't be used uninitialized would 
> be better than a URL.  I.e., if buf2 == NULL then this code won't be 
> executed; if buf2 != NULL, then first time round the loop recvresp1 and 
> recvresp2 will be 0 so this code won't be executed but "thisresplenp = 
> &resplen;" followed by "*thisresplenp = rlen;" will be executed so that 
> subsequent times round the loop resplen has been initialized.

I will replace the ULR pointer with a comment.

>
> * The version number in DIAG_IGNORE_NEEDS_COMMENT is the most recent GCC 
> version with which the issue has been observed, not the oldest.

Right, I thought I saw it failing by using something different that the compiler
used (GCC 4.6 in my tests), but it was a mistake from my part.  Changed to 5.

>
> * A conditional __GNUC_PREREQ (4, 7) is needed around the 
> DIAG_IGNORE_NEEDS_COMMENT call because 4.6 doesn't have 
> -Wmaybe-uninitialized (if the warnings appear with 4.6, a #else case to 
> use -Wuninitialized instead with 4.6 will be needed).
>
I added the guards.  I checked with GCC 4.6 and building with it does not
shows the warnings.

What about now:

--

2014-12-11  Stefan Liebler  <stli@linux.vnet.ibm.com>
	    Adhemerval Zanella  <azanella@linux.vnet.ibm.com>

	* resolv/res_send.c (send_vc): Disable warning resplen may
	be used uninitialized. 

--

Comments

Stefan Liebler Dec. 11, 2014, 4:27 p.m. UTC | #1 
>> * Do you actually need this here, or only around the use of the 
variable?
 >
 > Yes, in my testing we need on both places to silent the compiler.
On s390x, too.

> +	   by "*thisresplenp = rlen;" will be executed so that subsequent
> +	   times round the loop resplen has been initialized.  So this is
After "So this is" is a whitespace.
> +	   a false-positive.
> +	 */
> +#if __GNUC_PREREQ (4, 7)
> +	DIAG_PUSH_NEEDS_COMMENT;

tested on s390x with gcc 4.8 4.9, gcc head.

Thanks.
Adhemerval Zanella Netto Dec. 12, 2014, 11:35 a.m. UTC | #2 
Ping, this is the remaining fix to enable powerpc build with -werror.

PS: I fix the the comment trialing whitespace noted by Stefan.


On 11-12-2014 13:29, Adhemerval Zanella wrote:
> On 11-12-2014 12:40, Joseph Myers wrote:
>> On Thu, 11 Dec 2014, Adhemerval Zanella wrote:
>>
>>> +       int truncating, connreset, n;
>>> +       /* There is the following warning on some architectures:
>>> +          'resplen' may be used uninitialized in this function
>>> +          [-Wmaybe-uninitialized]
>>> +          This is a false positive according to:
>>> +          https://www.sourceware.org/ml/libc-alpha/2014-12/msg00323.html
>>> +        */
>>> +       DIAG_PUSH_NEEDS_COMMENT;
>>> +       DIAG_IGNORE_NEEDS_COMMENT (4.7, "-Wmaybe-uninitialized");
>>> +       int resplen;
>>> +       DIAG_POP_NEEDS_COMMENT;
>> * Do you actually need this here, or only around the use of the variable?
> Yes, in my testing we need on both places to silent the compiler.
>
>> * An actual analysis of why the variable can't be used uninitialized would 
>> be better than a URL.  I.e., if buf2 == NULL then this code won't be 
>> executed; if buf2 != NULL, then first time round the loop recvresp1 and 
>> recvresp2 will be 0 so this code won't be executed but "thisresplenp = 
>> &resplen;" followed by "*thisresplenp = rlen;" will be executed so that 
>> subsequent times round the loop resplen has been initialized.
> I will replace the ULR pointer with a comment.
>
>> * The version number in DIAG_IGNORE_NEEDS_COMMENT is the most recent GCC 
>> version with which the issue has been observed, not the oldest.
> Right, I thought I saw it failing by using something different that the compiler
> used (GCC 4.6 in my tests), but it was a mistake from my part.  Changed to 5.
>
>> * A conditional __GNUC_PREREQ (4, 7) is needed around the 
>> DIAG_IGNORE_NEEDS_COMMENT call because 4.6 doesn't have 
>> -Wmaybe-uninitialized (if the warnings appear with 4.6, a #else case to 
>> use -Wuninitialized instead with 4.6 will be needed).
>>
> I added the guards.  I checked with GCC 4.6 and building with it does not
> shows the warnings.
>
> What about now:
>
> --
>
> 2014-12-11  Stefan Liebler  <stli@linux.vnet.ibm.com>
> 	    Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
>
> 	* resolv/res_send.c (send_vc): Disable warning resplen may
> 	be used uninitialized. 
>
> --
>
> diff --git a/resolv/res_send.c b/resolv/res_send.c
> index af42b8a..9ec951a 100644
> --- a/resolv/res_send.c
> +++ b/resolv/res_send.c
> @@ -96,6 +96,7 @@ static const char rcsid[] = "$BINDId: res_send.c,v 8.38 2000/03/30 20:16:51 vixi
>  #include <string.h>
>  #include <unistd.h>
>  #include <kernel-features.h>
> +#include <libc-internal.h>
>
>  #if PACKETSZ > 65536
>  #define MAXPACKET       PACKETSZ
> @@ -668,7 +669,24 @@ send_vc(res_state statp,
>  	// int anssiz = *anssizp;
>  	HEADER *anhp = (HEADER *) ans;
>  	struct sockaddr_in6 *nsap = EXT(statp).nsaddrs[ns];
> -	int truncating, connreset, resplen, n;
> +	int truncating, connreset, n;
> +	/* On some architectures compiler might emit a warning indicating
> +	   'resplen' may be used uninitialized.  However if buf2 == NULL
> +	   then this code won't be executed; if buf2 != NULL, then first
> +	   time round the loop recvresp1 and recvresp2 will be 0 so this
> +	   code won't be executed but "thisresplenp = &resplen;" followed
> +	   by "*thisresplenp = rlen;" will be executed so that subsequent
> +	   times round the loop resplen has been initialized.  So this is 
> +	   a false-positive.
> +	 */
> +#if __GNUC_PREREQ (4, 7)
> +	DIAG_PUSH_NEEDS_COMMENT;
> +	DIAG_IGNORE_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
> +#endif
> +	int resplen;
> +#if __GNUC_PREREQ (4, 7)
> +	DIAG_POP_NEEDS_COMMENT;
> +#endif
>  	struct iovec iov[4];
>  	u_short len;
>  	u_short len2;
> @@ -787,6 +805,10 @@ send_vc(res_state statp,
>  			/* No buffer allocated for the first
>  			   reply.  We can try to use the rest
>  			   of the user-provided buffer.  */
> +#if __GNUC_PREREQ (4, 7)
> +			DIAG_PUSH_NEEDS_COMMENT;
> +			DIAG_IGNORE_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
> +#endif
>  #if _STRING_ARCH_unaligned
>  			*anssizp2 = orig_anssizp - resplen;
>  			*ansp2 = *ansp + resplen;
> @@ -797,6 +819,9 @@ send_vc(res_state statp,
>  			*anssizp2 = orig_anssizp - aligned_resplen;
>  			*ansp2 = *ansp + aligned_resplen;
>  #endif
> +#if __GNUC_PREREQ (4, 7)
> +			DIAG_POP_NEEDS_COMMENT;
> +#endif
>  		} else {
>  			/* The first reply did not fit into the
>  			   user-provided buffer.  Maybe the second
>
Roland McGrath Dec. 12, 2014, 9:08 p.m. UTC | #3 
Looks OK
diff mbox

Patch

diff --git a/resolv/res_send.c b/resolv/res_send.c
index af42b8a..9ec951a 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -96,6 +96,7 @@  static const char rcsid[] = "$BINDId: res_send.c,v 8.38 2000/03/30 20:16:51 vixi
 #include <string.h>
 #include <unistd.h>
 #include <kernel-features.h>
+#include <libc-internal.h>
 
 #if PACKETSZ > 65536
 #define MAXPACKET       PACKETSZ
@@ -668,7 +669,24 @@  send_vc(res_state statp,
 	// int anssiz = *anssizp;
 	HEADER *anhp = (HEADER *) ans;
 	struct sockaddr_in6 *nsap = EXT(statp).nsaddrs[ns];
-	int truncating, connreset, resplen, n;
+	int truncating, connreset, n;
+	/* On some architectures compiler might emit a warning indicating
+	   'resplen' may be used uninitialized.  However if buf2 == NULL
+	   then this code won't be executed; if buf2 != NULL, then first
+	   time round the loop recvresp1 and recvresp2 will be 0 so this
+	   code won't be executed but "thisresplenp = &resplen;" followed
+	   by "*thisresplenp = rlen;" will be executed so that subsequent
+	   times round the loop resplen has been initialized.  So this is 
+	   a false-positive.
+	 */
+#if __GNUC_PREREQ (4, 7)
+	DIAG_PUSH_NEEDS_COMMENT;
+	DIAG_IGNORE_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
+#endif
+	int resplen;
+#if __GNUC_PREREQ (4, 7)
+	DIAG_POP_NEEDS_COMMENT;
+#endif
 	struct iovec iov[4];
 	u_short len;
 	u_short len2;
@@ -787,6 +805,10 @@  send_vc(res_state statp,
 			/* No buffer allocated for the first
 			   reply.  We can try to use the rest
 			   of the user-provided buffer.  */
+#if __GNUC_PREREQ (4, 7)
+			DIAG_PUSH_NEEDS_COMMENT;
+			DIAG_IGNORE_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
+#endif
 #if _STRING_ARCH_unaligned
 			*anssizp2 = orig_anssizp - resplen;
 			*ansp2 = *ansp + resplen;
@@ -797,6 +819,9 @@  send_vc(res_state statp,
 			*anssizp2 = orig_anssizp - aligned_resplen;
 			*ansp2 = *ansp + aligned_resplen;
 #endif
+#if __GNUC_PREREQ (4, 7)
+			DIAG_POP_NEEDS_COMMENT;
+#endif
 		} else {
 			/* The first reply did not fit into the
 			   user-provided buffer.  Maybe the second