[OB] Fix linux-low.c build error

[OB] Fix linux-low.c build error

[Hui Zhu]

Got:
gcc -c  -g -O2    -I. -I../../../src/gdb/gdbserver
-I../../../src/gdb/gdbserver/../common
-I../../../src/gdb/gdbserver/../regformats
-I../../../src/gdb/gdbserver/../../include -Wall
-Wdeclaration-after-statement -Wpointer-arith -Wformat-nonliteral
-Wno-char-subscripts -Werror ../../../src/gdb/gdbserver/linux-low.c
-DUSE_THREAD_DB
cc1: warnings being treated as errors
../../../src/gdb/gdbserver/linux-low.c: In function ‘linux_create_inferior’:
../../../src/gdb/gdbserver/linux-low.c:580:10: error: ignoring return
value of ‘write’, declared with attribute warn_unused_result

2011-12-18  Hui Zhu  <teawater@gmail.com>

	* linux-low.c (linux_create_inferior): Save return value to ret.
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.185
retrieving revision 1.186
diff -u -r1.185 -r1.186
--- src/gdb/gdbserver/linux-low.c	2011/12/16 19:06:37	1.185
+++ src/gdb/gdbserver/linux-low.c	2011/12/18 15:49:04	1.186
@@ -574,11 +574,12 @@
 	 Also, redirect stdin to /dev/null.  */
       if (remote_connection_is_stdio ())
 	{
+	  int ret;
 	  close (0);
 	  open ("/dev/null", O_RDONLY);
 	  dup2 (2, 1);
-	  write (2, "stdin/stdout redirected\n",
-		 sizeof ("stdin/stdout redirected\n") - 1);
+	  ret = write (2, "stdin/stdout redirected\n",
+		       sizeof ("stdin/stdout redirected\n") - 1);
 	}

       execv (program, allargs);

Re: [OB] Fix linux-low.c build error

[Jan Kratochvil]

On Sun, 18 Dec 2011 16:49:43 +0100, Hui Zhu wrote:
> Got:
> gcc -c  -g -O2    -I. -I../../../src/gdb/gdbserver
> -I../../../src/gdb/gdbserver/../common
> -I../../../src/gdb/gdbserver/../regformats
> -I../../../src/gdb/gdbserver/../../include -Wall
> -Wdeclaration-after-statement -Wpointer-arith -Wformat-nonliteral
> -Wno-char-subscripts -Werror ../../../src/gdb/gdbserver/linux-low.c
> -DUSE_THREAD_DB
> cc1: warnings being treated as errors
> ../../../src/gdb/gdbserver/linux-low.c: In function ‘linux_create_inferior’:
> ../../../src/gdb/gdbserver/linux-low.c:580:10: error: ignoring return
> value of ‘write’, declared with attribute warn_unused_result

(a) Please do not wrap the lines, it is very unreadable this way.

(b) Please write down the GCC version you used.  I do not see such behavior at
    any of the FSF GCC versions
	FSF: gcc (GCC) 4.4.7 20111218 (prerelease)
	FSF: gcc (GCC) 4.5.4 20111218 (prerelease)
	FSF: gcc (GCC) 4.6.3 20111218 (prerelease)
	FSF: gcc (GCC) 4.7.0 20111218 (experimental)
    nor at Fedora GCC versions such as gcc-4.6.2-1.fc17.1.x86_64.

(c) With these GCCs
	FSF: gcc (GCC) 4.6.3 20111218 (prerelease)
	FSF: gcc (GCC) 4.7.0 20111218 (experimental)
	gcc-4.6.2-1.fc17.1.x86_64 gcc-4.6.2-1.fc16.x86_64
	gcc-4.6.1-9.fc15.x86_64 (older Fedoras are EOLed)
    it errors with:
	gcc -c  -m64 -ggdb2 -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4    -I. -I. -I./../common -I./../regformats -I./../../include -Wall -Wdeclaration-after-statement -Wpointer-arith -Wformat-nonliteral -Wno-char-subscripts -Werror linux-low.c -DUSE_THREAD_DB
	linux-low.c: In function ‘linux_create_inferior’:
	linux-low.c:577:8: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
	cc1: all warnings being treated as errors
    This is a regression.

(d) Please do not push changes needed for any obsolete and/or broken vendor
    compilers into FSF GDB.  I really have (or had) a ton of patches specific
    to Fedoras, I do not push them to FSF GDB.  They should stay at vendor GDB
    branches.

Please revert the patch or fix it or FSF GCC >= 4.6.


Thanks,
Jan


> --- src/gdb/gdbserver/linux-low.c	2011/12/16 19:06:37	1.185
> +++ src/gdb/gdbserver/linux-low.c	2011/12/18 15:49:04	1.186
> @@ -574,11 +574,12 @@
>  	 Also, redirect stdin to /dev/null.  */
>        if (remote_connection_is_stdio ())
>  	{
> +	  int ret;

Here should be empty line, between declarations and code.

>  	  close (0);
>  	  open ("/dev/null", O_RDONLY);
>  	  dup2 (2, 1);
> -	  write (2, "stdin/stdout redirected\n",
> -		 sizeof ("stdin/stdout redirected\n") - 1);
> +	  ret = write (2, "stdin/stdout redirected\n",
> +		       sizeof ("stdin/stdout redirected\n") - 1);
>  	}
> 
>        execv (program, allargs);

[obv] Fix unused-but-set-variable error [Re: [OB] Fix linux-low.c build error]

[Jan Kratochvil]

On Sun, 18 Dec 2011 20:22:39 +0100, Jan Kratochvil wrote:
> (c) With these GCCs
> 	FSF: gcc (GCC) 4.6.3 20111218 (prerelease)
> 	FSF: gcc (GCC) 4.7.0 20111218 (experimental)
[...]
> 	linux-low.c: In function ‘linux_create_inferior’:
> 	linux-low.c:577:8: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]

Checked in this alternative fix to not break the next nightly builds.

I believe it now should pass also on Ubuntu, tested it with artificial
warn_unused_result attribute for `write'.


Regards,
Jan


http://sourceware.org/ml/gdb-cvs/2011-12/msg00185.html

--- src/gdb/gdbserver/ChangeLog	2011/12/18 15:49:04	1.529
+++ src/gdb/gdbserver/ChangeLog	2011/12/18 20:55:08	1.530
@@ -1,3 +1,11 @@
+2011-12-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* linux-low.c (linux_create_inferior): Put empty if clause for write.
+
+	Revert:
+	2011-12-18  Hui Zhu  <teawater@gmail.com>
+	* linux-low.c (linux_create_inferior): Save return value to ret.
+
 2011-12-18  Hui Zhu  <teawater@gmail.com>
 
 	* linux-low.c (linux_create_inferior): Save return value to ret.
--- src/gdb/gdbserver/linux-low.c	2011/12/18 15:49:04	1.186
+++ src/gdb/gdbserver/linux-low.c	2011/12/18 20:55:08	1.187
@@ -574,12 +574,12 @@
 	 Also, redirect stdin to /dev/null.  */
       if (remote_connection_is_stdio ())
 	{
-	  int ret;
 	  close (0);
 	  open ("/dev/null", O_RDONLY);
 	  dup2 (2, 1);
-	  ret = write (2, "stdin/stdout redirected\n",
-		       sizeof ("stdin/stdout redirected\n") - 1);
+	  if (write (2, "stdin/stdout redirected\n",
+		     sizeof ("stdin/stdout redirected\n") - 1) < 0)
+	    /* Errors ignored.  */;
 	}
 
       execv (program, allargs);

Re: [obv] Fix unused-but-set-variable error [Re: [OB] Fix linux-low.c build error]

[Hui Zhu]

Thanks Jan.

Best,
Hui

2011/12/19 Jan Kratochvil <jan.kratochvil@redhat.com>:
> On Sun, 18 Dec 2011 20:22:39 +0100, Jan Kratochvil wrote:
>> (c) With these GCCs
>>       FSF: gcc (GCC) 4.6.3 20111218 (prerelease)
>>       FSF: gcc (GCC) 4.7.0 20111218 (experimental)
> [...]
>>       linux-low.c: In function ‘linux_create_inferior’:
>>       linux-low.c:577:8: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
>
> Checked in this alternative fix to not break the next nightly builds.
>
> I believe it now should pass also on Ubuntu, tested it with artificial
> warn_unused_result attribute for `write'.
>
>
> Regards,
> Jan
>
>
> http://sourceware.org/ml/gdb-cvs/2011-12/msg00185.html
>
> --- src/gdb/gdbserver/ChangeLog 2011/12/18 15:49:04     1.529
> +++ src/gdb/gdbserver/ChangeLog 2011/12/18 20:55:08     1.530
> @@ -1,3 +1,11 @@
> +2011-12-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
> +
> +       * linux-low.c (linux_create_inferior): Put empty if clause for write.
> +
> +       Revert:
> +       2011-12-18  Hui Zhu  <teawater@gmail.com>
> +       * linux-low.c (linux_create_inferior): Save return value to ret.
> +
>  2011-12-18  Hui Zhu  <teawater@gmail.com>
>
>        * linux-low.c (linux_create_inferior): Save return value to ret.
> --- src/gdb/gdbserver/linux-low.c       2011/12/18 15:49:04     1.186
> +++ src/gdb/gdbserver/linux-low.c       2011/12/18 20:55:08     1.187
> @@ -574,12 +574,12 @@
>         Also, redirect stdin to /dev/null.  */
>       if (remote_connection_is_stdio ())
>        {
> -         int ret;
>          close (0);
>          open ("/dev/null", O_RDONLY);
>          dup2 (2, 1);
> -         ret = write (2, "stdin/stdout redirected\n",
> -                      sizeof ("stdin/stdout redirected\n") - 1);
> +         if (write (2, "stdin/stdout redirected\n",
> +                    sizeof ("stdin/stdout redirected\n") - 1) < 0)
> +           /* Errors ignored.  */;
>        }
>
>       execv (program, allargs);

Re: [obv] Fix unused-but-set-variable error [Re: [OB] Fix linux-low.c build error]

[Joel Brobecker]

In this case, I think that the warning was helpful regardless of
the version of GCC that emitted it. However, I did think that Hui's
solution was only sweeping the problem under the carpet and that
did make me uncomfortable.

> Checked in this alternative fix to not break the next nightly builds.

I much prefer your solution which makes it obvious that we decided
to ignore write errors.

-- 
Joel

Re: [obv] Fix unused-but-set-variable error [Re: [OB] Fix linux-low.c build error]

[Yao Qi]

On 12/19/2011 04:58 AM, Jan Kratochvil wrote:
> Checked in this alternative fix to not break the next nightly builds.
> 
> I believe it now should pass also on Ubuntu, tested it with artificial
> warn_unused_result attribute for `write'.
> 
> 
> Regards,
> Jan
> 
> 
> http://sourceware.org/ml/gdb-cvs/2011-12/msg00185.html
> 
> --- src/gdb/gdbserver/ChangeLog	2011/12/18 15:49:04	1.529
> +++ src/gdb/gdbserver/ChangeLog	2011/12/18 20:55:08	1.530
> @@ -1,3 +1,11 @@
> +2011-12-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
> +
> +	* linux-low.c (linux_create_inferior): Put empty if clause for write.
> +
> +	Revert:
> +	2011-12-18  Hui Zhu  <teawater@gmail.com>
> +	* linux-low.c (linux_create_inferior): Save return value to ret.
> +

This reminds me that I committed a similar patch some days ago.  Here is a
patch to revert.

-- 
Yao (齐尧) 

	* tracepoint.c (gdb_ust_thread): Put an empty if clause for write.

	Revert:
	2011-12-14  Yao Qi  <yao@codesourcery.com>
	* tracepoint.c (gdb_ust_thread): Don't ignore return value
	of write.

Index: gdb/gdbserver/tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/tracepoint.c,v
retrieving revision 1.38
diff -u -r1.38 tracepoint.c
--- gdb/gdbserver/tracepoint.c	15 Dec 2011 12:40:03 -0000	1.38
+++ gdb/gdbserver/tracepoint.c	19 Dec 2011 06:17:49 -0000
@@ -8122,8 +8122,8 @@
 		strcpy (cmd_buf, "");
 	    }
 
-	  /* Fix compiler's warning: ignoring return value of 'write'.  */
-	  ret = write (fd, buf, 1);
+	  if (write (fd, buf, 1) < 0)
+	    /* Errors ignored.  */;
 	  close (fd);
 	}
     }

Re: [obv] Fix unused-but-set-variable error [Re: [OB] Fix linux-low.c build error]

[Joel Brobecker]

> This reminds me that I committed a similar patch some days ago.  Here is a
> patch to revert.
> 
> -- 
> Yao (??????) 
> 
> 	* tracepoint.c (gdb_ust_thread): Put an empty if clause for write.
> 
> 	Revert:
> 	2011-12-14  Yao Qi  <yao@codesourcery.com>
> 	* tracepoint.c (gdb_ust_thread): Don't ignore return value
> 	of write.

Thanks! This begs the question, though: Why is it OK to ignore
the error?

I am looking at the code and, not knowing much about tracepoints, I don't
understand well the prupose of gdb_ust_thread (hint: No function
description). As far as I can tell, the write is simply writing the byte
that was read from the connection back to it. What will happen if
we fail to write, and should the user be warned, for instance?

IIRC, in the previous case, we were simply writing to stderr, I think,
and there wasn't much else we could do if that failed...

> Index: gdb/gdbserver/tracepoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/tracepoint.c,v
> retrieving revision 1.38
> diff -u -r1.38 tracepoint.c
> --- gdb/gdbserver/tracepoint.c	15 Dec 2011 12:40:03 -0000	1.38
> +++ gdb/gdbserver/tracepoint.c	19 Dec 2011 06:17:49 -0000
> @@ -8122,8 +8122,8 @@
>  		strcpy (cmd_buf, "");
>  	    }
>  
> -	  /* Fix compiler's warning: ignoring return value of 'write'.  */
> -	  ret = write (fd, buf, 1);
> +	  if (write (fd, buf, 1) < 0)
> +	    /* Errors ignored.  */;
>  	  close (fd);
>  	}
>      }

-- 
Joel

Re: [obv] Fix unused-but-set-variable error [Re: [OB] Fix linux-low.c build error]

[Yao Qi]

On 12/19/2011 02:50 PM, Joel Brobecker wrote:
> Thanks! This begs the question, though: Why is it OK to ignore
> the error?
> 
> I am looking at the code and, not knowing much about tracepoints, I don't
> understand well the prupose of gdb_ust_thread (hint: No function
> description). As far as I can tell, the write is simply writing the byte
> that was read from the connection back to it. What will happen if
> we fail to write, and should the user be warned, for instance?
> 

This native socket is used for synchronization between gdbserver and
libinproctrace.so agent.  gdbserver puts command into cmd_buf, writes
one byte in socket to notify gdb_ust_thread (a dedicated thread) and
wait.  gdb_ust_thread picks up command from cmd_buf.  When process is
done, gdb_ust_thread writes one byte again to notify gdbserver and close
socket.  I don't think we have to do something w.r.t write failure.  The
description above is quite user-invisible, so a warning may confuse
users to some extent, IMO.

-- 
Yao (齐尧)

Re: [obv] Fix unused-but-set-variable error [Re: [OB] Fix linux-low.c build error]

[Joel Brobecker]

> This native socket is used for synchronization between gdbserver and
> libinproctrace.so agent.  gdbserver puts command into cmd_buf, writes
> one byte in socket to notify gdb_ust_thread (a dedicated thread) and
> wait.  gdb_ust_thread picks up command from cmd_buf.  When process is
> done, gdb_ust_thread writes one byte again to notify gdbserver and close
> socket.  I don't think we have to do something w.r.t write failure.  The
> description above is quite user-invisible, so a warning may confuse
> users to some extent, IMO.

Sounds good to me. But wouldn't a comment in the code be useful?
It's probably obvious to you today, but it might not be to someone
else, or even to you in a couple of years from now...

-- 
Joel

Re: [obv] Fix unused-but-set-variable error [Re: [OB] Fix linux-low.c build error]

[Jan Kratochvil]

On Mon, 19 Dec 2011 07:26:03 +0100, Yao Qi wrote:
> This reminds me that I committed a similar patch some days ago.  Here is a
> patch to revert.
[...]
> -	  /* Fix compiler's warning: ignoring return value of 'write'.  */
> -	  ret = write (fd, buf, 1);
> +	  if (write (fd, buf, 1) < 0)
> +	    /* Errors ignored.  */;
>  	  close (fd);
>  	}
>      }

I would add a comment that the variable "ret" is already used elsewhere incl.
reading it in that function.  This is the reason it did not trigger the
unused-but-set-variable error.

Still sure I agree such patch is useful (+Joel suggests more comments for it),
this is unwised code dependency.  .


Thanks,
Jan