RFC: fix bug in compare_breakpoints

RFC: fix bug in compare_breakpoints

[Tom Tromey]

I built gdb with clang today.

This found a bunch of nits (I'll send a nit-cleanup patch later) but
also a couple real bugs.

Here's the first one.  compare_breakpoints has an invalid comparison.

Built (with clang and gcc) and regtested (gcc only) on x86-64 F16.

Tom

2012-10-17  Tom Tromey  <tromey@redhat.com>

	* breakpoint.c (compare_breakpoints): Fix comparison.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bebad75..76e3e89 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11766,7 +11766,7 @@ compare_breakpoints (const void *a, const void *b)
      the number 0.  */
   if (ua < ub)
     return -1;
-  return ub > ub ? 1 : 0;
+  return ua > ub ? 1 : 0;
 }
 
 /* Delete breakpoints by address or line.  */

Re: RFC: fix bug in compare_breakpoints

[Joel Brobecker]

> 2012-10-17  Tom Tromey  <tromey@redhat.com>
> 
> 	* breakpoint.c (compare_breakpoints): Fix comparison.

I sent a remark about this patch on IRC, but better repeat it here.
FWIW, the patch looks good to me.

-- 
Joel

Re: RFC: fix bug in compare_breakpoints

[Pedro Alves]

On 10/17/2012 08:35 PM, Tom Tromey wrote:
> I built gdb with clang today.
> 
> This found a bunch of nits (I'll send a nit-cleanup patch later) but
> also a couple real bugs.
> 
> Here's the first one.  compare_breakpoints has an invalid comparison.
> 
> Built (with clang and gcc) and regtested (gcc only) on x86-64 F16.

Eh!  Shame that gcc doesn't warn on this ("comparison always false",
I gather?).  Sounds like something that shouldn't be hard for the
compiler to detect.  IWBN to have gcc PRs for these issues.

> 
> Tom
> 
> 2012-10-17  Tom Tromey  <tromey@redhat.com>
> 
> 	* breakpoint.c (compare_breakpoints): Fix comparison.
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index bebad75..76e3e89 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -11766,7 +11766,7 @@ compare_breakpoints (const void *a, const void *b)
>       the number 0.  */
>    if (ua < ub)
>      return -1;
> -  return ub > ub ? 1 : 0;
> +  return ua > ub ? 1 : 0;
>  }
>  
>  /* Delete breakpoints by address or line.  */
> 

-- 
Pedro Alves

Re: RFC: fix bug in compare_breakpoints

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Eh!  Shame that gcc doesn't warn on this ("comparison always false",
Pedro> I gather?).  Sounds like something that shouldn't be hard for the
Pedro> compiler to detect.  IWBN to have gcc PRs for these issues.

I filed http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54979
for the comparison issue.

The other problem is caught by -Wempty-body, which we don't enable.  It
yields a number of warnings and would require a coding style change
from:

   if (blah)
     ;

to

   if (blah)
     {
     }

That would be fine with me, and I'd be happy to write the patch if there
is general agreement.

Tom

Re: RFC: fix bug in compare_breakpoints

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> 2012-10-17  Tom Tromey  <tromey@redhat.com>
>> 
>> * breakpoint.c (compare_breakpoints): Fix comparison.

Joel> I sent a remark about this patch on IRC, but better repeat it here.
Joel> FWIW, the patch looks good to me.

I'm checking it in.

Tom

Re: RFC: fix bug in compare_breakpoints

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> The other problem is caught by -Wempty-body, which we don't enable.  It
Tom> yields a number of warnings and would require a coding style change
Tom> from:
[...]

FWIW, here's the patch.  Comments appreciated.

I do think it is a mild improvement.  It is a lot easier to mistype a
";" than a correctly formatted brace pair.

I left out the rs6000-aix-tdep.c part of the patch since it is being
addressed separately.

Tom

    	* configure: Rebuild.
    	* configure.ac (build_warnings): Add -Wempty-body.
    	* m68k-tdep.c (m68k_gdbarch_init): Remove empty 'if'.
    	* remote.c (handle_notification): Use braces for empty 'else' body.
    	* s390-tdep.c (s390_analyze_prologue): Use braces for empty
    	'else' body.
    	* sh64-tdep.c (sh64_push_dummy_call): Use braces for empty
    	'else' body.
    	* solib-som.c (som_relocate_section_addresses): Use braces
    	for empty 'else' body.
    	* ui-file.c (stdio_file_write): Use braces for empty 'if' body.
    	(stdio_file_write_async_safe, stdio_file_fputs): Likewise.

diff --git a/gdb/configure.ac b/gdb/configure.ac
index fc181fd..f0b7df3 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1920,7 +1920,7 @@ build_warnings="-Wall -Wdeclaration-after-statement -Wpointer-arith \
 -Wformat-nonliteral -Wno-pointer-sign \
 -Wno-unused -Wunused-value -Wunused-function \
 -Wno-switch -Wno-char-subscripts -Wmissing-prototypes \
--Wdeclaration-after-statement"
+-Wdeclaration-after-statement -Wempty-body"
 
 # Enable -Wno-format by default when using gcc on mingw since many
 # GCC versions complain about %I64.
diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index b45a80f..0bad9ac 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -1100,9 +1100,6 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
       feature = tdesc_find_feature (info.target_desc,
 				    "org.gnu.gdb.m68k.core");
-      if (feature != NULL)
-	/* Do nothing.  */
-	;
 
       if (feature == NULL)
 	{
diff --git a/gdb/remote.c b/gdb/remote.c
index 1750bee..2ed5501 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6781,9 +6781,10 @@ handle_notification (char *buf)
 	}
     }
   else
-    /* We ignore notifications we don't recognize, for compatibility
-       with newer stubs.  */
-    ;
+    {
+      /* We ignore notifications we don't recognize, for compatibility
+	 with newer stubs.  */
+    }
 }
 
 \f
diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index 6cd6fd4..0af6f51 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -1461,13 +1461,14 @@ s390_analyze_prologue (struct gdbarch *gdbarch,
 	break;
 
       else
-        /* An instruction we don't know how to simulate.  The only
-           safe thing to do would be to set every value we're tracking
-           to 'unknown'.  Instead, we'll be optimistic: we assume that
-	   we *can* interpret every instruction that the compiler uses
-	   to manipulate any of the data we're interested in here --
-	   then we can just ignore anything else.  */
-        ;
+	{
+	  /* An instruction we don't know how to simulate.  The only
+	     safe thing to do would be to set every value we're tracking
+	     to 'unknown'.  Instead, we'll be optimistic: we assume that
+	     we *can* interpret every instruction that the compiler uses
+	     to manipulate any of the data we're interested in here --
+	     then we can just ignore anything else.  */
+	}
 
       /* Record the address after the last instruction that changed
          the FP, SP, or backlink.  Ignore instructions that changed
diff --git a/gdb/sh64-tdep.c b/gdb/sh64-tdep.c
index 4e6f262..e11e746 100644
--- a/gdb/sh64-tdep.c
+++ b/gdb/sh64-tdep.c
@@ -1175,10 +1175,10 @@ sh64_push_dummy_call (struct gdbarch *gdbarch,
 		  int_argreg ++;
 		}
 	      else 
-		;
-		/* Store it as the integers, 8 bytes at the time, if
-		   necessary spilling on the stack.  */
-	      
+		{
+		  /* Store it as the integers, 8 bytes at the time, if
+		     necessary spilling on the stack.  */
+		}
 	    }
 	    else if (len == 8)
 	      {
@@ -1202,9 +1202,10 @@ sh64_push_dummy_call (struct gdbarch *gdbarch,
 		    int_argreg ++;
 		  }
 		else
-		  ;
-		  /* Store it as the integers, 8 bytes at the time, if
-                     necessary spilling on the stack.  */
+		  {
+		    /* Store it as the integers, 8 bytes at the time, if
+		       necessary spilling on the stack.  */
+		  }
 	      }
 	}
     }
diff --git a/gdb/solib-som.c b/gdb/solib-som.c
index 6100cbe..6fea108 100644
--- a/gdb/solib-som.c
+++ b/gdb/solib-som.c
@@ -126,7 +126,9 @@ som_relocate_section_addresses (struct so_list *so,
       sec->endaddr += so->lm_info->data_start;
     }
   else
-    ;
+    {
+      /* Nothing.  */
+    }
 }
 
 
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 8528793..68089e6 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -561,7 +561,9 @@ stdio_file_write (struct ui_file *file, const char *buf, long length_buf)
 		    _("stdio_file_write: bad magic number"));
   /* Calling error crashes when we are called from the exception framework.  */
   if (fwrite (buf, length_buf, 1, stdio->file))
-    ;
+    {
+      /* Nothing.  */
+    }
 }
 
 static void
@@ -583,7 +585,9 @@ stdio_file_write_async_safe (struct ui_file *file,
      result of write (since it can be declared with attribute warn_unused_result).
      Alas casting to void doesn't work for this.  */
   if (write (stdio->fd, buf, length_buf))
-    ;
+    {
+      /* Nothing.  */
+    }
 }
 
 static void
@@ -596,7 +600,9 @@ stdio_file_fputs (const char *linebuffer, struct ui_file *file)
 		    _("stdio_file_fputs: bad magic number"));
   /* Calling error crashes when we are called from the exception framework.  */
   if (fputs (linebuffer, stdio->file))
-    ;
+    {
+      /* Nothing.  */
+    }
 }
 
 static int

Add -Wempty-body to warning set (was: Re: RFC: fix bug in compare_breakpoints)

[Pedro Alves]

[Changing $subject.  As with all warning set addition, this is likely to introduce
new -Werror build failures on other hosts/ports.]

On 10/19/2012 05:16 PM, Tom Tromey wrote:
> Tom> The other problem is caught by -Wempty-body, which we don't enable.  It
> Tom> yields a number of warnings and would require a coding style change
> Tom> from:
> [...]
> 
> FWIW, here's the patch.  Comments appreciated.
> 
> I do think it is a mild improvement.  It is a lot easier to mistype a
> ";" than a correctly formatted brace pair.

I agree it's a mild improvement.  Though I'm surprised at how few cases
in common code this reveals -- I thought we had more of:

while (...)
  ;

FAOD, this is fine with me.

Thanks.

-- 
Pedro Alves

Re: Add -Wempty-body to warning set

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> [Changing $subject.  As with all warning set addition, this is likely
Pedro> to introduce
Pedro> new -Werror build failures on other hosts/ports.]

Thanks.

Pedro> I agree it's a mild improvement.  Though I'm surprised at how few cases
Pedro> in common code this reveals -- I thought we had more of:
Pedro> while (...)
Pedro>   ;

I was surprised, too, but then I read that 'while' is explicitly
excluded from the warning:

    `-Wempty-body'
         Warn if an empty body occurs in an `if', `else' or `do while'
         statement.  This warning is also enabled by `-Wextra'.

Pedro> FAOD, this is fine with me.

Thanks.  I'm going to wait for more comments.

Tom

Re: RFC: fix bug in compare_breakpoints

[Joel Brobecker]

> I do think it is a mild improvement.  It is a lot easier to mistype a
> ";" than a correctly formatted brace pair.

Thanks, Tom. I also think it's a mild improvement.

It does make we wonder why we need the if/else at all in some of
the cases, but that's for another day/patch...

-- 
Joel

Re: RFC: fix bug in compare_breakpoints

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> It does make we wonder why we need the if/else at all in some of
Joel> the cases, but that's for another day/patch...

Yeah, some of them seem useless.
A few are just to stick a comment there, and some others are to work
around warn_unused_result.

Tom

Re: RFC: fix bug in compare_breakpoints

[Joel Brobecker]

> A few are just to stick a comment there, and some others are to work
> around warn_unused_result.

Yeah. In the first case, we could move the comment back to the top,
although perhaps it's easier to read an understand the current way.
I also saw the ones around "warn_unused_result" and we could have
used a cast to (void), I suppose. But in the end, all of this isn't
really all that important. Gives us an opportunity to request a comment
explaining why it's OK to ignore the return value :-).

-- 
Joel

Re: RFC: fix bug in compare_breakpoints

[Pedro Alves]

On 10/19/2012 06:12 PM, Joel Brobecker wrote:
>> A few are just to stick a comment there, and some others are to work
>> around warn_unused_result.
> 
> Yeah. In the first case, we could move the comment back to the top,
> although perhaps it's easier to read an understand the current way.
> I also saw the ones around "warn_unused_result" and we could have
> used a cast to (void), I suppose. 

Last I wrote one of those, a cast to (void) wasn't good enough to silence
the warning.

> But in the end, all of this isn't
> really all that important. Gives us an opportunity to request a comment
> explaining why it's OK to ignore the return value :-).

Indeed.  :-)

-- 
Pedro Alves

Re: RFC: fix bug in compare_breakpoints

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> The other problem is caught by -Wempty-body, which we don't enable.  It
Tom> yields a number of warnings and would require a coding style change
Tom> from:
Tom> [...]

Tom> FWIW, here's the patch.  Comments appreciated.

It's been a couple of weeks and there haven't been any objections.
I am checking this in.

Tom