[PATCH OBV] Add nowarnings in gdb.base/fileio.exp

[PATCH OBV] Add nowarnings in gdb.base/fileio.exp

[Yao Qi]

I see the following warning in gdb.base/fileio.c,

testsuite/gdb.base/fileio.c:297:3: warning: null argument where non-null required (argument 1) [-Wnonnull]
   ret = stat (NULL, &st);
   ^

This patch adds "nowarnings" to the list passed to gdb_compile.

It is obvious, patch is pushed in.

gdb/testsuite:

2017-05-17  Yao Qi  <yao.qi@linaro.org>

	* gdb.base/fileio.exp: Pass nowarnings to gdb_compile.
---
 gdb/testsuite/ChangeLog           | 4 ++++
 gdb/testsuite/gdb.base/fileio.exp | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 91712e2..6f877da 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2017-05-17  Yao Qi  <yao.qi@linaro.org>
+
+	* gdb.base/fileio.exp: Pass nowarnings to gdb_compile.
+
 2017-05-17  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* gdb.base/set-inferior-tty.exp (test_set_inferior_tty): Add
diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/fileio.exp
index 6bb7141..14aaa0d 100644
--- a/gdb/testsuite/gdb.base/fileio.exp
+++ b/gdb/testsuite/gdb.base/fileio.exp
@@ -31,7 +31,7 @@ if {[is_remote host]} {
 
 if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
 	   executable \
-	   [list debug "additional_flags=-DOUTDIR=\"$outdir/\""]] != "" } {
+	   [list debug nowarnings "additional_flags=-DOUTDIR=\"$outdir/\""]] != "" } {
     untested "failed to compile"
     return -1
 }
-- 
1.9.1

Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp

[Pedro Alves]

FWIW, I think that wrapping the offending code with

 #pragma diagnostics push
 #pragma diagnostics ignored "-Wnonnull"
 ... stat (NULL, ...);
 #pragma diagnostics pop

would be appropriate in this case.  This testcase is checking that the syscalls
on the target map back to host I/O on the gdb side, and some targets have
slightly non-POSIX-like system calls APIs; IMHO, it's better to see warnings
due to such mismatches instead of potentially silently miscompiling.

Thanks,
Pedro Alves

On 05/17/2017 02:45 PM, Yao Qi wrote:
> I see the following warning in gdb.base/fileio.c,
> 
> testsuite/gdb.base/fileio.c:297:3: warning: null argument where non-null required (argument 1) [-Wnonnull]
>    ret = stat (NULL, &st);
>    ^
> 
> This patch adds "nowarnings" to the list passed to gdb_compile.
> 
> It is obvious, patch is pushed in.
> 
> gdb/testsuite:
> 
> 2017-05-17  Yao Qi  <yao.qi@linaro.org>
> 
> 	* gdb.base/fileio.exp: Pass nowarnings to gdb_compile.
> ---
>  gdb/testsuite/ChangeLog           | 4 ++++
>  gdb/testsuite/gdb.base/fileio.exp | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 91712e2..6f877da 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-05-17  Yao Qi  <yao.qi@linaro.org>
> +
> +	* gdb.base/fileio.exp: Pass nowarnings to gdb_compile.
> +
>  2017-05-17  Simon Marchi  <simon.marchi@ericsson.com>
>  
>  	* gdb.base/set-inferior-tty.exp (test_set_inferior_tty): Add
> diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/fileio.exp
> index 6bb7141..14aaa0d 100644
> --- a/gdb/testsuite/gdb.base/fileio.exp
> +++ b/gdb/testsuite/gdb.base/fileio.exp
> @@ -31,7 +31,7 @@ if {[is_remote host]} {
>  
>  if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
>  	   executable \
> -	   [list debug "additional_flags=-DOUTDIR=\"$outdir/\""]] != "" } {
> +	   [list debug nowarnings "additional_flags=-DOUTDIR=\"$outdir/\""]] != "" } {
>      untested "failed to compile"
>      return -1
>  }
> 

Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> FWIW, I think that wrapping the offending code with
>
>  #pragma diagnostics push
>  #pragma diagnostics ignored "-Wnonnull"
>  ... stat (NULL, ...);
>  #pragma diagnostics pop
>
> would be appropriate in this case.  This testcase is checking that the syscalls
> on the target map back to host I/O on the gdb side, and some targets have
> slightly non-POSIX-like system calls APIs; IMHO, it's better to see warnings
> due to such mismatches instead of potentially silently miscompiling.

The diagnostic was added in gcc 4.6.  Do we require gcc 4.6 to run
testsuite?  I am OK with this requirement.

-- 
Yao (齐尧)

Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp

[Pedro Alves]

On 05/18/2017 09:34 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> FWIW, I think that wrapping the offending code with
>>
>>  #pragma diagnostics push
>>  #pragma diagnostics ignored "-Wnonnull"
>>  ... stat (NULL, ...);
>>  #pragma diagnostics pop
>>
>> would be appropriate in this case.  This testcase is checking that the syscalls
>> on the target map back to host I/O on the gdb side, and some targets have
>> slightly non-POSIX-like system calls APIs; IMHO, it's better to see warnings
>> due to such mismatches instead of potentially silently miscompiling.
> 
> The diagnostic was added in gcc 4.6.  Do we require gcc 4.6 to run
> testsuite?  I am OK with this requirement.

I don't think so.  I think that'd be the equivalent of saying that we
don't support debugging target code built with gcc versions earlier
than 4.6.  That's a totally different discussion from the discussion
about requirements for building gdb.

How about an even simpler fix: pass a global pointer variable that
happens to be NULL to stat, instead of a NULL literal.  The compiler can't
prove (without LTO) that the variable may still be NULL, so it doesn't
warn.  Works on all compilers I tried it on: gcc 3.4/5.3/7, clang 3.7.

Let me send a mini series in reply to this email, which does that,
and also fixes a few other warnings that compiling the fileio.c with
-Wall exposes.

Thanks,
Pedro Alves

[PATCH 3/3] gdb.base/fileio.c: Fix several -Wmaybe-uninitialized warnings

[Pedro Alves]

 src/gdb/testsuite/gdb.base/fileio.c: In function ‘test_write’:
 src/gdb/testsuite/gdb.base/fileio.c:158:5: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
      printf ("write 1: ret = %d, errno = %d\n", ret, errno);
      ^

gdb/ChangeLog:
2017-05-18  Pedro Alves  <palves@redhat.com>

	* gdb.base/fileio.c (test_write, test_read, test_close)
	(test_fstat): Don't print 'ret' in the fail path.
---
 gdb/testsuite/gdb.base/fileio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.base/fileio.c b/gdb/testsuite/gdb.base/fileio.c
index 415f2d0..7f482a3 100644
--- a/gdb/testsuite/gdb.base/fileio.c
+++ b/gdb/testsuite/gdb.base/fileio.c
@@ -157,7 +157,7 @@ test_write (void)
       close (fd);
     }
   else
-    printf ("write 1: ret = %d, errno = %d\n", ret, errno);
+    printf ("write 1: errno = %d\n", errno);
   stop ();
   /* Write using invalid file descriptor */
   errno = 0;
@@ -177,7 +177,7 @@ test_write (void)
       close (fd);
     }
   else
-    printf ("write 3: ret = %d, errno = %d\n", ret, errno);
+    printf ("write 3: errno = %d\n", errno);
   stop ();
 }
 
@@ -203,7 +203,7 @@ test_read (void)
       close (fd);
     }
   else
-    printf ("read 1: ret = %d, errno = %d\n", ret, errno);
+    printf ("read 1: errno = %d\n", errno);
   stop ();
   /* Read using invalid file descriptor */
   errno = 0;
@@ -271,7 +271,7 @@ test_close (void)
               ret == 0 ? "OK" : "");
     }
   else
-    printf ("close 1: ret = %d, errno = %d\n", ret, errno);
+    printf ("close 1: errno = %d\n", errno);
   stop ();
   /* Close an invalid file descriptor */
   errno = 0;
@@ -337,7 +337,7 @@ test_fstat (void)
       close (fd);
     }
   else
-    printf ("fstat 1: ret = %d, errno = %d\n", ret, errno);
+    printf ("fstat 1: errno = %d\n", errno);
   stop ();
   /* Fstat using invalid file descriptor */
   errno = 0;
-- 
2.5.5

[PATCH 1/3] gdb.base/fileio.exp: Remove nowarnings

[Pedro Alves]

... and quiet -Wnonnull in a different way.

gdb/testsuite/ChangeLog:
2017-05-18  Pedro Alves  <palves@redhat.com>

	* gdb.base/fileio.c (null_str): New global.
	(test_stat): Use it.
	* gdb.base/fileio.exp: Remove nowarnings.
---
 gdb/testsuite/gdb.base/fileio.c   | 6 +++++-
 gdb/testsuite/gdb.base/fileio.exp | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/fileio.c b/gdb/testsuite/gdb.base/fileio.c
index e443173..38537db 100644
--- a/gdb/testsuite/gdb.base/fileio.c
+++ b/gdb/testsuite/gdb.base/fileio.c
@@ -76,6 +76,10 @@ static const char *strerrno (int err);
 
 static void stop () {}
 
+/* A NULL string.  We pass this to stat below instead of a NULL
+   literal to avoid -Wnonnull warnings.  */
+const char *null_str;
+
 int
 test_open ()
 {
@@ -294,7 +298,7 @@ test_stat ()
   stop ();
   /* NULL pathname */
   errno = 0;
-  ret = stat (NULL, &st);
+  ret = stat (null_str, &st);
   printf ("stat 2: ret = %d, errno = %d %s\n", ret, errno,
   	  strerrno (errno));
   stop ();
diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/fileio.exp
index 14aaa0d..6bb7141 100644
--- a/gdb/testsuite/gdb.base/fileio.exp
+++ b/gdb/testsuite/gdb.base/fileio.exp
@@ -31,7 +31,7 @@ if {[is_remote host]} {
 
 if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
 	   executable \
-	   [list debug nowarnings "additional_flags=-DOUTDIR=\"$outdir/\""]] != "" } {
+	   [list debug "additional_flags=-DOUTDIR=\"$outdir/\""]] != "" } {
     untested "failed to compile"
     return -1
 }
-- 
2.5.5

[PATCH 2/3] gdb.base/fileio.c: Fix several -Wreturn-type warnings

[Pedro Alves]

All the "test_" functions warn like:

  src/gdb/testsuite/gdb.base/fileio.c: In function ‘test_close’:
  src/gdb/testsuite/gdb.base/fileio.c:280:1: warning: control reaches end of non-void function [-Wreturn-type]
   }
   ^

Nothing looks at the return of these functions, so just make them
return void.  While at it, "()" is not the same as "(void)" in C - fix
that too.

gdb/ChangeLog:
2017-05-18  Pedro Alves  <palves@redhat.com>

	* gdb.base/fileio.c (stop, test_open, test_write, test_read)
	(test_lseek, test_close, test_stat, test_fstat, test_isatty)
	(test_system, test_rename, test_unlink, test_time): Change
	prototypes.
	* gdb.base/fileio.exp (stop_msg): Adjust.
---
 gdb/testsuite/gdb.base/fileio.c   | 50 +++++++++++++++++++--------------------
 gdb/testsuite/gdb.base/fileio.exp |  2 +-
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/gdb/testsuite/gdb.base/fileio.c b/gdb/testsuite/gdb.base/fileio.c
index 38537db..415f2d0 100644
--- a/gdb/testsuite/gdb.base/fileio.c
+++ b/gdb/testsuite/gdb.base/fileio.c
@@ -74,14 +74,14 @@ static const char *strerrno (int err);
 
 #define STRING      "Hello World"
 
-static void stop () {}
+static void stop (void) {}
 
 /* A NULL string.  We pass this to stat below instead of a NULL
    literal to avoid -Wnonnull warnings.  */
 const char *null_str;
 
-int
-test_open ()
+void
+test_open (void)
 {
   int ret;
 
@@ -140,8 +140,8 @@ test_open ()
   stop ();
 }
 
-int
-test_write ()
+void
+test_write (void)
 {
   int fd, ret;
 
@@ -181,8 +181,8 @@ test_write ()
   stop ();
 }
 
-int
-test_read ()
+void
+test_read (void)
 {
   int fd, ret;
   char buf[16];
@@ -213,8 +213,8 @@ test_read ()
   stop ();
 }
 
-int
-test_lseek ()
+void
+test_lseek (void)
 {
   int fd;
   off_t ret = 0;
@@ -255,8 +255,8 @@ test_lseek ()
   stop ();
 }
 
-int
-test_close ()
+void
+test_close (void)
 {
   int fd, ret;
 
@@ -281,8 +281,8 @@ test_close ()
   stop ();
 }
 
-int
-test_stat ()
+void
+test_stat (void)
 {
   int ret;
   struct stat st;
@@ -316,8 +316,8 @@ test_stat ()
   stop ();
 }
 
-int
-test_fstat ()
+void
+test_fstat (void)
 {
   int fd, ret;
   struct stat st;
@@ -347,8 +347,8 @@ test_fstat ()
   stop ();
 }
 
-int
-test_isatty ()
+void
+test_isatty (void)
 {
   int fd;
 
@@ -377,8 +377,8 @@ test_isatty ()
 
 char sys[1512];
 
-int
-test_system ()
+void
+test_system (void)
 {
   /*
    * Requires test framework to switch on "set remote system-call-allowed 1"
@@ -409,8 +409,8 @@ test_system ()
   stop ();
 }
 
-int
-test_rename ()
+void
+test_rename (void)
 {
   int ret;
   struct stat st;
@@ -464,8 +464,8 @@ test_rename ()
 
 char name[1256];
 
-int
-test_unlink ()
+void
+test_unlink (void)
 {
   int ret;
 
@@ -504,8 +504,8 @@ test_unlink ()
   stop ();
 }
 
-int
-test_time ()
+void
+test_time (void)
 {
   time_t ret, t;
 
diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/fileio.exp
index 6bb7141..99afaff 100644
--- a/gdb/testsuite/gdb.base/fileio.exp
+++ b/gdb/testsuite/gdb.base/fileio.exp
@@ -69,7 +69,7 @@ if ![runto_main] then {
 
 gdb_test "break stop" "Breakpoint .*$srcfile.*" 
 
-set stop_msg ".*Breakpoint .* stop \\(\\) at.*$srcfile:.*static void stop \\(\\) {}.*"
+set stop_msg ".*Breakpoint .* stop \\(\\) at.*$srcfile:.*static void stop \\(void\\) {}.*"
 
 gdb_test continue \
 "Continuing\\..*open 1:.*OK$stop_msg" \
-- 
2.5.5

Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I don't think so.  I think that'd be the equivalent of saying that we
> don't support debugging target code built with gcc versions earlier
> than 4.6.  That's a totally different discussion from the discussion
> about requirements for building gdb.

That is what I meant by "require gcc 4.6 to run testsuite".  If we use
such diagnostic in tests, we need gcc 4.6 or later to compile gdb test
cases.

> How about an even simpler fix: pass a global pointer variable that
> happens to be NULL to stat, instead of a NULL literal.  The compiler can't
> prove (without LTO) that the variable may still be NULL, so it doesn't
> warn.  Works on all compilers I tried it on: gcc 3.4/5.3/7, clang 3.7.
>
> Let me send a mini series in reply to this email, which does that,
> and also fixes a few other warnings that compiling the fileio.c with
> -Wall exposes.

These three patches are good to me.

-- 
Yao (齐尧)

Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp

[Pedro Alves]

On 05/18/2017 12:30 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> I don't think so.  I think that'd be the equivalent of saying that we
>> don't support debugging target code built with gcc versions earlier
>> than 4.6.  That's a totally different discussion from the discussion
>> about requirements for building gdb.
> 
> That is what I meant by "require gcc 4.6 to run testsuite".  If we use
> such diagnostic in tests, we need gcc 4.6 or later to compile gdb test
> cases.

Right, and I was replying to your "I am OK with this requirement."
by explaining why I don't think it's an OK requirement.

I knew that "#pragma GCC diagnostics ignored -Wwhatever" ignores
unknown warnings, and I thought that older GCCs just ignored
unknown #pragmas, but that's not actually true -- I tried it with
gcc 3, and "-Wall" makes it warn about unrecognized pragmas.

So I got confused for a while when you said the "diagnostic" was added
in gcc 4.6.  The warning (the diagnostic) is much older than gcc 4.6.
What I think was added in gcc 4.6 was "#pragma GCC diagnostic push/pop".
This means that we wrap the "#pragma GCC diagnostic ignore" under
a gcc 4.6 check, we'd still get a warning with older compilers.
Only with -Wall though, so maybe it was still OK.

BTW, what compiler are you testing with that enables -Wnonnull by default?

>> How about an even simpler fix: pass a global pointer variable that
>> happens to be NULL to stat, instead of a NULL literal.  The compiler can't
>> prove (without LTO) that the variable may still be NULL, so it doesn't
>> warn.  Works on all compilers I tried it on: gcc 3.4/5.3/7, clang 3.7.
>>
>> Let me send a mini series in reply to this email, which does that,
>> and also fixes a few other warnings that compiling the fileio.c with
>> -Wall exposes.
> 
> These three patches are good to me.

I'll push them in in a bit.

Thanks,
Pedro Alves