delete_breakpoint: don't try to insert other breakpoints

delete_breakpoint: don't try to insert other breakpoints

[Vladimir Prus]

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

 
The delete_breakpoint function tries to check if the
breakpoint being deleted is inserted in inferior, and if
so, searched for breakpoints that are set at the same address
and tries to insert them.  However, GDB removes breakpoint
from the inferior immediately when inferior is stopped,
so this code will never run.  Removing this code has
no effect on test results. 

(And generally, if that function needed to insert breakpoints,
it should have used insert_breakpoints, instead of duplicating
the logic somewhat).

OK?

- Volodya




[-- Attachment #2: dont_insert.diff --]
[-- Type: text/x-diff, Size: 3730 bytes --]

commit 85f4f28add9d57725aefed2b1ea01e45624d542a
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Wed Nov 14 23:22:04 2007 +0300

    After deleting breakpoint, don't try to insert others.
    
    The delete_breakpoint function tries to check if the
    breakpoint being deleted is inserted in inferior, and if
    so, searched for breakpoints that are set at the same address
    and tries to insert them.  However, GDB removes breakpoint
    from the inferior immediately when inferior is stopped,
    so this code will never run.  Removing this code has
    no effect on test results.
    
          * breakpoint.c (delete_breakpoint): Don't try
          to insert other breakpoints at the same address.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e81ec20..176b890 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7148,80 +7148,6 @@ delete_breakpoint (struct breakpoint *bpt)
 
   check_duplicates (bpt);
 
-  if (bpt->type != bp_hardware_watchpoint
-      && bpt->type != bp_read_watchpoint
-      && bpt->type != bp_access_watchpoint
-      && bpt->type != bp_catch_fork
-      && bpt->type != bp_catch_vfork
-      && bpt->type != bp_catch_exec)
-    for (loc = bpt->loc; loc; loc = loc->next)
-      {
-	/* If this breakpoint location was inserted, and there is 
-	   another breakpoint at the same address, we need to 
-	   insert the other breakpoint.  */
-	if (loc->inserted)
-	  {
-	    struct bp_location *loc2;
-	    ALL_BP_LOCATIONS (loc2)
-	      if (loc2->address == loc->address
-		  && loc2->section == loc->section
-		  && !loc->duplicate
-		  && loc2->owner->enable_state != bp_disabled
-		  && loc2->enabled 
-		  && !loc2->shlib_disabled
-		  && loc2->owner->enable_state != bp_call_disabled)
-		{
-		  int val;
-
-		  /* We should never reach this point if there is a permanent
-		     breakpoint at the same address as the one being deleted.
-		     If there is a permanent breakpoint somewhere, it should
-		     always be the only one inserted.  */
-		  if (loc2->owner->enable_state == bp_permanent)
-		    internal_error (__FILE__, __LINE__,
-				    _("another breakpoint was inserted on top of "
-				      "a permanent breakpoint"));
-
-		  memset (&loc2->target_info, 0, sizeof (loc2->target_info));
-		  loc2->target_info.placed_address = loc2->address;
-		  if (b->type == bp_hardware_breakpoint)
-		    val = target_insert_hw_breakpoint (&loc2->target_info);
-		  else
-		    val = target_insert_breakpoint (&loc2->target_info);
-
-		  /* If there was an error in the insert, print a message, then stop execution.  */
-		  if (val != 0)
-		    {
-		      struct ui_file *tmp_error_stream = mem_fileopen ();
-		      make_cleanup_ui_file_delete (tmp_error_stream);
-		      
-		      
-		      if (b->type == bp_hardware_breakpoint)
-			{
-			  fprintf_unfiltered (tmp_error_stream, 
-					      "Cannot insert hardware breakpoint %d.\n"
-					      "You may have requested too many hardware breakpoints.\n",
-					      b->number);
-			}
-		      else
-			{
-			  fprintf_unfiltered (tmp_error_stream, "Cannot insert breakpoint %d.\n", b->number);
-			  fprintf_filtered (tmp_error_stream, "Error accessing memory address ");
-			  deprecated_print_address_numeric (loc2->address, 1, tmp_error_stream);
-			  fprintf_filtered (tmp_error_stream, ": %s.\n",
-					    safe_strerror (val));
-			}
-		      
-		      fprintf_unfiltered (tmp_error_stream,"The same program may be running in another process.");
-		      target_terminal_ours_for_output ();
-		      error_stream(tmp_error_stream); 
-		    }
-		  else
-		    loc2->inserted = 1;
-		}
-	  }
-      }
-
   free_command_lines (&bpt->commands);
   if (bpt->cond_string != NULL)
     xfree (bpt->cond_string);

Re: delete_breakpoint: don't try to insert other breakpoints

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Wed, 14 Nov 2007 23:24:10 +0300
> 
> The delete_breakpoint function tries to check if the
> breakpoint being deleted is inserted in inferior, and if
> so, searched for breakpoints that are set at the same address
> and tries to insert them.  However, GDB removes breakpoint
> from the inferior immediately when inferior is stopped,
> so this code will never run.

I think you are assuming that this code does and will always run
synchronously.  Is that a valid assumption?

Re: delete_breakpoint: don't try to insert other breakpoints

[Vladimir Prus]

Eli Zaretskii wrote:

>> From: Vladimir Prus <vladimir@codesourcery.com>
>> Date: Wed, 14 Nov 2007 23:24:10 +0300
>> 
>> The delete_breakpoint function tries to check if the
>> breakpoint being deleted is inserted in inferior, and if
>> so, searched for breakpoints that are set at the same address
>> and tries to insert them.  However, GDB removes breakpoint
>> from the inferior immediately when inferior is stopped,
>> so this code will never run.
> 
> I think you are assuming that this code does and will always run
> synchronously.  Is that a valid assumption?

At the moment, I'm not aware of any way to invoke delete_breakpoint
while existing breakpoints are inserted. Is there one?
(In particular, "target async" does not seem to work at all).

In future, this situation might be possible. However, in general 
I think it's better to start by making code clear while still
working for current GDB, and then adjust it as needed for future GDB.
In this particular case, a call to insert_breakpoints, together
with some adjustments, would be much better for any future async code,
as it won't duplicate code logic.

- Volodya

Re: delete_breakpoint: don't try to insert other breakpoints

[Eli Zaretskii]

> From:  Vladimir Prus <ghost@cs.msu.su>
> Date:  Thu, 15 Nov 2007 08:59:19 +0300
> 
> > I think you are assuming that this code does and will always run
> > synchronously.  Is that a valid assumption?
> 
> At the moment, I'm not aware of any way to invoke delete_breakpoint
> while existing breakpoints are inserted. Is there one?
> (In particular, "target async" does not seem to work at all).
> 
> In future, this situation might be possible. However, in general 
> I think it's better to start by making code clear while still
> working for current GDB, and then adjust it as needed for future GDB.
> In this particular case, a call to insert_breakpoints, together
> with some adjustments, would be much better for any future async code,
> as it won't duplicate code logic.

I'm okay with making the code cleaner, but on at a price of removing
features, even if they are currently unused.

Re: delete_breakpoint: don't try to insert other breakpoints

[Vladimir Prus]

On Friday 16 November 2007 13:13:24 Eli Zaretskii wrote:
> > From:  Vladimir Prus <ghost@cs.msu.su>
> > Date:  Thu, 15 Nov 2007 08:59:19 +0300
> > 
> > > I think you are assuming that this code does and will always run
> > > synchronously.  Is that a valid assumption?
> > 
> > At the moment, I'm not aware of any way to invoke delete_breakpoint
> > while existing breakpoints are inserted. Is there one?
> > (In particular, "target async" does not seem to work at all).
> > 
> > In future, this situation might be possible. However, in general 
> > I think it's better to start by making code clear while still
> > working for current GDB, and then adjust it as needed for future GDB.
> > In this particular case, a call to insert_breakpoints, together
> > with some adjustments, would be much better for any future async code,
> > as it won't duplicate code logic.
> 
> I'm okay with making the code cleaner, but on at a price of removing
> features, even if they are currently unused.

Let me clarify -- I'm not aware of *any* way that this code can affect anything
in the current GDB, it's not excercised by any tests, and therefore is essentially
a dead code. Therefore, I don't think it's fair to say this code is a "feature".
Removing it will simplify current code, and the removed part will still be in CVS,
so should we ever need to consult it, it's quite possible. Why keep unused code
in HEAD?

- Volodya

Re: delete_breakpoint: don't try to insert other breakpoints

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Fri, 16 Nov 2007 13:34:40 +0300
> Cc: gdb-patches@sources.redhat.com
> 
> > I'm okay with making the code cleaner, but on at a price of removing
> > features, even if they are currently unused.
> 
> Let me clarify

No need, I understood you the first time.

> Why keep unused code in HEAD?

I'm sorry, I already said why I'm opposed to it.  I have nothing
further to add to what I said.

Re: delete_breakpoint: don't try to insert other breakpoints

[Daniel Jacobowitz]

On Fri, Nov 16, 2007 at 01:13:51PM +0200, Eli Zaretskii wrote:
> > > I'm okay with making the code cleaner, but on at a price of removing
> > > features, even if they are currently unused.

> I'm sorry, I already said why I'm opposed to it.  I have nothing
> further to add to what I said.

I have to say I don't understand this conversation either.  Eli, how
is code which will never be reached a feature?  Vlad, you said this
would be cleaner if it just went back to insert_breakpoints; can we
do that?  Or insert_bp_location, I think.

-- 
Daniel Jacobowitz
CodeSourcery

Re: delete_breakpoint: don't try to insert other breakpoints

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Fri, Nov 16, 2007 at 01:13:51PM +0200, Eli Zaretskii wrote:
>> > > I'm okay with making the code cleaner, but on at a price of removing
>> > > features, even if they are currently unused.
> 
>> I'm sorry, I already said why I'm opposed to it.  I have nothing
>> further to add to what I said.
> 
> I have to say I don't understand this conversation either.  Eli, how
> is code which will never be reached a feature?  Vlad, you said this
> would be cleaner if it just went back to insert_breakpoints; can we
> do that?  Or insert_bp_location, I think.

I just was about to suggest that, but... if I convert this to
insert_breakpoints/insert_bp_location, I cannot actually test it
using current gdb.

In order to test it, I'd need to change GDB to stop deleting
breakpoints from inferior whenever it stops -- and such a change
will likely require rewriting major bits of functionality. I think
it's better to remove code that is apparently not used, and
then revive parts of it as needed for future features (which would
be tested), then hope this code will work.

- Volodya

Re: delete_breakpoint: don't try to insert other breakpoints

[Eli Zaretskii]

> Date: Fri, 16 Nov 2007 08:02:50 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Vladimir Prus <vladimir@codesourcery.com>,
> 	gdb-patches@sources.redhat.com
> 
> Eli, how is code which will never be reached a feature?

The code is never reached only as long as breakpoints are deleted
synchronously (because we remove the breakpoint instruction as soon as
the inferior stops).

Re: delete_breakpoint: don't try to insert other breakpoints

[Nick Roberts]

 > > Eli, how is code which will never be reached a feature?
 > 
 > The code is never reached only as long as breakpoints are deleted
 > synchronously (because we remove the breakpoint instruction as soon as
 > the inferior stops).

I've run the testsuite using my asynchronous version of GDB, with and
without Vladimir's change and, in fact, there was one less failure and
seven more passes with it. Notably, the extra passes with print-threads.exp
involved breakpoints.

Of course, this doesn't prove anything, and there may be a test that
shows the contrary, but it might provide some context.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


diff -cp /extra/src-async/build/gdb/testsuite/gdb1.sum /extra/src-async/build/gdb/testsuite/gdb.sum
*** /extra/src-async/build/gdb/testsuite/gdb1.sum	2007-11-17 01:22:14.000000000 +1300
--- /extra/src-async/build/gdb/testsuite/gdb.sum	2007-11-17 10:32:41.000000000 +1300
***************
*** 1,4 ****
! Test Run By nickrob on Sat Nov 17 00:48:47 2007
  Native configuration is i686-pc-linux-gnu
  
  		=== gdb tests ===
--- 1,4 ----
! Test Run By nickrob on Sat Nov 17 09:59:26 2007
  Native configuration is i686-pc-linux-gnu
  
  		=== gdb tests ===
*************** PASS: gdb.mi/mi2-simplerun.exp: next at 
*** 10993,10999 ****
  PASS: gdb.mi/mi2-simplerun.exp: step at main
  PASS: gdb.mi/mi2-simplerun.exp: step to callee4
  PASS: gdb.mi/mi2-simplerun.exp: exec-finish
! FAIL: gdb.mi/mi2-simplerun.exp: continue to end (1)
  Running ../../../gdb/testsuite/gdb.mi/mi2-stack.exp ...
  PASS: gdb.mi/mi2-stack.exp: breakpoint at callee4
  PASS: gdb.mi/mi2-stack.exp: mi runto callee4
--- 10993,10999 ----
  PASS: gdb.mi/mi2-simplerun.exp: step at main
  PASS: gdb.mi/mi2-simplerun.exp: step to callee4
  PASS: gdb.mi/mi2-simplerun.exp: exec-finish
! PASS: gdb.mi/mi2-simplerun.exp: continue to end
  Running ../../../gdb/testsuite/gdb.mi/mi2-stack.exp ...
  PASS: gdb.mi/mi2-stack.exp: breakpoint at callee4
  PASS: gdb.mi/mi2-stack.exp: mi runto callee4
*************** PASS: gdb.threads/print-threads.exp: pro
*** 11835,11841 ****
  PASS: gdb.threads/print-threads.exp: all threads ran once (slow)
  PASS: gdb.threads/print-threads.exp: break thread_function (3)
  PASS: gdb.threads/print-threads.exp: set var slow = 1 (2)
! FAIL: gdb.threads/print-threads.exp: Running threads (slow with kill breakpoint) (timeout)
  Running ../../../gdb/testsuite/gdb.threads/pthread_cond_wait.exp ...
  PASS: gdb.threads/pthread_cond_wait.exp: successfully compiled posix threads test case
  PASS: gdb.threads/pthread_cond_wait.exp: breakpoint on break_me
--- 11835,11847 ----
  PASS: gdb.threads/print-threads.exp: all threads ran once (slow)
  PASS: gdb.threads/print-threads.exp: break thread_function (3)
  PASS: gdb.threads/print-threads.exp: set var slow = 1 (2)
! PASS: gdb.threads/print-threads.exp: Hit thread_function breakpoint, 1 (slow with kill breakpoint)
! PASS: gdb.threads/print-threads.exp: Hit thread_function breakpoint, 2 (slow with kill breakpoint)
! PASS: gdb.threads/print-threads.exp: Hit thread_function breakpoint, 3 (slow with kill breakpoint)
! PASS: gdb.threads/print-threads.exp: Hit thread_function breakpoint, 4 (slow with kill breakpoint)
! PASS: gdb.threads/print-threads.exp: Hit thread_function breakpoint, 5 (slow with kill breakpoint)
! PASS: gdb.threads/print-threads.exp: program exited normally
! PASS: gdb.threads/print-threads.exp: all threads ran once (slow with kill breakpoint)
  Running ../../../gdb/testsuite/gdb.threads/pthread_cond_wait.exp ...
  PASS: gdb.threads/pthread_cond_wait.exp: successfully compiled posix threads test case
  PASS: gdb.threads/pthread_cond_wait.exp: breakpoint on break_me
*************** PASS: gdb.threads/pthreads.exp: disable
*** 11873,11879 ****
  PASS: gdb.threads/pthreads.exp: tbreak common_routine if hits >= 15
  PASS: gdb.threads/pthreads.exp: stopped before calling common_routine 15 times
  PASS: gdb.threads/pthreads.exp: All threads running after continuing from ^C stop
! PASS: gdb.threads/pthreads.exp: check backtrace from main thread
  PASS: gdb.threads/pthreads.exp: check backtrace from thread 1
  PASS: gdb.threads/pthreads.exp: check backtrace from thread 2
  PASS: gdb.threads/pthreads.exp: apply backtrace command to all three threads
--- 11879,11885 ----
  PASS: gdb.threads/pthreads.exp: tbreak common_routine if hits >= 15
  PASS: gdb.threads/pthreads.exp: stopped before calling common_routine 15 times
  PASS: gdb.threads/pthreads.exp: All threads running after continuing from ^C stop
! FAIL: gdb.threads/pthreads.exp: check backtrace from main thread
  PASS: gdb.threads/pthreads.exp: check backtrace from thread 1
  PASS: gdb.threads/pthreads.exp: check backtrace from thread 2
  PASS: gdb.threads/pthreads.exp: apply backtrace command to all three threads
*************** PASS: gdb.xml/tdesc-xinclude.exp: set td
*** 12226,12233 ****
  
  		=== gdb Summary ===
  
! # of expected passes		11639
! # of unexpected failures	52
  # of unexpected successes	3
  # of expected failures		41
  # of unknown successes		6
--- 12232,12239 ----
  
  		=== gdb Summary ===
  
! # of expected passes		11646
! # of unexpected failures	51
  # of unexpected successes	3
  # of expected failures		41
  # of unknown successes		6

Diff finished.  Sat Nov 17 10:35:47 2007

Re: delete_breakpoint: don't try to insert other breakpoints

[Vladimir Prus]

Nick Roberts wrote:

>  > > Eli, how is code which will never be reached a feature?
>  > 
>  > The code is never reached only as long as breakpoints are deleted
>  > synchronously (because we remove the breakpoint instruction as soon as
>  > the inferior stops).
> 
> I've run the testsuite using my asynchronous version of GDB, with and
> without Vladimir's change and, in fact, there was one less failure and
> seven more passes with it. Notably, the extra passes with
> print-threads.exp involved breakpoints.
> 
> Of course, this doesn't prove anything, and there may be a test that
> shows the contrary, but it might provide some context.

In fact, I've run across this code today while doing something different,
and I now believe it's even not theoretically unused -- it's wrong.
There are two possible cases for having breakpoints inserted when
doing delete_breakpoint:

1. Just not removing breakpoints from inferior when we stop.
2. Ability to delete breakpoints from inferior while inferior is
actually running.

(1) does not seem useful for anything. For (2), delete_breakpoint will:

  - Remove the breakpoint from inferior
  - See if there's another breakpoint at the same address
  - Try to insert it

As the result, there's a window when a breakpoint that should be inserted
is actually not. 

- Volodya

Re: delete_breakpoint: don't try to insert other breakpoints

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sat, 17 Nov 2007 10:42:35 +1300
> Cc: Daniel Jacobowitz <drow@false.org>, vladimir@codesourcery.com,
> 	gdb-patches@sources.redhat.com
> 
>  > > Eli, how is code which will never be reached a feature?
>  > 
>  > The code is never reached only as long as breakpoints are deleted
>  > synchronously (because we remove the breakpoint instruction as soon as
>  > the inferior stops).
> 
> I've run the testsuite using my asynchronous version of GDB, with and
> without Vladimir's change and, in fact, there was one less failure and
> seven more passes with it. Notably, the extra passes with print-threads.exp
> involved breakpoints.

That's because we don't allow deleting breakpoints asynchronously even
in the async version, AFAIK.

Re: delete_breakpoint: don't try to insert other breakpoints

[Nick Roberts]

 > That's because we don't allow deleting breakpoints asynchronously even
 > in the async version, AFAIK.

Yes, I've probably misunderstood the meaning of asynchronous in this
context.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: delete_breakpoint: don't try to insert other breakpoints

[Daniel Jacobowitz]

On Wed, Nov 14, 2007 at 11:24:10PM +0300, Vladimir Prus wrote:
>  
> The delete_breakpoint function tries to check if the
> breakpoint being deleted is inserted in inferior, and if
> so, searched for breakpoints that are set at the same address
> and tries to insert them.  However, GDB removes breakpoint
> from the inferior immediately when inferior is stopped,
> so this code will never run.  Removing this code has
> no effect on test results. 
> 
> (And generally, if that function needed to insert breakpoints,
> it should have used insert_breakpoints, instead of duplicating
> the logic somewhat).
> 
> OK?

I believe that another version of this patch, which replaces the wrong
code with more useful behavior, is part of your keep breakpoints
inserted RFC, so this patch is withdrawn.  Let me know if I'm
wrong :-)


-- 
Daniel Jacobowitz
CodeSourcery