* [PATCH OBV] Add nowarnings in gdb.base/fileio.exp
@ 2017-05-17 13:46 Yao Qi
2017-05-17 14:17 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Yao Qi @ 2017-05-17 13:46 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp
2017-05-17 13:46 [PATCH OBV] Add nowarnings in gdb.base/fileio.exp Yao Qi
@ 2017-05-17 14:17 ` Pedro Alves
2017-05-18 8:34 ` Yao Qi
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2017-05-17 14:17 UTC (permalink / raw)
To: Yao Qi, gdb-patches
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
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp
2017-05-17 14:17 ` Pedro Alves
@ 2017-05-18 8:34 ` Yao Qi
2017-05-18 11:06 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Yao Qi @ 2017-05-18 8:34 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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 (齐尧)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp
2017-05-18 8:34 ` Yao Qi
@ 2017-05-18 11:06 ` Pedro Alves
2017-05-18 11:09 ` [PATCH 1/3] gdb.base/fileio.exp: Remove nowarnings Pedro Alves
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Pedro Alves @ 2017-05-18 11:06 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] gdb.base/fileio.c: Fix several -Wmaybe-uninitialized warnings
2017-05-18 11:06 ` Pedro Alves
2017-05-18 11:09 ` [PATCH 1/3] gdb.base/fileio.exp: Remove nowarnings Pedro Alves
2017-05-18 11:09 ` [PATCH 2/3] gdb.base/fileio.c: Fix several -Wreturn-type warnings Pedro Alves
@ 2017-05-18 11:09 ` Pedro Alves
2017-05-18 11:30 ` [PATCH OBV] Add nowarnings in gdb.base/fileio.exp Yao Qi
3 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-05-18 11:09 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] gdb.base/fileio.exp: Remove nowarnings
2017-05-18 11:06 ` Pedro Alves
@ 2017-05-18 11:09 ` Pedro Alves
2017-05-18 11:09 ` [PATCH 2/3] gdb.base/fileio.c: Fix several -Wreturn-type warnings Pedro Alves
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-05-18 11:09 UTC (permalink / raw)
To: gdb-patches
... 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] gdb.base/fileio.c: Fix several -Wreturn-type warnings
2017-05-18 11:06 ` Pedro Alves
2017-05-18 11:09 ` [PATCH 1/3] gdb.base/fileio.exp: Remove nowarnings Pedro Alves
@ 2017-05-18 11:09 ` Pedro Alves
2017-05-18 11:09 ` [PATCH 3/3] gdb.base/fileio.c: Fix several -Wmaybe-uninitialized warnings Pedro Alves
2017-05-18 11:30 ` [PATCH OBV] Add nowarnings in gdb.base/fileio.exp Yao Qi
3 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-05-18 11:09 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp
2017-05-18 11:06 ` Pedro Alves
` (2 preceding siblings ...)
2017-05-18 11:09 ` [PATCH 3/3] gdb.base/fileio.c: Fix several -Wmaybe-uninitialized warnings Pedro Alves
@ 2017-05-18 11:30 ` Yao Qi
2017-05-18 11:51 ` Pedro Alves
3 siblings, 1 reply; 9+ messages in thread
From: Yao Qi @ 2017-05-18 11:30 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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 (齐尧)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH OBV] Add nowarnings in gdb.base/fileio.exp
2017-05-18 11:30 ` [PATCH OBV] Add nowarnings in gdb.base/fileio.exp Yao Qi
@ 2017-05-18 11:51 ` Pedro Alves
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-05-18 11:51 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-05-18 11:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-17 13:46 [PATCH OBV] Add nowarnings in gdb.base/fileio.exp Yao Qi
2017-05-17 14:17 ` Pedro Alves
2017-05-18 8:34 ` Yao Qi
2017-05-18 11:06 ` Pedro Alves
2017-05-18 11:09 ` [PATCH 1/3] gdb.base/fileio.exp: Remove nowarnings Pedro Alves
2017-05-18 11:09 ` [PATCH 2/3] gdb.base/fileio.c: Fix several -Wreturn-type warnings Pedro Alves
2017-05-18 11:09 ` [PATCH 3/3] gdb.base/fileio.c: Fix several -Wmaybe-uninitialized warnings Pedro Alves
2017-05-18 11:30 ` [PATCH OBV] Add nowarnings in gdb.base/fileio.exp Yao Qi
2017-05-18 11:51 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).