[Patch]: Little Cleanup

[Patch]: Little Cleanup

[Markus Deuling]

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

Hi,

this patch is a little cleanup for breakpoint.c/infrun.c.

Ok to commit ?


ChangeLog:

          * infrun.c (breakpoints_failed): Remove unnecessary variable.
          (handle_inferior_event): Remove unnecessary braces.
          * breakpoint.c (bpstat_what): Remove wrong comment.


-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com



[-- Attachment #2: cleanup_patch --]
[-- Type: text/plain, Size: 1854 bytes --]

diff -urN src/gdb/breakpoint.c dev/gdb/breakpoint.c
--- src/gdb/breakpoint.c	2007-02-27 20:46:04.000000000 +0100
+++ dev/gdb/breakpoint.c	2007-03-02 07:29:06.000000000 +0100
@@ -3134,8 +3134,6 @@
   /* step_resume entries: a step resume breakpoint overrides another
      breakpoint of signal handling (see comment in wait_for_inferior
      at where we set the step_resume breakpoint).  */
-  /* We handle the through_sigtramp_breakpoint the same way; having both
-     one of those and a step_resume_breakpoint is probably very rare (?).  */
 
   static const enum bpstat_what_main_action
     table[(int) class_last][(int) BPSTAT_WHAT_LAST] =
diff -urN src/gdb/infrun.c dev/gdb/infrun.c
--- src/gdb/infrun.c	2007-03-01 06:45:40.000000000 +0100
+++ dev/gdb/infrun.c	2007-03-02 07:28:17.000000000 +0100
@@ -288,10 +288,6 @@
 
 struct regcache *stop_registers;
 
-/* Nonzero if program stopped due to error trying to insert breakpoints.  */
-
-static int breakpoints_failed;
-
 /* Nonzero after stop if current stack frame should be printed.  */
 
 static int stop_print_frame;
@@ -1830,7 +1826,6 @@
   stop_print_frame = 1;
   ecs->random_signal = 0;
   stopped_by_random_signal = 0;
-  breakpoints_failed = 0;
 
   if (stop_signal == TARGET_SIGNAL_TRAP
       && trap_expected
@@ -2126,9 +2121,7 @@
         if (debug_infrun)
 	  fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
 	if (breakpoints_inserted)
-	  {
-	    remove_breakpoints ();
-	  }
+	  remove_breakpoints ();
 	breakpoints_inserted = 0;
 	ecs->another_trap = 1;
 	/* Still need to check other stuff, at least the case
@@ -2909,8 +2902,7 @@
 
       if (!breakpoints_inserted && !ecs->another_trap)
 	{
-	  breakpoints_failed = insert_breakpoints ();
-	  if (breakpoints_failed)
+          if (insert_breakpoints ())
 	    {
 	      stop_stepping (ecs);
 	      return;

Re: [Patch]: Little Cleanup

[Pedro Alves]

Markus Deuling wrote:
> @@ -2909,8 +2902,7 @@
>  
>        if (!breakpoints_inserted && !ecs->another_trap)
>  	{
> -	  breakpoints_failed = insert_breakpoints ();
> -	  if (breakpoints_failed)
> +          if (insert_breakpoints ())
>  	    {
>  	      stop_stepping (ecs);
>  	      return;
>
>   

Actually, I find your version harder to read for someone not knowing the 
insert_breakpoints
API by heart.  To me, it looks like you are testing a function that 
returns a boolean.
I read it as: "if insert_breakpoints succeeded".  It looks like the de 
facto standard is to use:

if (insert_breakpoints () != 0)

The current version doesn't do it either, but 'breakpoints_failed'
gives a pretty good hint.
It may actually by a GNU coding convention, but I'm not sure.

(Just being picky on a Saturday morning, ignore at will.)

Cheers,
Pedro Alves

Re: [Patch]: Little Cleanup

[Eli Zaretskii]

> Date: Sat, 03 Mar 2007 09:07:49 +0000
> From: Pedro Alves <pedro_alves@portugalmail.pt>
> CC: GDB Patches <gdb-patches@sourceware.org>
> 
> Markus Deuling wrote:
> > @@ -2909,8 +2902,7 @@
> >  
> >        if (!breakpoints_inserted && !ecs->another_trap)
> >  	{
> > -	  breakpoints_failed = insert_breakpoints ();
> > -	  if (breakpoints_failed)
> > +          if (insert_breakpoints ())
> >  	    {
> >  	      stop_stepping (ecs);
> >  	      return;
> >
> >   
> 
> Actually, I find your version harder to read for someone not knowing the 
> insert_breakpoints API by heart.

FWIW, I agree.  The new version requires a comment to be as readable
as the old one.

(Of course, I'd expect the optimizer to produce the same code from
both old and new versions.)

Re: [Patch]: Little Cleanup

[Markus Deuling]

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

Hi,

thank you for your comment.

Eli Zaretskii wrote:
>>
>> Actually, I find your version harder to read for someone not knowing the 
>> insert_breakpoints API by heart.
>>     
>
> FWIW, I agree.  The new version requires a comment to be as readable
> as the old one.
>
> (Of course, I'd expect the optimizer to produce the same code from
> both old and new versions.)
I agree.

Comment in insert_breakpoints() says:
"Both return zero if successful,   or an `errno' value if ..." so
"if (insert_breakpoints () != 0)" is more clear and precise.

I changed the patch.  Ok ?

ChangeLog:

         * infrun.c (breakpoints_failed): Remove unnecessary variable.
         (handle_inferior_event): Remove unnecessary braces.
         * breakpoint.c (bpstat_what): Remove wrong comment.



-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com



[-- Attachment #2: cleanup_patch --]
[-- Type: text/plain, Size: 1858 bytes --]

diff -urN src/gdb/breakpoint.c dev/gdb/breakpoint.c
--- src/gdb/breakpoint.c	2007-02-27 20:46:04.000000000 +0100
+++ dev/gdb/breakpoint.c	2007-03-02 07:29:06.000000000 +0100
@@ -3134,8 +3134,6 @@
   /* step_resume entries: a step resume breakpoint overrides another
      breakpoint of signal handling (see comment in wait_for_inferior
      at where we set the step_resume breakpoint).  */
-  /* We handle the through_sigtramp_breakpoint the same way; having both
-     one of those and a step_resume_breakpoint is probably very rare (?).  */
 
   static const enum bpstat_what_main_action
     table[(int) class_last][(int) BPSTAT_WHAT_LAST] =
diff -urN src/gdb/infrun.c dev/gdb/infrun.c
--- src/gdb/infrun.c	2007-03-01 06:45:40.000000000 +0100
+++ dev/gdb/infrun.c	2007-03-02 07:28:17.000000000 +0100
@@ -288,10 +288,6 @@
 
 struct regcache *stop_registers;
 
-/* Nonzero if program stopped due to error trying to insert breakpoints.  */
-
-static int breakpoints_failed;
-
 /* Nonzero after stop if current stack frame should be printed.  */
 
 static int stop_print_frame;
@@ -1830,7 +1826,6 @@
   stop_print_frame = 1;
   ecs->random_signal = 0;
   stopped_by_random_signal = 0;
-  breakpoints_failed = 0;
 
   if (stop_signal == TARGET_SIGNAL_TRAP
       && trap_expected
@@ -2126,9 +2121,7 @@
         if (debug_infrun)
 	  fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
 	if (breakpoints_inserted)
-	  {
-	    remove_breakpoints ();
-	  }
+	  remove_breakpoints ();
 	breakpoints_inserted = 0;
 	ecs->another_trap = 1;
 	/* Still need to check other stuff, at least the case
@@ -2909,8 +2902,7 @@
 
       if (!breakpoints_inserted && !ecs->another_trap)
 	{
-	  breakpoints_failed = insert_breakpoints ();
-	  if (breakpoints_failed)
+          if (insert_breakpoints () != 0)
 	    {
 	      stop_stepping (ecs);
 	      return;

Re: [Patch]: Little Cleanup

[Eli Zaretskii]

> Date: Mon, 05 Mar 2007 06:57:50 +0100
> From: Markus Deuling <deuling@de.ibm.com>
> CC: Pedro Alves <pedro_alves@portugalmail.pt>, gdb-patches@sourceware.org
> 
>        if (!breakpoints_inserted && !ecs->another_trap)
>  	{
> -	  breakpoints_failed = insert_breakpoints ();
> -	  if (breakpoints_failed)
> +          if (insert_breakpoints () != 0)
>  	    {
>  	      stop_stepping (ecs);
>  	      return;

I thought we agreed about adding a comment here, something like:

  /* insert_breakpoints returns non-zero if it fails to insert the
     breakpoints.  */

Re: [Patch]: Little Cleanup

[Mark Kettenis]

> Date: Mon, 05 Mar 2007 23:04:48 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Mon, 05 Mar 2007 06:57:50 +0100
> > From: Markus Deuling <deuling@de.ibm.com>
> > CC: Pedro Alves <pedro_alves@portugalmail.pt>, gdb-patches@sourceware.org
> > 
> >        if (!breakpoints_inserted && !ecs->another_trap)
> >  	{
> > -	  breakpoints_failed = insert_breakpoints ();
> > -	  if (breakpoints_failed)
> > +          if (insert_breakpoints () != 0)
> >  	    {
> >  	      stop_stepping (ecs);
> >  	      return;
> 
> I thought we agreed about adding a comment here, something like:
> 
>   /* insert_breakpoints returns non-zero if it fails to insert the
>      breakpoints.  */

Adding this sort of comments at all call sites of such functions is
really silly.

Mark

Re: [Patch]: Little Cleanup

[Eli Zaretskii]

> Date: Mon, 5 Mar 2007 22:19:20 +0100 (CET)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: deuling@de.ibm.com, pedro_alves@portugalmail.pt,
>         gdb-patches@sourceware.org
> > 
> > I thought we agreed about adding a comment here, something like:
> > 
> >   /* insert_breakpoints returns non-zero if it fails to insert the
> >      breakpoints.  */
> 
> Adding this sort of comments at all call sites of such functions is
> really silly.

I don't see anything silly about making the code clearer.

Re: [Patch]: Little Cleanup

[Jim Blandy]

Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Mon, 5 Mar 2007 22:19:20 +0100 (CET)
>> From: Mark Kettenis <mark.kettenis@xs4all.nl>
>> CC: deuling@de.ibm.com, pedro_alves@portugalmail.pt,
>>         gdb-patches@sourceware.org
>> > 
>> > I thought we agreed about adding a comment here, something like:
>> > 
>> >   /* insert_breakpoints returns non-zero if it fails to insert the
>> >      breakpoints.  */
>> 
>> Adding this sort of comments at all call sites of such functions is
>> really silly.
>
> I don't see anything silly about making the code clearer.

I'm confident Mark believes clarifying code is not silly.  I think
Mark disagrees that the comment you requested is, in fact, a
clarification.

I tend to agree with Mark, here.  The place to document the meaning of
a function's return value is at the function, not at each of its call
sites.

Re: [Patch]: Little Cleanup

[Eli Zaretskii]

> Cc: Mark Kettenis <mark.kettenis@xs4all.nl>,  deuling@de.ibm.com,
> 	  pedro_alves@portugalmail.pt,  gdb-patches@sourceware.org
> From: Jim Blandy <jimb@codesourcery.com>
> Date: Mon, 05 Mar 2007 14:01:18 -0800
> 
> The place to document the meaning of a function's return value is at
> the function, not at each of its call sites.

So you are saying that it's normal to expect the code reader to
constantly jump to the function's definition trying to understand what
its callers try to accomplish?

Re: [Patch]: Little Cleanup

[Joel Brobecker]

> > The place to document the meaning of a function's return value is at
> > the function, not at each of its call sites.
> 
> So you are saying that it's normal to expect the code reader to
> constantly jump to the function's definition trying to understand what
> its callers try to accomplish?

Then perhaps this suggests that the function could be renamed into
something clearer? But otherwise, yes, I agree with Jim and Mark,
because maintaining these comments everywhere is going to be an issue.

-- 
Joel

Re: [Patch]: Little Cleanup

[Michael Snyder]

On Tue, 2007-03-06 at 11:52 -0800, Joel Brobecker wrote:
> > > The place to document the meaning of a function's return value is at
> > > the function, not at each of its call sites.
> > 
> > So you are saying that it's normal to expect the code reader to
> > constantly jump to the function's definition trying to understand what
> > its callers try to accomplish?
> 
> Then perhaps this suggests that the function could be renamed into
> something clearer? But otherwise, yes, I agree with Jim and Mark,
> because maintaining these comments everywhere is going to be an issue.

Yes.  The logical place for a function to be documented is
at the definition -- not at the call.

Re: [Patch]: Little Cleanup

[Eli Zaretskii]

> Date: Tue, 6 Mar 2007 11:52:11 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Jim Blandy <jimb@codesourcery.com>, mark.kettenis@xs4all.nl,
> 	deuling@de.ibm.com, pedro_alves@portugalmail.pt,
> 	gdb-patches@sourceware.org
> 
> Then perhaps this suggests that the function could be renamed into
> something clearer?

Fine with me.  Note that the original code had a local variable that
made this clear; this whole discussion was caused by a patch that
removed that variable.

> But otherwise, yes, I agree with Jim and Mark,
> because maintaining these comments everywhere is going to be an issue.

I never said anything about ``maintaining these comments everywhere''.
There's no need for such generalizations.

Re: [Patch]: Little Cleanup

[Ulrich Weigand]

Eli Zaretskii wrote:

> Fine with me.  Note that the original code had a local variable that
> made this clear; this whole discussion was caused by a patch that
> removed that variable.

Actually, the original code had a *global* static variable that was
used only at this single location -- cleaning this up was really the
point of the patch, I guess.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [Patch]: Little Cleanup

[Markus Deuling]

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

Ulrich Weigand wrote:
> Eli Zaretskii wrote:
>
>   
>> Fine with me.  Note that the original code had a local variable that
>> made this clear; this whole discussion was caused by a patch that
>> removed that variable.
>>     
>
> Actually, the original code had a *global* static variable that was
> used only at this single location -- cleaning this up was really the
> point of the patch, I guess.
>
> Bye,
> Ulrich
>
>   
The function "handle_inferior_event"'s length is >1450 (!!!) lines of 
code and I think its
a good idea to clean it up if possible. The patch removes an unnecessary
variable from that function. To increase the readability I added a 
comment to the patch.

Ok?

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com



[-- Attachment #2: cleanup_patch_1 --]
[-- Type: text/plain, Size: 1943 bytes --]

diff -urN src/gdb/breakpoint.c dev/gdb/breakpoint.c
--- src/gdb/breakpoint.c	2007-02-27 20:46:04.000000000 +0100
+++ dev/gdb/breakpoint.c	2007-03-07 08:23:06.000000000 +0100
@@ -3134,8 +3134,6 @@
   /* step_resume entries: a step resume breakpoint overrides another
      breakpoint of signal handling (see comment in wait_for_inferior
      at where we set the step_resume breakpoint).  */
-  /* We handle the through_sigtramp_breakpoint the same way; having both
-     one of those and a step_resume_breakpoint is probably very rare (?).  */
 
   static const enum bpstat_what_main_action
     table[(int) class_last][(int) BPSTAT_WHAT_LAST] =
diff -urN src/gdb/infrun.c dev/gdb/infrun.c
--- src/gdb/infrun.c	2007-03-01 06:45:40.000000000 +0100
+++ dev/gdb/infrun.c	2007-03-07 08:24:33.000000000 +0100
@@ -288,10 +288,6 @@
 
 struct regcache *stop_registers;
 
-/* Nonzero if program stopped due to error trying to insert breakpoints.  */
-
-static int breakpoints_failed;
-
 /* Nonzero after stop if current stack frame should be printed.  */
 
 static int stop_print_frame;
@@ -1830,7 +1826,6 @@
   stop_print_frame = 1;
   ecs->random_signal = 0;
   stopped_by_random_signal = 0;
-  breakpoints_failed = 0;
 
   if (stop_signal == TARGET_SIGNAL_TRAP
       && trap_expected
@@ -2126,9 +2121,7 @@
         if (debug_infrun)
 	  fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
 	if (breakpoints_inserted)
-	  {
-	    remove_breakpoints ();
-	  }
+	  remove_breakpoints ();
 	breakpoints_inserted = 0;
 	ecs->another_trap = 1;
 	/* Still need to check other stuff, at least the case
@@ -2909,8 +2902,9 @@
 
       if (!breakpoints_inserted && !ecs->another_trap)
 	{
-	  breakpoints_failed = insert_breakpoints ();
-	  if (breakpoints_failed)
+          /* Stop stepping when inserting breakpoints
+             has failed.  */
+          if (insert_breakpoints () != 0)
 	    {
 	      stop_stepping (ecs);
 	      return;

Re: [Patch]: Little Cleanup

[Daniel Jacobowitz]

On Wed, Mar 07, 2007 at 08:34:57AM +0100, Markus Deuling wrote:
> -	  breakpoints_failed = insert_breakpoints ();
> -	  if (breakpoints_failed)
> +          /* Stop stepping when inserting breakpoints
> +             has failed.  */
> +          if (insert_breakpoints () != 0)

If you check this in, please fix the whitespace (tabs versus spaces).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [Patch]: Little Cleanup

[Eli Zaretskii]

> Date: Wed, 07 Mar 2007 08:34:57 +0100
> From: Markus Deuling <deuling@de.ibm.com>
> CC: eliz@gnu.org, Joel Brobecker <brobecker@adacore.com>,
>         jimb@codesourcery.com, mark.kettenis@xs4all.nl, deuling@de.ibm.com,
>         pedro_alves@portugalmail.pt, Ulrich Weigand <uweigand@de.ibm.com>
> >   
> The function "handle_inferior_event"'s length is >1450 (!!!) lines of 
> code and I think its
> a good idea to clean it up if possible. The patch removes an unnecessary
> variable from that function. To increase the readability I added a 
> comment to the patch.
> 
> Ok?

Fine with me, thanks.

Re: [Patch]: Little Cleanup

[Markus Deuling]

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

Eli Zaretskii wrote:
>>
>> Ok?
> 
> Fine with me, thanks.
> 

Ok,
this is the version with comments and tabs.

ChangeLog:

         * infrun.c (breakpoints_failed): Remove unnecessary variable.
         (handle_inferior_event): Remove unnecessary braces.
         * breakpoint.c (bpstat_what): Remove wrong comme


-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com


[-- Attachment #2: cleanup_patch --]
[-- Type: text/plain, Size: 1922 bytes --]

diff -urN src/gdb/breakpoint.c dev/gdb/breakpoint.c
--- src/gdb/breakpoint.c	2007-02-27 20:46:04.000000000 +0100
+++ dev/gdb/breakpoint.c	2007-03-07 08:23:06.000000000 +0100
@@ -3134,8 +3134,6 @@
   /* step_resume entries: a step resume breakpoint overrides another
      breakpoint of signal handling (see comment in wait_for_inferior
      at where we set the step_resume breakpoint).  */
-  /* We handle the through_sigtramp_breakpoint the same way; having both
-     one of those and a step_resume_breakpoint is probably very rare (?).  */
 
   static const enum bpstat_what_main_action
     table[(int) class_last][(int) BPSTAT_WHAT_LAST] =
diff -urN src/gdb/infrun.c dev/gdb/infrun.c
--- src/gdb/infrun.c	2007-03-01 06:45:40.000000000 +0100
+++ dev/gdb/infrun.c	2007-03-08 06:00:08.000000000 +0100
@@ -288,10 +288,6 @@
 
 struct regcache *stop_registers;
 
-/* Nonzero if program stopped due to error trying to insert breakpoints.  */
-
-static int breakpoints_failed;
-
 /* Nonzero after stop if current stack frame should be printed.  */
 
 static int stop_print_frame;
@@ -1830,7 +1826,6 @@
   stop_print_frame = 1;
   ecs->random_signal = 0;
   stopped_by_random_signal = 0;
-  breakpoints_failed = 0;
 
   if (stop_signal == TARGET_SIGNAL_TRAP
       && trap_expected
@@ -2126,9 +2121,7 @@
         if (debug_infrun)
 	  fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
 	if (breakpoints_inserted)
-	  {
-	    remove_breakpoints ();
-	  }
+	  remove_breakpoints ();
 	breakpoints_inserted = 0;
 	ecs->another_trap = 1;
 	/* Still need to check other stuff, at least the case
@@ -2909,8 +2902,9 @@
 
       if (!breakpoints_inserted && !ecs->another_trap)
 	{
-	  breakpoints_failed = insert_breakpoints ();
-	  if (breakpoints_failed)
+	  /* Stop stepping when inserting breakpoints
+	     has failed.  */
+	  if (insert_breakpoints () != 0)
 	    {
 	      stop_stepping (ecs);
 	      return;

Re: [Patch]: Little Cleanup

[Ulrich Weigand]

Markus Deuling wrote:

>          * infrun.c (breakpoints_failed): Remove unnecessary variable.
>          (handle_inferior_event): Remove unnecessary braces.
>          * breakpoint.c (bpstat_what): Remove wrong comme

I've committed this version now, thanks.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com