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

  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(-)
  

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

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.
  

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
  

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.
  

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