[patch] cleanup: Wunused corefile.c

[patch] cleanup: Wunused corefile.c

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

Hello,

In addition to already posted/committed Wunused I have a bunch of 
patches which work around unused vars by adding attribute unused.

Rationale was: either it was unclear whether the assignment might have 
side-effects or there was something that should have been done (e.g. 
this case) with the variable.

Let me know if this is acceptable approach.

Thank you,

Aleksandar Ristovski
QNX Software Systems


ChangeLog:

         * corefile.c (reopen_exec_file): Add unused attribute to res,
         add FIXME comment for future cleanup.

[-- Attachment #2: Wunused-corefile-201301311429.patch --]
[-- Type: text/x-patch, Size: 755 bytes --]

Index: gdb/corefile.c
===================================================================
RCS file: /cvs/src/src/gdb/corefile.c,v
retrieving revision 1.71
diff -u -p -r1.71 corefile.c
--- gdb/corefile.c	14 Jan 2013 21:03:54 -0000	1.71
+++ gdb/corefile.c	30 Jan 2013 22:25:14 -0000
@@ -136,7 +136,7 @@ void
 reopen_exec_file (void)
 {
   char *filename;
-  int res;
+  int res __attribute__ ((unused));
   struct stat st;
   struct cleanup *cleanups;
 
@@ -149,6 +149,8 @@ reopen_exec_file (void)
   cleanups = make_cleanup (xfree, filename);
   res = stat (filename, &st);
 
+  /* FIXME: error checking using 'res' (and remove attribute unused).  */
+
   if (exec_bfd_mtime && exec_bfd_mtime != st.st_mtime)
     exec_file_attach (filename, 0);
   else

Re: [patch] cleanup: Wunused corefile.c

[Tom Tromey]

>>>>> "Aleksandar" == Aleksandar Ristovski <aristovski@qnx.com> writes:

Aleksandar> In addition to already posted/committed Wunused I have a bunch of
Aleksandar> patches which work around unused vars by adding attribute unused.

You should probably try a --enable-targets=all build if you want to
enable -Wunused in configure...

Aleksandar> Rationale was: either it was unclear whether the assignment
Aleksandar> might have side-effects or there was something that should
Aleksandar> have been done (e.g. this case) with the variable.

Aleksandar> Let me know if this is acceptable approach.

It definitely isn't in this form, and perhaps not in other forms.

First, in this case, the is just buggy.  If stat fails, then st.st_mtime
isn't necessarily set, and so the subsequent test is reading garbage.
This is the sort of bug that -Wunused helps to diagnose -- so adding an
attribute to silence the error isn't what we should do.

Second, using __attribute__ unconditionally isn't ok.  We can use the
ATTRIBUTE_UNUSED macro; but I would imagine in most cases it would be
better to fix the problem in some other way.

Tom

Re: [patch] cleanup: Wunused corefile.c

[Aleksandar Ristovski]

On 13-01-31 03:24 PM, Tom Tromey wrote:
>>>>>> "Aleksandar" == Aleksandar Ristovski <aristovski@qnx.com> writes:
>
> Aleksandar> In addition to already posted/committed Wunused I have a bunch of
> Aleksandar> patches which work around unused vars by adding attribute unused.
>
> You should probably try a --enable-targets=all build if you want to
> enable -Wunused in configure...
>
> Aleksandar> Rationale was: either it was unclear whether the assignment
> Aleksandar> might have side-effects or there was something that should
> Aleksandar> have been done (e.g. this case) with the variable.
>
> Aleksandar> Let me know if this is acceptable approach.
>
> It definitely isn't in this form, and perhaps not in other forms.
>
> First, in this case, the is just buggy.  If stat fails, then st.st_mtime
> isn't necessarily set, and so the subsequent test is reading garbage.
> This is the sort of bug that -Wunused helps to diagnose -- so adding an
> attribute to silence the error isn't what we should do.
>
> Second, using __attribute__ unconditionally isn't ok.  We can use the
> ATTRIBUTE_UNUSED macro; but I would imagine in most cases it would be
> better to fix the problem in some other way.

This is why I put a FIXME - intending to revisit and properly fix.

But there are other cases where it is not about missing code, but e.g. 
conditional compilation. For example:

Index: gdb/inflow.c
===================================================================
RCS file: /cvs/src/src/gdb/inflow.c,v
retrieving revision 1.69
diff -u -p -r1.69 inflow.c
--- gdb/inflow.c        1 Jan 2013 06:32:45 -0000       1.69
+++ gdb/inflow.c        30 Jan 2013 22:25:15 -0000
@@ -397,7 +397,7 @@ terminal_ours_1 (int output_only)
           pgrp.  */
        void (*osigttou) () = NULL;
  #endif
-      int result;
+      int result __attribute__ ((unused));

  #ifdef SIGTTOU
        if (job_control)


However, I do not intend to push this very hard due to lack of time. I 
simply did a swipe over the code and made it compile with Wunused 
(without regressions) on x86_64-linux-gnu and wanted to contribute as 
much as I can without spending too much time on it.


Thanks again,

Aleksandar

Re: [patch] cleanup: Wunused corefile.c

[Tom Tromey]

>>>>> "Aleksandar" == Aleksandar Ristovski <aristovski@qnx.com> writes:

Aleksandar> This is why I put a FIXME - intending to revisit and properly fix.

FIXME comments are just ignored in practice.  IME, nobody ever fixes
them except as a side effect of what they were intending to do.

Aleksandar> But there are other cases where it is not about missing code, but
Aleksandar> e.g. conditional compilation. For example:

Yeah, in that case I am less sure.
Making the definition conditional might be ok.
Or it might be uglier than the occasional ATTRIBUTE_UNUSED.
I guess it depends on the case.

Aleksandar> However, I do not intend to push this very hard due to lack
Aleksandar> of time. I simply did a swipe over the code and made it
Aleksandar> compile with Wunused (without regressions) on
Aleksandar> x86_64-linux-gnu and wanted to contribute as much as I can
Aleksandar> without spending too much time on it.

I understand, but I think the hard part of this work is also the most
useful part.  What I mean is that it is certainly valuable to get all
the simple cases fixed; but going through the trickier cases and writing
proper fixes is the real benefit of enabling this warning -- finding and
fixing real bugs.  Making these warnings disappear is contrary to that.

Tom

Re: [patch] cleanup: Wunused corefile.c

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On 13-02-01 02:25 PM, Tom Tromey wrote:
>
> I understand, but I think the hard part of this work is also the most
> useful part.  What I mean is that it is certainly valuable to get all
> the simple cases fixed; but going through the trickier cases and writing
> proper fixes is the real benefit of enabling this warning -- finding and
> fixing real bugs.  Making these warnings disappear is contrary to that.
>

I agree and I don't agree. I'm not shy from hard work, but I simply have 
limited time resources for contributing back to FSF.

I really believe making warnings disappear and turning on Wunused by 
default would make a big difference worth "obscuring" a bug or two (they 
are already obscure as we are not aware of them). Once Wunused is on by 
default, there will not be creeping in of unused stuff any more.


But here is revised patch for corefile.c. Not that it represents any 
hard work or anything, but since I was at...


Thanks,

Aleksandar Ristovski
QNX Software Systems



ChangeLog:
  Aleksandar Ristovski  <aristovski@qnx.com>

         * corefile.c (reopen_exec_file): Check return value from stat.


[-- Attachment #2: Wunused-corefile-201302010947.patch --]
[-- Type: text/x-patch, Size: 767 bytes --]

Index: gdb/corefile.c
===================================================================
RCS file: /cvs/src/src/gdb/corefile.c,v
retrieving revision 1.71
diff -u -p -r1.71 corefile.c
--- gdb/corefile.c	14 Jan 2013 21:03:54 -0000	1.71
+++ gdb/corefile.c	1 Feb 2013 15:42:17 -0000
@@ -149,7 +149,11 @@ reopen_exec_file (void)
   cleanups = make_cleanup (xfree, filename);
   res = stat (filename, &st);
 
-  if (exec_bfd_mtime && exec_bfd_mtime != st.st_mtime)
+  if (res != 0 && info_verbose)
+    warning (_("File %s could not be stat-ed (%s)\n"), filename,
+	     strerror (res));
+
+  if (res == 0 && exec_bfd_mtime && exec_bfd_mtime != st.st_mtime)
     exec_file_attach (filename, 0);
   else
     /* If we accessed the file since last opening it, close it now;

Re: [patch] cleanup: Wunused corefile.c

[Aleksandar Ristovski]

On 13-02-01 04:31 PM, Aleksandar Ristovski wrote:
> On 13-02-01 02:25 PM, Tom Tromey wrote:
>>
>> I understand, but I think the hard part of this work is also the most
>> useful part.  What I mean is that it is certainly valuable to get all
>> the simple cases fixed; but going through the trickier cases and writing
>> proper fixes is the real benefit of enabling this warning -- finding and
>> fixing real bugs.  Making these warnings disappear is contrary to that.
>>
>
> I agree and I don't agree. I'm not shy from hard work, but I simply have
> limited time resources for contributing back to FSF.
>
> I really believe making warnings disappear and turning on Wunused by
> default would make a big difference worth "obscuring" a bug or two (they
> are already obscure as we are not aware of them). Once Wunused is on by
> default, there will not be creeping in of unused stuff any more.
>
>
> But here is revised patch for corefile.c. Not that it represents any
> hard work or anything, but since I was at...
>
>
> Thanks,
>
> Aleksandar Ristovski
> QNX Software Systems
>
>
>
> ChangeLog:
>   Aleksandar Ristovski  <aristovski@qnx.com>
>
>          * corefile.c (reopen_exec_file): Check return value from stat.
>


Sorry it should have been errno for strerror not res:

Index: gdb/corefile.c
===================================================================
RCS file: /cvs/src/src/gdb/corefile.c,v
retrieving revision 1.71
diff -u -p -r1.71 corefile.c
--- gdb/corefile.c      14 Jan 2013 21:03:54 -0000      1.71
+++ gdb/corefile.c      1 Feb 2013 21:48:42 -0000
@@ -149,7 +149,11 @@ reopen_exec_file (void)
    cleanups = make_cleanup (xfree, filename);
    res = stat (filename, &st);

-  if (exec_bfd_mtime && exec_bfd_mtime != st.st_mtime)
+  if (res != 0 && info_verbose)
+    warning (_("File %s could not be stat-ed (%s)\n"), filename,
+            strerror (errno));
+
+  if (res == 0 && exec_bfd_mtime && exec_bfd_mtime != st.st_mtime)
      exec_file_attach (filename, 0);
    else
      /* If we accessed the file since last opening it, close it now;

Re: [patch] cleanup: Wunused corefile.c

[Sergio Durigan Junior]

On Friday, February 01 2013, Aleksandar Ristovski wrote:

> Index: gdb/corefile.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/corefile.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 corefile.c
> --- gdb/corefile.c      14 Jan 2013 21:03:54 -0000      1.71
> +++ gdb/corefile.c      1 Feb 2013 21:48:42 -0000
> @@ -149,7 +149,11 @@ reopen_exec_file (void)
>    cleanups = make_cleanup (xfree, filename);
>    res = stat (filename, &st);
>
> -  if (exec_bfd_mtime && exec_bfd_mtime != st.st_mtime)
> +  if (res != 0 && info_verbose)
> +    warning (_("File %s could not be stat-ed (%s)\n"), filename,
> +            strerror (errno));

Thanks for that.  GDB uses `safe_strerror' instead of `strerror'.

-- 
Sergio