RFC: don't set the pspace on ordinary breakpoints

RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

I would appreciate comments on this patch.

This is the second of two patches in preparation for my "ambiguous
linespec" patch.  I think they are reasonably independent so I am
sending them separately.


This patch does two things.

First, it changes ordinary breakpoints to set leave their 'pspace' field
NULL.  The rationale for this is that, with the ambiguous linespec
patch, a linespec may apply to different program spaces, not just the
one which was selected when the breakpoint was created.  For these
breakpoints, we do not want breakpoint_program_space_exit to remove the
breakpoint.

I looked at removing the pspace field entirely.  Internal breakpoints
often set this field, though, and I chose not to touch all this code in
hopes of keeping the patch down to a reasonable size.  So, I left the
field and added a special meaning for NULL.


Second, this patch removes bp_startup_disabled.  Instead, I changed
should_be_inserted to directly examine the location's pspace, and I
changed one spot (update_global_location_list) to use should_be_inserted
to pick this up.

Built and regtested on x86-64 F15, but only when applied on top of the
previous patch.

Tom

2011-10-26  Tom Tromey  <tromey@redhat.com>

	* elfread.c (elf_gnu_ifunc_resolver_return_stop): Allow
	breakpoint's pspace to be NULL.
	* breakpoint.h (enum enable_state) <bp_startup_disabled>: Remove.
	(struct breakpoint) <pspace>: Update comment.
	* breakpoint.c (should_be_inserted): Explicitly check if program
	space is executing startup.
	(describe_other_breakpoints): Update.
	(init_raw_breakpoint): Conditionally set breakpoint's pspace.
	(disable_breakpoints_before_startup): Change executing_startup
	earlier.  Use location's pspace.
	(enable_breakpoints_after_startup): Likewise.
	(init_breakpoint_sal): Don't set breakpoint's pspace.  Don't use
	bp_startup_disabled.
	(create_breakpoint): Conditionally set breakpoint's pspace.  Don't
	use bp_startup_disabled.
	(update_global_location_list): Use should_be_inserted.
	(bkpt_re_set): Update.
	(prepare_re_set_context): Conditionally switch program space.

2011-10-26  Tom Tromey  <tromey@redhat.com>

From b7cf18b3ee2b13ef0855fd196fa242d238b03949 Mon Sep 17 00:00:00 2001
From: Tom Tromey <tromey@redhat.com>
Date: Fri, 7 Oct 2011 10:56:00 -0600
Subject: [PATCH 2/2] don't set the pspace on ordinary breakpoints also remove
 bp_startup_disabled

---
 gdb/ChangeLog    |   21 +++++++++++++++
 gdb/breakpoint.c |   76 ++++++++++++++++++++++--------------------------------
 gdb/breakpoint.h |   12 ++------
 gdb/elfread.c    |    2 +-
 4 files changed, 56 insertions(+), 55 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3ae7508..1c6a43b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1578,6 +1578,9 @@ should_be_inserted (struct bp_location *bl)
   if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
     return 0;
 
+  if (bl->pspace->executing_startup)
+    return 0;
+
   /* This is set for example, when we're attached to the parent of a
      vfork, and have detached from the child.  The child is running
      free, and we expect it to do an exec or exit, at which point the
@@ -5323,8 +5326,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 	      printf_filtered (" (thread %d)", b->thread);
 	    printf_filtered ("%s%s ",
 			     ((b->enable_state == bp_disabled
-			       || b->enable_state == bp_call_disabled
-			       || b->enable_state == bp_startup_disabled)
+			       || b->enable_state == bp_call_disabled)
 			      ? " (disabled)"
 			      : b->enable_state == bp_permanent 
 			      ? " (permanent)"
@@ -5817,9 +5819,11 @@ init_raw_breakpoint (struct breakpoint *b, struct gdbarch *gdbarch,
   b->loc->address = adjusted_address;
   b->loc->pspace = sal.pspace;
 
-  /* Store the program space that was used to set the breakpoint, for
-     breakpoint resetting.  */
-  b->pspace = sal.pspace;
+  /* Store the program space that was used to set the breakpoint,
+     except for ordinary breakpoints, which are independent of the
+     program space.  */
+  if (bptype != bp_breakpoint && bptype != bp_hardware_breakpoint)
+    b->pspace = sal.pspace;
 
   if (sal.symtab == NULL)
     b->source_file = NULL;
@@ -6960,48 +6964,48 @@ enable_watchpoints_after_interactive_call_stop (void)
 void
 disable_breakpoints_before_startup (void)
 {
-  struct breakpoint *b;
+  struct bp_location *loc, **locp_tmp;
   int found = 0;
 
-  ALL_BREAKPOINTS (b)
+  current_program_space->executing_startup = 1;
+
+  ALL_BP_LOCATIONS (loc, locp_tmp)
     {
-      if (b->pspace != current_program_space)
+      if (loc->pspace != current_program_space)
 	continue;
 
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && breakpoint_enabled (b))
+      if ((loc->owner->type == bp_breakpoint
+	   || loc->owner->type == bp_hardware_breakpoint)
+	  && breakpoint_enabled (loc->owner))
 	{
-	  b->enable_state = bp_startup_disabled;
 	  found = 1;
+	  break;
 	}
     }
 
   if (found)
     update_global_location_list (0);
-
-  current_program_space->executing_startup = 1;
 }
 
 void
 enable_breakpoints_after_startup (void)
 {
-  struct breakpoint *b;
+  struct bp_location *loc, **locp_tmp;
   int found = 0;
 
   current_program_space->executing_startup = 0;
 
-  ALL_BREAKPOINTS (b)
+  ALL_BP_LOCATIONS (loc, locp_tmp)
     {
-      if (b->pspace != current_program_space)
+      if (loc->pspace != current_program_space)
 	continue;
 
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && b->enable_state == bp_startup_disabled)
+      if ((loc->owner->type == bp_breakpoint
+	   || loc->owner->type == bp_hardware_breakpoint)
+	  && breakpoint_enabled (loc->owner))
 	{
-	  b->enable_state = bp_enabled;
 	  found = 1;
+	  break;
 	}
     }
 
@@ -7241,7 +7245,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	  b->ignore_count = ignore_count;
 	  b->enable_state = enabled ? bp_enabled : bp_disabled;
 	  b->disposition = disposition;
-	  b->pspace = sals.sals[0].pspace;
 
 	  if (type == bp_static_tracepoint)
 	    {
@@ -7282,11 +7285,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 			   "tracepoint marker to probe"));
 	    }
 
-	  if (enabled && b->pspace->executing_startup
-	      && (b->type == bp_breakpoint
-		  || b->type == bp_hardware_breakpoint))
-	    b->enable_state = bp_startup_disabled;
-
 	  loc = b->loc;
 	}
       else
@@ -8001,14 +7999,10 @@ create_breakpoint (struct gdbarch *gdbarch,
       b->disposition = tempflag ? disp_del : disp_donttouch;
       b->condition_not_parsed = 1;
       b->enable_state = enabled ? bp_enabled : bp_disabled;
-      b->pspace = current_program_space;
+      if (type_wanted != bp_breakpoint && type_wanted != bp_hardware_breakpoint)
+	b->pspace = current_program_space;
       b->py_bp_object = NULL;
 
-      if (enabled && b->pspace->executing_startup
-	  && (b->type == bp_breakpoint
-	      || b->type == bp_hardware_breakpoint))
-	b->enable_state = bp_startup_disabled;
-
       if (!internal)
         /* Do not mention breakpoints with a negative number, 
 	   but do notify observers.  */
@@ -10625,13 +10619,8 @@ update_global_location_list (int should_insert)
       struct breakpoint *b = loc->owner;
       struct bp_location **loc_first_p;
 
-      if (b->enable_state == bp_disabled
-	  || b->enable_state == bp_call_disabled
-	  || b->enable_state == bp_startup_disabled
-	  || !loc->enabled
-	  || loc->shlib_disabled
-	  || !breakpoint_address_is_meaningful (b)
-	  || is_tracepoint (b))
+      if (!should_be_inserted (loc)
+	  || !breakpoint_address_is_meaningful (b))
 	continue;
 
       /* Permanent breakpoint should always be inserted.  */
@@ -10909,10 +10898,6 @@ static struct breakpoint_ops base_breakpoint_ops =
 static void
 bkpt_re_set (struct breakpoint *b)
 {
-  /* Do not attempt to re-set breakpoints disabled during startup.  */
-  if (b->enable_state == bp_startup_disabled)
-    return;
-
   /* FIXME: is this still reachable?  */
   if (b->addr_string == NULL)
     {
@@ -11931,7 +11916,8 @@ prepare_re_set_context (struct breakpoint *b)
 
   input_radix = b->input_radix;
   cleanups = save_current_space_and_thread ();
-  switch_to_program_space_and_thread (b->pspace);
+  if (b->pspace != NULL)
+    switch_to_program_space_and_thread (b->pspace);
   set_language (b->language);
 
   return cleanups;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index dba5392..e038999 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -186,14 +186,6 @@ enum enable_state
 			    automatically enabled and reset when the
 			    call "lands" (either completes, or stops
 			    at another eventpoint).  */
-    bp_startup_disabled, /* The eventpoint has been disabled during
-			    inferior startup.  This is necessary on
-			    some targets where the main executable
-			    will get relocated during startup, making
-			    breakpoint addresses invalid.  The
-			    eventpoint will be automatically enabled
-			    and reset once inferior startup is
-			    complete.  */
     bp_permanent	 /* There is a breakpoint instruction
 			    hard-wired into the target's code.  Don't
 			    try to write another breakpoint
@@ -571,7 +563,9 @@ struct breakpoint
        equals this.  */
     struct frame_id frame_id;
 
-    /* The program space used to set the breakpoint.  */
+    /* The program space used to set the breakpoint.  This is only set
+       for breakpoints which are specific to a program space; for
+       ordinary breakpoints this is NULL.  */
     struct program_space *pspace;
 
     /* String we used to set the breakpoint (malloc'd).  */
diff --git a/gdb/elfread.c b/gdb/elfread.c
index a309a2c..067c77f 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1032,7 +1032,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
     }
   gdb_assert (b->type == bp_gnu_ifunc_resolver);
 
-  gdb_assert (current_program_space == b->pspace);
+  gdb_assert (current_program_space == b->pspace || b->pspace == NULL);
   elf_gnu_ifunc_record_cache (b->addr_string, resolved_pc);
 
   sal = find_pc_line (resolved_pc, 0);
-- 
1.7.6.4

Re: RFC: don't set the pspace on ordinary breakpoints

[Jan Kratochvil]

On Thu, 27 Oct 2011 17:20:38 +0200, Tom Tromey wrote:
> Built and regtested on x86-64 F15, but only when applied on top of the
> previous patch.

While I find the patch OK I have a regression on
{x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu:

 run^M
 Starting program: gdb/testsuite.unix.-m64/gdb.multi/bkpt-multi-exec ^M
+Error in re-setting breakpoint 1: No source file named crashme.c.^M
 foll-exec is about to execl(crashme)...^M
-process 30058 is executing new program: gdb/testsuite.unix.-m64/gdb.multi/crashme^M
+process 13203 is executing new program: gdb/testsuite.unix.-m64/gdb.multi/crashme^M
+Oh no, a bug!^M
 ^M
-Breakpoint 1, main (argc=1, argv=0x7fffffffe1d8) at gdb/testsuite/gdb.multi/crashme.c:9^M
-9        printf ("Oh no, a bug!\n"); /* set breakpoint here */^M
-(gdb) PASS: gdb.multi/bkpt-multi-exec.exp: run
+Program received signal SIGSEGV, Segmentation fault.^M
+0x0000000000400519 in main (argc=1, argv=0x7fffffffe1b8) at gdb/testsuite/gdb.multi/crashme.c:11^M
+11       return *foo;^M
+(gdb) FAIL: gdb.multi/bkpt-multi-exec.exp: run


BTW the term "ordinary breakpoint" could be defined somewhere but it is
already used in FSF GDB HEAD.


Thanks,
Jan

Re: RFC: don't set the pspace on ordinary breakpoints

[Yao Qi]

On 10/27/2011 11:20 PM, Tom Tromey wrote:
> First, it changes ordinary breakpoints to set leave their 'pspace' field
> NULL.  The rationale for this is that, with the ambiguous linespec
> patch, a linespec may apply to different program spaces, not just the
> one which was selected when the breakpoint was created.  For these
> breakpoints, we do not want breakpoint_program_space_exit to remove the
> breakpoint.
> 

> @@ -5817,9 +5819,11 @@ init_raw_breakpoint (struct breakpoint *b,
struct gdbarch *gdbarch,
>    b->loc->address = adjusted_address;
>    b->loc->pspace = sal.pspace;
>
> -  /* Store the program space that was used to set the breakpoint, for
> -     breakpoint resetting.  */
> -  b->pspace = sal.pspace;
> +  /* Store the program space that was used to set the breakpoint,
> +     except for ordinary breakpoints, which are independent of the
> +     program space.  */
> +  if (bptype != bp_breakpoint && bptype != bp_hardware_breakpoint)
> +    b->pspace = sal.pspace;

Looks "ordinary breakpoints" includes bp_breakpoint and
bp_hardware_breakpoint.  However, as you described above, IIUC,
'ambiguous linespec' should apply to trap tracepoint and fast tracepoint
as well.

-- 
Yao (齐尧)

Re: RFC: don't set the pspace on ordinary breakpoints

[Pedro Alves]

On Thursday 27 October 2011 16:20:38, Tom Tromey wrote:
> I would appreciate comments on this patch.
> 
> This is the second of two patches in preparation for my "ambiguous
> linespec" patch.  I think they are reasonably independent so I am
> sending them separately.
> 
> 
> This patch does two things.

Could have been two independent patches.

> First, it changes ordinary breakpoints to set leave their 'pspace' field
> NULL.  The rationale for this is that, with the ambiguous linespec
> patch, a linespec may apply to different program spaces, not just the
> one which was selected when the breakpoint was created.  For these
> breakpoints, we do not want breakpoint_program_space_exit to remove the
> breakpoint.

I'm not clear how this isn't breaking multi-process without
the linespec changes.  I mean, if we no longer have a pspace
pointer, breakpoint re-setting will no longer work correctly.
(breakpoint_re_set -> ... -> prepare_re_set_context)

> I looked at removing the pspace field entirely.  Internal breakpoints
> often set this field, though, and I chose not to touch all this code in
> hopes of keeping the patch down to a reasonable size.  So, I left the
> field and added a special meaning for NULL.
> 
> 
> Second, this patch removes bp_startup_disabled.  Instead, I changed
> should_be_inserted to directly examine the location's pspace, and I
> changed one spot (update_global_location_list) to use should_be_inserted
> to pick this up.

I don't think that's correct.  During startup, we disable user breakpoints,
because the symbols haven't been relocated yet.  But, we still need to
insert internal breakpoints set at magic addresses (not through symbols), so
that we know when the startup is done with.  Ulrich?

-- 
Pedro Alves

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Could have been two independent patches.

Ok, noted.

Tom> First, it changes ordinary breakpoints to set leave their 'pspace' field
Tom> NULL.  The rationale for this is that, with the ambiguous linespec
Tom> patch, a linespec may apply to different program spaces, not just the
Tom> one which was selected when the breakpoint was created.  For these
Tom> breakpoints, we do not want breakpoint_program_space_exit to remove the
Tom> breakpoint.

Pedro> I'm not clear how this isn't breaking multi-process without
Pedro> the linespec changes.  I mean, if we no longer have a pspace
Pedro> pointer, breakpoint re-setting will no longer work correctly.
Pedro> (breakpoint_re_set -> ... -> prepare_re_set_context)

Yeah, it probably does.
I'm not sure why this doesn't result in any regressions.

I won't put it in separately... it is mostly useful as a way to break
off a chunk of the huge linespec patch for readability.

Pedro> I don't think that's correct.  During startup, we disable user
Pedro> breakpoints, because the symbols haven't been relocated yet.
Pedro> But, we still need to insert internal breakpoints set at magic
Pedro> addresses (not through symbols), so that we know when the startup
Pedro> is done with.  Ulrich?

Ok.  How would I test this?

Plan B would be to put the startup-disabled state on bp_location.

Tom

Re: RFC: don't set the pspace on ordinary breakpoints

[Pedro Alves]

On Wednesday 02 November 2011 19:46:54, Tom Tromey wrote:

> Pedro> I'm not clear how this isn't breaking multi-process without
> Pedro> the linespec changes.  I mean, if we no longer have a pspace
> Pedro> pointer, breakpoint re-setting will no longer work correctly.
> Pedro> (breakpoint_re_set -> ... -> prepare_re_set_context)
> 
> Yeah, it probably does.
> I'm not sure why this doesn't result in any regressions.

Jan pointed at one in gdb.multi/.  Not sure if it's related.

> I won't put it in separately... it is mostly useful as a way to break
> off a chunk of the huge linespec patch for readability.

Yeah, thanks for that.

> Pedro> I don't think that's correct.  During startup, we disable user
> Pedro> breakpoints, because the symbols haven't been relocated yet.
> Pedro> But, we still need to insert internal breakpoints set at magic
> Pedro> addresses (not through symbols), so that we know when the startup
> Pedro> is done with.  Ulrich?
> 
> Ok.  How would I test this?

The only calls to disable_breakpoints_before_startup/enable_breakpoints_after_startup
are in solib-spu.c, so you'd need testing on Cell.

> Plan B would be to put the startup-disabled state on bp_location.

Maybe keeping your change, but disabling only user breakpoints would
be enough:

 @@ -1578,6 +1578,9 @@ should_be_inserted (struct bp_location *bl)
    if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
      return 0;
 
-+  if (bl->pspace->executing_startup)
++  if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
 +    return 0;

Though I'm not sure of the impact the bkpt_re_set change has.
I'm hoping Ulrich still remembers things from when he added the
startup disabled stuff.  :-)

On Wednesday 02 November 2011 19:46:54, Tom Tromey wrote:
> Pedro> Could have been two independent patches.
> Ok, noted.

Thanks.  IMO, the executing_startup change IMO could go in
separately, as soon as it is correct, it looks quite independent.
What prompted it, BTW?

-- 
Pedro Alves

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Thanks.  IMO, the executing_startup change IMO could go in
Pedro> separately, as soon as it is correct, it looks quite independent.
Pedro> What prompted it, BTW?

It seemed wrong to mark an entire breakpoint as "startup disabled" in
the case where the breakpoint has locations in multiple inferiors.

Tom

Re: RFC: don't set the pspace on ordinary breakpoints

[Pedro Alves]

On Thursday 03 November 2011 14:01:19, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> Thanks.  IMO, the executing_startup change IMO could go in
> Pedro> separately, as soon as it is correct, it looks quite independent.
> Pedro> What prompted it, BTW?
> 
> It seemed wrong to mark an entire breakpoint as "startup disabled" in
> the case where the breakpoint has locations in multiple inferiors.

Ah, indeed.

>  static void
>  bkpt_re_set (struct breakpoint *b)
>  {
> -  /* Do not attempt to re-set breakpoints disabled during startup.  */
> -  if (b->enable_state == bp_startup_disabled)
> -    return;
> -

I'm looking at this one, and thinking that if we simply remove the
guard, we'll allow re-set to go through while we can't trust
symbols/sections in the program space, because they haven't been
relocated yet, but this is wrong (as opposed to just inneficient)
because GDB will actually read memory from the wrong addresses
(prologue skipping, breakpoint shadow, at least).

We can't just make bp_startup_disabled be per-location,
because a re-set could find new locations, defeating the guard.

So I think we'll maybe want this:

 static void
 bkpt_re_set (struct breakpoint *b)
 {
-  /* Do not attempt to re-set breakpoints disabled during startup.  */
-  if (b->enable_state == bp_startup_disabled)
-    return;
+  if (current_program_space->executing_startup)
+    return 0;

And adjust the `current_program_space' reference in the
ambiguous linespec patch to whatever will be necessary then.

and:

@@ -1578,6 +1578,9 @@ should_be_inserted (struct bp_location *bl)
   if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
     return 0;

+   if (!bl->owner->ops->should_be_inserted (bl))
+     return 0;

With the regular breakpoint's (or any other breakpoint that
uses linespecs) implementation of the new should_be_inserted hook
being:

static int
bkpt_should_be_inserted (struct bp_location *bl)
{
  return !bl->pspace->executing_startup;
}

It seems to me that watchpoints shouldn't be
inserted during startup either (can't trust symbols
for the watchpoint's expression either), so maybe my
previous suggestion is indeed good enough:

 @@ -1578,6 +1578,9 @@ should_be_inserted (struct bp_location *bl)
    if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
      return 0;
 
-+  if (bl->pspace->executing_startup)
++  if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
 +    return 0;

Unless we have some user breakpoint kind that would want to 
be inserted in the startup phase.  One such example may simply be
a breakpoint set at a specific address, to debug exactly the
startup phase.  So we'd need:

static int
bkpt_should_be_inserted (struct bp_location *bl)
{
  if (!bl->pspace->executing_startup)
   {
      struct breakpoint *b = bl->owner;

      if (!linespec_is_address (b->addr_string))
        return 0;

      if (b->addr_string_range_end != NULL
          && !linespec_is_address (b->addr_string_range_end))
        return 0;
    }

  return 1;
}

linespec_is_address would return true to linespecs of "*NUMBER" form.
(or if we have a linespec struct, we'd record address-ness there).

-- 
Pedro Alves

Re: RFC: don't set the pspace on ordinary breakpoints

[Ulrich Weigand]

Pedro Alves wrote:
> On Thursday 27 October 2011 16:20:38, Tom Tromey wrote:
> > Second, this patch removes bp_startup_disabled.  Instead, I changed
> > should_be_inserted to directly examine the location's pspace, and I
> > changed one spot (update_global_location_list) to use should_be_inserted
> > to pick this up.
> 
> I don't think that's correct.  During startup, we disable user breakpoints,
> because the symbols haven't been relocated yet.  But, we still need to
> insert internal breakpoints set at magic addresses (not through symbols), so
> that we know when the startup is done with.  Ulrich?

The scenario that was fixed by the disabled-during-startup feature is
debugging stand-alone SPU applications on Cell/B.E. using the combined
PPU/SPU debugger.  A stand-alone application is an SPU architecture
ELF binary; running such a binary involves binfmt_misc loader which
is a PowerPC executable that loads the SPU binary up into an SPU
context and runs it.

From GDB's perspective, before the program is started, the inferior
is just a plain "exec" target for the SPU binary.  The user will be
able to set breakpoint on SPU funtions, source lines etc.

Once the binary is started, however, GDB will suddenly see the PowerPC
loader executable.  After a while, once the loader has finished starting
up the SPU context, GDB will suddenly see the SPU executable (in that
newly started context) again.  At that point, all breakpoints can be
re-set (properly relocated to the virtual address that includes the
SPU context ID).

However, in the time in-between, when the loader has started up but
not yet loaded the SPU executable, any attempt to place a SPU breakpoint
will fail, because nothing is at those addresses.   Likewise, any attempt
to re-set the breakpoint will fail, because the SPU executable is not
in fact visible in the inferior during this time period.

Thus the idea was to simply make all such breakpoints that might have been
set by the user during the "exec" target phase completely dormant until
such time as the SPU context management code in solib-spu.c has detected
the presence of the newly started-up SPU context.

However, *system* breakpoints, in particular those needed by the shared
library (and SPU context) layers, must definitely be placed as usual
during that period, otherwise that detection of the new SPU context
will itself fail.


> On Thursday 03 November 2011 14:01:19, Tom Tromey wrote:
> > >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> > 
> > Pedro> Thanks.  IMO, the executing_startup change IMO could go in
> > Pedro> separately, as soon as it is correct, it looks quite independent.
> > Pedro> What prompted it, BTW?
> > 
> > It seemed wrong to mark an entire breakpoint as "startup disabled" in
> > the case where the breakpoint has locations in multiple inferiors.
> 
> Ah, indeed.
> 
> >  static void
> >  bkpt_re_set (struct breakpoint *b)
> >  {
> > -  /* Do not attempt to re-set breakpoints disabled during startup.  */
> > -  if (b->enable_state == bp_startup_disabled)
> > -    return;
> > -
> 
> I'm looking at this one, and thinking that if we simply remove the
> guard, we'll allow re-set to go through while we can't trust
> symbols/sections in the program space, because they haven't been
> relocated yet, but this is wrong (as opposed to just inneficient)
> because GDB will actually read memory from the wrong addresses
> (prologue skipping, breakpoint shadow, at least).
> 
> We can't just make bp_startup_disabled be per-location,
> because a re-set could find new locations, defeating the guard.

Agreed, exactly.

> So I think we'll maybe want this:
> 
>  static void
>  bkpt_re_set (struct breakpoint *b)
>  {
> -  /* Do not attempt to re-set breakpoints disabled during startup.  */
> -  if (b->enable_state == bp_startup_disabled)
> -    return;
> +  if (current_program_space->executing_startup)
> +    return 0;
> 
> And adjust the `current_program_space' reference in the
> ambiguous linespec patch to whatever will be necessary then.

This assumes that bkpt_re_set is never called for the system breakpoints,
which seems to be true as far as I can see ...

> and:
> 
> @@ -1578,6 +1578,9 @@ should_be_inserted (struct bp_location *bl)
>    if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
>      return 0;
> 
> +   if (!bl->owner->ops->should_be_inserted (bl))
> +     return 0;
> 
> With the regular breakpoint's (or any other breakpoint that
> uses linespecs) implementation of the new should_be_inserted hook
> being:
> 
> static int
> bkpt_should_be_inserted (struct bp_location *bl)
> {
>   return !bl->pspace->executing_startup;
> }
> 
> It seems to me that watchpoints shouldn't be
> inserted during startup either (can't trust symbols
> for the watchpoint's expression either), so maybe my
> previous suggestion is indeed good enough:
> 
>  @@ -1578,6 +1578,9 @@ should_be_inserted (struct bp_location *bl)
>     if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
>       return 0;
>  
> -+  if (bl->pspace->executing_startup)
> ++  if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
>  +    return 0;

That looks reasonable to me.

> Unless we have some user breakpoint kind that would want to 
> be inserted in the startup phase.  One such example may simply be
> a breakpoint set at a specific address, to debug exactly the
> startup phase.  So we'd need:
> 
> static int
> bkpt_should_be_inserted (struct bp_location *bl)
> {
>   if (!bl->pspace->executing_startup)
>    {
>       struct breakpoint *b = bl->owner;
> 
>       if (!linespec_is_address (b->addr_string))
>         return 0;
> 
>       if (b->addr_string_range_end != NULL
>           && !linespec_is_address (b->addr_string_range_end))
>         return 0;
>     }
> 
>   return 1;
> }
> 
> linespec_is_address would return true to linespecs of "*NUMBER" form.
> (or if we have a linespec struct, we'd record address-ness there).

For the SPU case, this would also cause failures, since even *NUMBER
is not there while the loader is active.  (Once the SPU context is
back up, *NUMBER will work because the address will be re-located
to the proper SPU context ID via the integer_to_address hook.)

Bye,
Ulrich

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

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I'm looking at this one, and thinking that if we simply remove the
Pedro> guard, we'll allow re-set to go through while we can't trust
Pedro> symbols/sections in the program space, because they haven't been
Pedro> relocated yet, but this is wrong (as opposed to just inneficient)
Pedro> because GDB will actually read memory from the wrong addresses
Pedro> (prologue skipping, breakpoint shadow, at least).

Pedro> We can't just make bp_startup_disabled be per-location,
Pedro> because a re-set could find new locations, defeating the guard.

Yeah, that makes sense.

I think I will change linespec to ignore program spaces in this state.

Tom

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

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

Tom> I think I will change linespec to ignore program spaces in this state.

The appended has two parts: the linespec.c change, relative to the big
patch I posted, and the breakpoint.c change I think is needed too.  Only
the latter makes sense in the context of this thread.

I *think* the should_be_inserted change is all that was really needed,
after re-reading all the messages in this thread.  Let me know what you
think.

The addr_string_to_sals change is there to handle the case where a
breakpoint has hits in just the program space that is in the
executing_startup state.  In this situation, linespec can throw an
exception, and if we do not take the "not_found_and_ok" branch, then we
will proceed to disable the breakpoint... which is wrong, we want to
re-set again when the program space is done with startup.

Tom

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 610aece..dd1f935 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1578,7 +1578,7 @@ should_be_inserted (struct bp_location *bl)
   if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
     return 0;
 
-  if (bl->pspace->executing_startup)
+  if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
     return 0;
 
   /* This is set for example, when we're attached to the parent of a
@@ -11725,6 +11725,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
       if (e.error == NOT_FOUND_ERROR
 	  && (b->condition_not_parsed 
 	      || (b->loc && b->loc->shlib_disabled)
+	      || (b->loc && b->loc->pspace->executing_startup)
 	      || b->enable_state == bp_disabled))
 	not_found_and_ok = 1;
 
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 21189d3..c24a7a0 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -351,6 +351,8 @@ iterate_over_all_matching_symtabs (const char *name,
   {
     if (search_pspace != NULL && search_pspace != pspace)
       continue;
+    if (pspace->executing_startup)
+      continue;
 
     set_current_program_space (pspace);
 
@@ -1327,6 +1329,12 @@ decode_indirect (struct linespec_state *self, char **argptr)
   CORE_ADDR pc;
   char *initial = *argptr;
   
+  if (current_program_space->executing_startup)
+    /* The error message doesn't really matter, because this case
+       should only hit during breakpoint reset.  */
+    throw_error (NOT_FOUND_ERROR, _("cannot evaluate expressions while "
+				    "program space is in startup"));
+
   (*argptr)++;
   pc = value_as_address (parse_to_comma_and_eval (argptr));
 
@@ -1872,6 +1880,9 @@ lookup_prefix_sym (char **argptr, char *p, VEC (symtab_p) *file_symtabs,
 	{
 	  struct block *search_block;
 
+	  /* Program spaces that are executing startup should have
+	     been filtered out earlier.  */
+	  gdb_assert (!SYMTAB_PSPACE (elt)->executing_startup);
 	  set_current_program_space (SYMTAB_PSPACE (elt));
 	  search_block = get_search_block (elt);
 	  iterate_over_symbols (search_block, copy, STRUCT_DOMAIN,
@@ -2020,6 +2031,9 @@ find_method (struct linespec_state *self, char *saved_arg,
       struct type *t;
       struct program_space *pspace;
 
+      /* Program spaces that are executing startup should have
+	 been filtered out earlier.  */
+      gdb_assert (!SYMTAB_PSPACE (SYMBOL_SYMTAB (sym))->executing_startup);
       pspace = SYMTAB_PSPACE (SYMBOL_SYMTAB (sym));
       set_current_program_space (pspace);
       t = check_typedef (SYMBOL_TYPE (sym));
@@ -2122,6 +2136,9 @@ collect_symtabs_from_filename (const char *file)
   /* Find that file's data.  */
   ALL_PSPACES (pspace)
   {
+    if (pspace->executing_startup)
+      continue;
+
     set_current_program_space (pspace);
     iterate_over_symtabs (file, add_symtabs_to_list, &collector);
   }
@@ -2698,6 +2715,8 @@ search_minsyms_for_name (struct collect_info *info, const char *name,
   {
     if (search_pspace != NULL && search_pspace != pspace)
       continue;
+    if (pspace->executing_startup)
+      continue;
 
     set_current_program_space (pspace);
 
@@ -2734,6 +2753,9 @@ add_matching_symbols_to_info (const char *name,
 	}
       else if (pspace == NULL || pspace == SYMTAB_PSPACE (elt))
 	{
+	  /* Program spaces that are executing startup should have
+	     been filtered out earlier.  */
+	  gdb_assert (!SYMTAB_PSPACE (elt)->executing_startup);
 	  set_current_program_space (SYMTAB_PSPACE (elt));
 	  iterate_over_symbols (get_search_block (elt), name,
 				VAR_DOMAIN, collect_symbols,

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

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

Tom> I *think* the should_be_inserted change is all that was really needed,
Tom> after re-reading all the messages in this thread.  Let me know what you
Tom> think.

Here is a refresh of this patch.  This one is relative to CVS HEAD.
I didn't attempt to test it in isolation.

Tom

2011-10-28  Tom Tromey  <tromey@redhat.com>

	* elfread.c (elf_gnu_ifunc_resolver_return_stop): Allow
	breakpoint's pspace to be NULL.
	* breakpoint.h (enum enable_state) <bp_startup_disabled>: Remove.
	(struct breakpoint) <pspace>: Update comment.
	* breakpoint.c (should_be_inserted): Explicitly check if program
	space is executing startup.
	(describe_other_breakpoints): Update.
	(init_raw_breakpoint): Conditionally set breakpoint's pspace.
	(disable_breakpoints_before_startup): Change executing_startup
	earlier.  Use location's pspace.
	(enable_breakpoints_after_startup): Likewise.
	(init_breakpoint_sal): Don't set breakpoint's pspace.  Don't use
	bp_startup_disabled.
	(create_breakpoint): Conditionally set breakpoint's pspace.  Don't
	use bp_startup_disabled.
	(update_global_location_list): Use should_be_inserted.
	(bkpt_re_set): Update.
	(prepare_re_set_context): Conditionally switch program space.

From 0315321273c5dad6ec98744439c72e1e00729be7 Mon Sep 17 00:00:00 2001
From: Tom Tromey <tromey@redhat.com>
Date: Tue, 8 Nov 2011 13:50:12 -0700
Subject: [PATCH 1/2] remove bp_startup_disabled

---
 gdb/ChangeLog    |   21 ++++++++++++++
 gdb/breakpoint.c |   77 ++++++++++++++++++++++-------------------------------
 gdb/breakpoint.h |   12 ++------
 gdb/elfread.c    |    2 +-
 4 files changed, 57 insertions(+), 55 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8c98bef..8ab09ba 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1568,6 +1568,9 @@ should_be_inserted (struct bp_location *bl)
   if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
     return 0;
 
+  if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
+    return 0;
+
   /* This is set for example, when we're attached to the parent of a
      vfork, and have detached from the child.  The child is running
      free, and we expect it to do an exec or exit, at which point the
@@ -5313,8 +5316,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 	      printf_filtered (" (thread %d)", b->thread);
 	    printf_filtered ("%s%s ",
 			     ((b->enable_state == bp_disabled
-			       || b->enable_state == bp_call_disabled
-			       || b->enable_state == bp_startup_disabled)
+			       || b->enable_state == bp_call_disabled)
 			      ? " (disabled)"
 			      : b->enable_state == bp_permanent 
 			      ? " (permanent)"
@@ -5790,9 +5792,11 @@ init_raw_breakpoint (struct breakpoint *b, struct gdbarch *gdbarch,
   b->loc->address = adjusted_address;
   b->loc->pspace = sal.pspace;
 
-  /* Store the program space that was used to set the breakpoint, for
-     breakpoint resetting.  */
-  b->pspace = sal.pspace;
+  /* Store the program space that was used to set the breakpoint,
+     except for ordinary breakpoints, which are independent of the
+     program space.  */
+  if (bptype != bp_breakpoint && bptype != bp_hardware_breakpoint)
+    b->pspace = sal.pspace;
 
   if (sal.symtab == NULL)
     b->source_file = NULL;
@@ -6933,48 +6937,48 @@ enable_watchpoints_after_interactive_call_stop (void)
 void
 disable_breakpoints_before_startup (void)
 {
-  struct breakpoint *b;
+  struct bp_location *loc, **locp_tmp;
   int found = 0;
 
-  ALL_BREAKPOINTS (b)
+  current_program_space->executing_startup = 1;
+
+  ALL_BP_LOCATIONS (loc, locp_tmp)
     {
-      if (b->pspace != current_program_space)
+      if (loc->pspace != current_program_space)
 	continue;
 
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && breakpoint_enabled (b))
+      if ((loc->owner->type == bp_breakpoint
+	   || loc->owner->type == bp_hardware_breakpoint)
+	  && breakpoint_enabled (loc->owner))
 	{
-	  b->enable_state = bp_startup_disabled;
 	  found = 1;
+	  break;
 	}
     }
 
   if (found)
     update_global_location_list (0);
-
-  current_program_space->executing_startup = 1;
 }
 
 void
 enable_breakpoints_after_startup (void)
 {
-  struct breakpoint *b;
+  struct bp_location *loc, **locp_tmp;
   int found = 0;
 
   current_program_space->executing_startup = 0;
 
-  ALL_BREAKPOINTS (b)
+  ALL_BP_LOCATIONS (loc, locp_tmp)
     {
-      if (b->pspace != current_program_space)
+      if (loc->pspace != current_program_space)
 	continue;
 
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && b->enable_state == bp_startup_disabled)
+      if ((loc->owner->type == bp_breakpoint
+	   || loc->owner->type == bp_hardware_breakpoint)
+	  && breakpoint_enabled (loc->owner))
 	{
-	  b->enable_state = bp_enabled;
 	  found = 1;
+	  break;
 	}
     }
 
@@ -7215,7 +7219,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	  b->ignore_count = ignore_count;
 	  b->enable_state = enabled ? bp_enabled : bp_disabled;
 	  b->disposition = disposition;
-	  b->pspace = sals.sals[0].pspace;
 
 	  if (type == bp_static_tracepoint)
 	    {
@@ -7256,11 +7259,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 			   "tracepoint marker to probe"));
 	    }
 
-	  if (enabled && b->pspace->executing_startup
-	      && (b->type == bp_breakpoint
-		  || b->type == bp_hardware_breakpoint))
-	    b->enable_state = bp_startup_disabled;
-
 	  loc = b->loc;
 	}
       else
@@ -7980,14 +7978,10 @@ create_breakpoint (struct gdbarch *gdbarch,
       b->disposition = tempflag ? disp_del : disp_donttouch;
       b->condition_not_parsed = 1;
       b->enable_state = enabled ? bp_enabled : bp_disabled;
-      b->pspace = current_program_space;
+      if (type_wanted != bp_breakpoint && type_wanted != bp_hardware_breakpoint)
+	b->pspace = current_program_space;
       b->py_bp_object = NULL;
 
-      if (enabled && b->pspace->executing_startup
-	  && (b->type == bp_breakpoint
-	      || b->type == bp_hardware_breakpoint))
-	b->enable_state = bp_startup_disabled;
-
       if (!internal)
         /* Do not mention breakpoints with a negative number, 
 	   but do notify observers.  */
@@ -10607,13 +10601,8 @@ update_global_location_list (int should_insert)
       struct breakpoint *b = loc->owner;
       struct bp_location **loc_first_p;
 
-      if (b->enable_state == bp_disabled
-	  || b->enable_state == bp_call_disabled
-	  || b->enable_state == bp_startup_disabled
-	  || !loc->enabled
-	  || loc->shlib_disabled
-	  || !breakpoint_address_is_meaningful (b)
-	  || is_tracepoint (b))
+      if (!should_be_inserted (loc)
+	  || !breakpoint_address_is_meaningful (b))
 	continue;
 
       /* Permanent breakpoint should always be inserted.  */
@@ -10891,10 +10880,6 @@ static struct breakpoint_ops base_breakpoint_ops =
 static void
 bkpt_re_set (struct breakpoint *b)
 {
-  /* Do not attempt to re-set breakpoints disabled during startup.  */
-  if (b->enable_state == bp_startup_disabled)
-    return;
-
   /* FIXME: is this still reachable?  */
   if (b->addr_string == NULL)
     {
@@ -11825,6 +11810,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
       if (e.error == NOT_FOUND_ERROR
 	  && (b->condition_not_parsed 
 	      || (b->loc && b->loc->shlib_disabled)
+	      || (b->loc && b->loc->pspace->executing_startup)
 	      || b->enable_state == bp_disabled))
 	not_found_and_ok = 1;
 
@@ -11913,7 +11899,8 @@ prepare_re_set_context (struct breakpoint *b)
 
   input_radix = b->input_radix;
   cleanups = save_current_space_and_thread ();
-  switch_to_program_space_and_thread (b->pspace);
+  if (b->pspace != NULL)
+    switch_to_program_space_and_thread (b->pspace);
   set_language (b->language);
 
   return cleanups;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fe381df..94e324a 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -186,14 +186,6 @@ enum enable_state
 			    automatically enabled and reset when the
 			    call "lands" (either completes, or stops
 			    at another eventpoint).  */
-    bp_startup_disabled, /* The eventpoint has been disabled during
-			    inferior startup.  This is necessary on
-			    some targets where the main executable
-			    will get relocated during startup, making
-			    breakpoint addresses invalid.  The
-			    eventpoint will be automatically enabled
-			    and reset once inferior startup is
-			    complete.  */
     bp_permanent	 /* There is a breakpoint instruction
 			    hard-wired into the target's code.  Don't
 			    try to write another breakpoint
@@ -571,7 +563,9 @@ struct breakpoint
        equals this.  */
     struct frame_id frame_id;
 
-    /* The program space used to set the breakpoint.  */
+    /* The program space used to set the breakpoint.  This is only set
+       for breakpoints which are specific to a program space; for
+       ordinary breakpoints this is NULL.  */
     struct program_space *pspace;
 
     /* String we used to set the breakpoint (malloc'd).  */
diff --git a/gdb/elfread.c b/gdb/elfread.c
index a309a2c..067c77f 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1032,7 +1032,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
     }
   gdb_assert (b->type == bp_gnu_ifunc_resolver);
 
-  gdb_assert (current_program_space == b->pspace);
+  gdb_assert (current_program_space == b->pspace || b->pspace == NULL);
   elf_gnu_ifunc_record_cache (b->addr_string, resolved_pc);
 
   sal = find_pc_line (resolved_pc, 0);
-- 
1.7.6.4

Re: RFC: don't set the pspace on ordinary breakpoints

[Pedro Alves]

On Tuesday 08 November 2011 20:23:13, Tom Tromey wrote:
> >>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
> 
> Tom> I think I will change linespec to ignore program spaces in this state.
> 
> The appended has two parts: the linespec.c change, relative to the big
> patch I posted, and the breakpoint.c change I think is needed too.  Only
> the latter makes sense in the context of this thread.
> 
> I *think* the should_be_inserted change is all that was really needed,
> after re-reading all the messages in this thread.  Let me know what you
> think.

I think so, but it's hard to tell, given that the dependency
on the linespec.c changes.

E.g.,

> @@ -1327,6 +1329,12 @@ decode_indirect (struct linespec_state *self, char **argptr)
>    CORE_ADDR pc;
>    char *initial = *argptr;
>    
> +  if (current_program_space->executing_startup)
> +    /* The error message doesn't really matter, because this case
> +       should only hit during breakpoint reset.  */
> +    throw_error (NOT_FOUND_ERROR, _("cannot evaluate expressions while "
> +                                   "program space is in startup"));

Why is is okay to look at current_program_space here, if you're
iterating over pspaces elsewhere?

Any chance we can have a standalone patch for just the
startup-disabled changes?  We'd need something like my previous
suggestion in bkpt_re_set (even if we'd remain buggy WRT
multi-process).

-- 
Pedro Alves

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

>> @@ -1327,6 +1329,12 @@ decode_indirect (struct linespec_state *self,
>> char **argptr)
>> CORE_ADDR pc;
>> char *initial = *argptr;
>> 
>> +  if (current_program_space->executing_startup)
>> +    /* The error message doesn't really matter, because this case
>> +       should only hit during breakpoint reset.  */
>> +    throw_error (NOT_FOUND_ERROR, _("cannot evaluate expressions while "
>> +                                   "program space is in startup"));

Pedro> Why is is okay to look at current_program_space here, if you're
Pedro> iterating over pspaces elsewhere?

decode_indirect is for 'break *EXPRESSION', which is only evaluated in
the current context in the current program space.

Pedro> Any chance we can have a standalone patch for just the
Pedro> startup-disabled changes?  We'd need something like my previous
Pedro> suggestion in bkpt_re_set (even if we'd remain buggy WRT
Pedro> multi-process).

Sure, I will do that.

Tom

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

Pedro> Any chance we can have a standalone patch for just the
Pedro> startup-disabled changes?  We'd need something like my previous
Pedro> suggestion in bkpt_re_set (even if we'd remain buggy WRT
Pedro> multi-process).

Tom> Sure, I will do that.

Here it is.

Built and regtested on x86-64 F15.

Tom

2011-10-28  Tom Tromey  <tromey@redhat.com>

	* breakpoint.h (enum enable_state) <bp_startup_disabled>: Remove.
	* breakpoint.c (should_be_inserted): Explicitly check if program
	space is executing startup.
	(describe_other_breakpoints): Update.
	(disable_breakpoints_before_startup): Change executing_startup
	earlier.  Use location's pspace.
	(enable_breakpoints_after_startup): Likewise.
	(init_breakpoint_sal): Don't use bp_startup_disabled.
	(create_breakpoint): Don't use bp_startup_disabled.
	(update_global_location_list): Use should_be_inserted.
	(bkpt_re_set): Update.

From c8c42dffe736323958542623c1ca5e61db923a95 Mon Sep 17 00:00:00 2001
From: Tom Tromey <tromey@redhat.com>
Date: Thu, 10 Nov 2011 08:55:34 -0700
Subject: [PATCH 1/3] remove bp_startup_disabled

---
 gdb/ChangeLog    |   14 ++++++++++++
 gdb/breakpoint.c |   61 +++++++++++++++++++++--------------------------------
 gdb/breakpoint.h |    8 -------
 3 files changed, 38 insertions(+), 45 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8c98bef..ec45335 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1568,6 +1568,9 @@ should_be_inserted (struct bp_location *bl)
   if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
     return 0;
 
+  if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
+    return 0;
+
   /* This is set for example, when we're attached to the parent of a
      vfork, and have detached from the child.  The child is running
      free, and we expect it to do an exec or exit, at which point the
@@ -5313,8 +5316,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 	      printf_filtered (" (thread %d)", b->thread);
 	    printf_filtered ("%s%s ",
 			     ((b->enable_state == bp_disabled
-			       || b->enable_state == bp_call_disabled
-			       || b->enable_state == bp_startup_disabled)
+			       || b->enable_state == bp_call_disabled)
 			      ? " (disabled)"
 			      : b->enable_state == bp_permanent 
 			      ? " (permanent)"
@@ -6933,48 +6935,48 @@ enable_watchpoints_after_interactive_call_stop (void)
 void
 disable_breakpoints_before_startup (void)
 {
-  struct breakpoint *b;
+  struct bp_location *loc, **locp_tmp;
   int found = 0;
 
-  ALL_BREAKPOINTS (b)
+  current_program_space->executing_startup = 1;
+
+  ALL_BP_LOCATIONS (loc, locp_tmp)
     {
-      if (b->pspace != current_program_space)
+      if (loc->pspace != current_program_space)
 	continue;
 
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && breakpoint_enabled (b))
+      if ((loc->owner->type == bp_breakpoint
+	   || loc->owner->type == bp_hardware_breakpoint)
+	  && breakpoint_enabled (loc->owner))
 	{
-	  b->enable_state = bp_startup_disabled;
 	  found = 1;
+	  break;
 	}
     }
 
   if (found)
     update_global_location_list (0);
-
-  current_program_space->executing_startup = 1;
 }
 
 void
 enable_breakpoints_after_startup (void)
 {
-  struct breakpoint *b;
+  struct bp_location *loc, **locp_tmp;
   int found = 0;
 
   current_program_space->executing_startup = 0;
 
-  ALL_BREAKPOINTS (b)
+  ALL_BP_LOCATIONS (loc, locp_tmp)
     {
-      if (b->pspace != current_program_space)
+      if (loc->pspace != current_program_space)
 	continue;
 
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && b->enable_state == bp_startup_disabled)
+      if ((loc->owner->type == bp_breakpoint
+	   || loc->owner->type == bp_hardware_breakpoint)
+	  && breakpoint_enabled (loc->owner))
 	{
-	  b->enable_state = bp_enabled;
 	  found = 1;
+	  break;
 	}
     }
 
@@ -7256,11 +7258,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 			   "tracepoint marker to probe"));
 	    }
 
-	  if (enabled && b->pspace->executing_startup
-	      && (b->type == bp_breakpoint
-		  || b->type == bp_hardware_breakpoint))
-	    b->enable_state = bp_startup_disabled;
-
 	  loc = b->loc;
 	}
       else
@@ -7983,11 +7980,6 @@ create_breakpoint (struct gdbarch *gdbarch,
       b->pspace = current_program_space;
       b->py_bp_object = NULL;
 
-      if (enabled && b->pspace->executing_startup
-	  && (b->type == bp_breakpoint
-	      || b->type == bp_hardware_breakpoint))
-	b->enable_state = bp_startup_disabled;
-
       if (!internal)
         /* Do not mention breakpoints with a negative number, 
 	   but do notify observers.  */
@@ -10607,13 +10599,8 @@ update_global_location_list (int should_insert)
       struct breakpoint *b = loc->owner;
       struct bp_location **loc_first_p;
 
-      if (b->enable_state == bp_disabled
-	  || b->enable_state == bp_call_disabled
-	  || b->enable_state == bp_startup_disabled
-	  || !loc->enabled
-	  || loc->shlib_disabled
-	  || !breakpoint_address_is_meaningful (b)
-	  || is_tracepoint (b))
+      if (!should_be_inserted (loc)
+	  || !breakpoint_address_is_meaningful (b))
 	continue;
 
       /* Permanent breakpoint should always be inserted.  */
@@ -10891,8 +10878,7 @@ static struct breakpoint_ops base_breakpoint_ops =
 static void
 bkpt_re_set (struct breakpoint *b)
 {
-  /* Do not attempt to re-set breakpoints disabled during startup.  */
-  if (b->enable_state == bp_startup_disabled)
+  if (current_program_space->executing_startup)
     return;
 
   /* FIXME: is this still reachable?  */
@@ -11825,6 +11811,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
       if (e.error == NOT_FOUND_ERROR
 	  && (b->condition_not_parsed 
 	      || (b->loc && b->loc->shlib_disabled)
+	      || (b->loc && b->loc->pspace->executing_startup)
 	      || b->enable_state == bp_disabled))
 	not_found_and_ok = 1;
 
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fe381df..d9060cf 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -186,14 +186,6 @@ enum enable_state
 			    automatically enabled and reset when the
 			    call "lands" (either completes, or stops
 			    at another eventpoint).  */
-    bp_startup_disabled, /* The eventpoint has been disabled during
-			    inferior startup.  This is necessary on
-			    some targets where the main executable
-			    will get relocated during startup, making
-			    breakpoint addresses invalid.  The
-			    eventpoint will be automatically enabled
-			    and reset once inferior startup is
-			    complete.  */
     bp_permanent	 /* There is a breakpoint instruction
 			    hard-wired into the target's code.  Don't
 			    try to write another breakpoint
-- 
1.7.6.4

Re: RFC: don't set the pspace on ordinary breakpoints

[Pedro Alves]

On Thursday 10 November 2011 16:52:38, Tom Tromey wrote:
> @@ -11825,6 +11811,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
>        if (e.error == NOT_FOUND_ERROR
>           && (b->condition_not_parsed 
>               || (b->loc && b->loc->shlib_disabled)
> +             || (b->loc && b->loc->pspace->executing_startup)
>               || b->enable_state == bp_disabled))
>         not_found_and_ok = 1;

Is this necessary in this patch?  I though this was related to
the new errors thrown from the linespec changes.

Otherwise looks good to me.

-- 
Pedro Alves

Re: RFC: don't set the pspace on ordinary breakpoints

[Ulrich Weigand]

Tom Tromey wrote:

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 8c98bef..ec45335 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1568,6 +1568,9 @@ should_be_inserted (struct bp_location *bl)
>    if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
>      return 0;
>  
> +  if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
> +    return 0;
> +
>    /* This is set for example, when we're attached to the parent of a
>       vfork, and have detached from the child.  The child is running
>       free, and we expect it to do an exec or exit, at which point the
> @@ -5313,8 +5316,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
>  	      printf_filtered (" (thread %d)", b->thread);
>  	    printf_filtered ("%s%s ",
>  			     ((b->enable_state == bp_disabled
> -			       || b->enable_state == bp_call_disabled
> -			       || b->enable_state == bp_startup_disabled)
> +			       || b->enable_state == bp_call_disabled)
>  			      ? " (disabled)"
>  			      : b->enable_state == bp_permanent 
>  			      ? " (permanent)"
> @@ -6933,48 +6935,48 @@ enable_watchpoints_after_interactive_call_stop (void)
>  void
>  disable_breakpoints_before_startup (void)
>  {
> -  struct breakpoint *b;
> +  struct bp_location *loc, **locp_tmp;
>    int found = 0;
>  
> -  ALL_BREAKPOINTS (b)
> +  current_program_space->executing_startup = 1;
> +
> +  ALL_BP_LOCATIONS (loc, locp_tmp)
>      {
> -      if (b->pspace != current_program_space)
> +      if (loc->pspace != current_program_space)
>  	continue;
>  
> -      if ((b->type == bp_breakpoint
> -	   || b->type == bp_hardware_breakpoint)
> -	  && breakpoint_enabled (b))
> +      if ((loc->owner->type == bp_breakpoint
> +	   || loc->owner->type == bp_hardware_breakpoint)


Should this now use the same condition as above,
i.e. user_breakpoint_p (b) ?

> +	  && breakpoint_enabled (loc->owner))
>  	{
> -	  b->enable_state = bp_startup_disabled;
>  	  found = 1;
> +	  break;
>  	}
>      }
>  
>    if (found)
>      update_global_location_list (0);

Or maybe rather, since the sole remaining function of that loop
is to sometimes skip the update_global_location_list, which doesn't
appear to have any reason except performance tuning (which is
really unnecessary here as this routine is called extremely rarely)
-- why not just remove the whole loop and just call
update_global_location_list unconditionally?

Bye,
Ulrich

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

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> Or maybe rather, since the sole remaining function of that loop
Ulrich> is to sometimes skip the update_global_location_list, which doesn't
Ulrich> appear to have any reason except performance tuning (which is
Ulrich> really unnecessary here as this routine is called extremely rarely)
Ulrich> -- why not just remove the whole loop and just call
Ulrich> update_global_location_list unconditionally?

Sounds good to me.  I removed the loop and will retest shortly.

Tom

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

Tom> Sounds good to me.  I removed the loop and will retest shortly.

Here is a new version that removes the extra hunk that Pedro noticed,
and that removes the loop from disable_breakpoints_before_startup.

Now I wonder whether also removing the loop from
enable_breakpoints_after_startup is advisable.  It seems less problematic,
though.

Built and regtested on x86-64 F15.

Tom

2011-10-28  Tom Tromey  <tromey@redhat.com>

	* breakpoint.h (enum enable_state) <bp_startup_disabled>: Remove.
	* breakpoint.c (should_be_inserted): Explicitly check if program
	space is executing startup.
	(describe_other_breakpoints): Update.
	(disable_breakpoints_before_startup): Change executing_startup
	earlier.  Remove loop.
	(enable_breakpoints_after_startup): Change executing_startup
	earlier.  Use location's pspace.
	(init_breakpoint_sal): Don't use bp_startup_disabled.
	(create_breakpoint): Don't use bp_startup_disabled.
	(update_global_location_list): Use should_be_inserted.
	(bkpt_re_set): Update.

From 11d2f10808089e9501760e7c486af6b9b72b04ab Mon Sep 17 00:00:00 2001
From: Tom Tromey <tromey@redhat.com>
Date: Thu, 10 Nov 2011 08:55:34 -0700
Subject: [PATCH 1/3] remove bp_startup_disabled

---
 gdb/ChangeLog    |   15 ++++++++++++
 gdb/breakpoint.c |   63 ++++++++++++-----------------------------------------
 gdb/breakpoint.h |    8 ------
 3 files changed, 30 insertions(+), 56 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8c98bef..17c5b6c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1568,6 +1568,9 @@ should_be_inserted (struct bp_location *bl)
   if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
     return 0;
 
+  if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
+    return 0;
+
   /* This is set for example, when we're attached to the parent of a
      vfork, and have detached from the child.  The child is running
      free, and we expect it to do an exec or exit, at which point the
@@ -5313,8 +5316,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 	      printf_filtered (" (thread %d)", b->thread);
 	    printf_filtered ("%s%s ",
 			     ((b->enable_state == bp_disabled
-			       || b->enable_state == bp_call_disabled
-			       || b->enable_state == bp_startup_disabled)
+			       || b->enable_state == bp_call_disabled)
 			      ? " (disabled)"
 			      : b->enable_state == bp_permanent 
 			      ? " (permanent)"
@@ -6933,48 +6935,29 @@ enable_watchpoints_after_interactive_call_stop (void)
 void
 disable_breakpoints_before_startup (void)
 {
-  struct breakpoint *b;
-  int found = 0;
-
-  ALL_BREAKPOINTS (b)
-    {
-      if (b->pspace != current_program_space)
-	continue;
-
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && breakpoint_enabled (b))
-	{
-	  b->enable_state = bp_startup_disabled;
-	  found = 1;
-	}
-    }
-
-  if (found)
-    update_global_location_list (0);
-
   current_program_space->executing_startup = 1;
+  update_global_location_list (0);
 }
 
 void
 enable_breakpoints_after_startup (void)
 {
-  struct breakpoint *b;
+  struct bp_location *loc, **locp_tmp;
   int found = 0;
 
   current_program_space->executing_startup = 0;
 
-  ALL_BREAKPOINTS (b)
+  ALL_BP_LOCATIONS (loc, locp_tmp)
     {
-      if (b->pspace != current_program_space)
+      if (loc->pspace != current_program_space)
 	continue;
 
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && b->enable_state == bp_startup_disabled)
+      if ((loc->owner->type == bp_breakpoint
+	   || loc->owner->type == bp_hardware_breakpoint)
+	  && breakpoint_enabled (loc->owner))
 	{
-	  b->enable_state = bp_enabled;
 	  found = 1;
+	  break;
 	}
     }
 
@@ -7256,11 +7239,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 			   "tracepoint marker to probe"));
 	    }
 
-	  if (enabled && b->pspace->executing_startup
-	      && (b->type == bp_breakpoint
-		  || b->type == bp_hardware_breakpoint))
-	    b->enable_state = bp_startup_disabled;
-
 	  loc = b->loc;
 	}
       else
@@ -7983,11 +7961,6 @@ create_breakpoint (struct gdbarch *gdbarch,
       b->pspace = current_program_space;
       b->py_bp_object = NULL;
 
-      if (enabled && b->pspace->executing_startup
-	  && (b->type == bp_breakpoint
-	      || b->type == bp_hardware_breakpoint))
-	b->enable_state = bp_startup_disabled;
-
       if (!internal)
         /* Do not mention breakpoints with a negative number, 
 	   but do notify observers.  */
@@ -10607,13 +10580,8 @@ update_global_location_list (int should_insert)
       struct breakpoint *b = loc->owner;
       struct bp_location **loc_first_p;
 
-      if (b->enable_state == bp_disabled
-	  || b->enable_state == bp_call_disabled
-	  || b->enable_state == bp_startup_disabled
-	  || !loc->enabled
-	  || loc->shlib_disabled
-	  || !breakpoint_address_is_meaningful (b)
-	  || is_tracepoint (b))
+      if (!should_be_inserted (loc)
+	  || !breakpoint_address_is_meaningful (b))
 	continue;
 
       /* Permanent breakpoint should always be inserted.  */
@@ -10891,8 +10859,7 @@ static struct breakpoint_ops base_breakpoint_ops =
 static void
 bkpt_re_set (struct breakpoint *b)
 {
-  /* Do not attempt to re-set breakpoints disabled during startup.  */
-  if (b->enable_state == bp_startup_disabled)
+  if (current_program_space->executing_startup)
     return;
 
   /* FIXME: is this still reachable?  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fe381df..d9060cf 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -186,14 +186,6 @@ enum enable_state
 			    automatically enabled and reset when the
 			    call "lands" (either completes, or stops
 			    at another eventpoint).  */
-    bp_startup_disabled, /* The eventpoint has been disabled during
-			    inferior startup.  This is necessary on
-			    some targets where the main executable
-			    will get relocated during startup, making
-			    breakpoint addresses invalid.  The
-			    eventpoint will be automatically enabled
-			    and reset once inferior startup is
-			    complete.  */
     bp_permanent	 /* There is a breakpoint instruction
 			    hard-wired into the target's code.  Don't
 			    try to write another breakpoint
-- 
1.7.6.4

Re: RFC: don't set the pspace on ordinary breakpoints

[Ulrich Weigand]

Tom Tromey wrote:

> Now I wonder whether also removing the loop from
> enable_breakpoints_after_startup is advisable.  It seems less problematic,
> though.

Ah, yes, I intended my comment to apply to both loops in the same way.

The same reasons apply for the loop in enable_breakpoints_after_startup,
and just for consistency and simplicity I'd prefer to remove that second
loop as well ...

Bye,
Ulrich

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

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

Ulrich> Ah, yes, I intended my comment to apply to both loops in the same way.

Ulrich> The same reasons apply for the loop in enable_breakpoints_after_startup,
Ulrich> and just for consistency and simplicity I'd prefer to remove that second
Ulrich> loop as well ...

No problem, here it is.

Tom

2011-10-28  Tom Tromey  <tromey@redhat.com>

	* breakpoint.h (enum enable_state) <bp_startup_disabled>: Remove.
	* breakpoint.c (should_be_inserted): Explicitly check if program
	space is executing startup.
	(describe_other_breakpoints): Update.
	(disable_breakpoints_before_startup): Change executing_startup
	earlier.  Remove loop.
	(enable_breakpoints_after_startup): Likewise.
	(init_breakpoint_sal): Don't use bp_startup_disabled.
	(create_breakpoint): Don't use bp_startup_disabled.
	(update_global_location_list): Use should_be_inserted.
	(bkpt_re_set): Update.

From 7ece16483d520b841c211ab1d468e2efed06cd27 Mon Sep 17 00:00:00 2001
From: Tom Tromey <tromey@redhat.com>
Date: Thu, 10 Nov 2011 08:55:34 -0700
Subject: [PATCH 1/3] remove bp_startup_disabled

---
 gdb/ChangeLog    |   14 ++++++++++
 gdb/breakpoint.c |   70 +++++++-----------------------------------------------
 gdb/breakpoint.h |    8 ------
 3 files changed, 23 insertions(+), 69 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8c98bef..f2e1c57 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1568,6 +1568,9 @@ should_be_inserted (struct bp_location *bl)
   if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
     return 0;
 
+  if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
+    return 0;
+
   /* This is set for example, when we're attached to the parent of a
      vfork, and have detached from the child.  The child is running
      free, and we expect it to do an exec or exit, at which point the
@@ -5313,8 +5316,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 	      printf_filtered (" (thread %d)", b->thread);
 	    printf_filtered ("%s%s ",
 			     ((b->enable_state == bp_disabled
-			       || b->enable_state == bp_call_disabled
-			       || b->enable_state == bp_startup_disabled)
+			       || b->enable_state == bp_call_disabled)
 			      ? " (disabled)"
 			      : b->enable_state == bp_permanent 
 			      ? " (permanent)"
@@ -6933,53 +6935,15 @@ enable_watchpoints_after_interactive_call_stop (void)
 void
 disable_breakpoints_before_startup (void)
 {
-  struct breakpoint *b;
-  int found = 0;
-
-  ALL_BREAKPOINTS (b)
-    {
-      if (b->pspace != current_program_space)
-	continue;
-
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && breakpoint_enabled (b))
-	{
-	  b->enable_state = bp_startup_disabled;
-	  found = 1;
-	}
-    }
-
-  if (found)
-    update_global_location_list (0);
-
   current_program_space->executing_startup = 1;
+  update_global_location_list (0);
 }
 
 void
 enable_breakpoints_after_startup (void)
 {
-  struct breakpoint *b;
-  int found = 0;
-
   current_program_space->executing_startup = 0;
-
-  ALL_BREAKPOINTS (b)
-    {
-      if (b->pspace != current_program_space)
-	continue;
-
-      if ((b->type == bp_breakpoint
-	   || b->type == bp_hardware_breakpoint)
-	  && b->enable_state == bp_startup_disabled)
-	{
-	  b->enable_state = bp_enabled;
-	  found = 1;
-	}
-    }
-
-  if (found)
-    breakpoint_re_set ();
+  breakpoint_re_set ();
 }
 
 
@@ -7256,11 +7220,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 			   "tracepoint marker to probe"));
 	    }
 
-	  if (enabled && b->pspace->executing_startup
-	      && (b->type == bp_breakpoint
-		  || b->type == bp_hardware_breakpoint))
-	    b->enable_state = bp_startup_disabled;
-
 	  loc = b->loc;
 	}
       else
@@ -7983,11 +7942,6 @@ create_breakpoint (struct gdbarch *gdbarch,
       b->pspace = current_program_space;
       b->py_bp_object = NULL;
 
-      if (enabled && b->pspace->executing_startup
-	  && (b->type == bp_breakpoint
-	      || b->type == bp_hardware_breakpoint))
-	b->enable_state = bp_startup_disabled;
-
       if (!internal)
         /* Do not mention breakpoints with a negative number, 
 	   but do notify observers.  */
@@ -10607,13 +10561,8 @@ update_global_location_list (int should_insert)
       struct breakpoint *b = loc->owner;
       struct bp_location **loc_first_p;
 
-      if (b->enable_state == bp_disabled
-	  || b->enable_state == bp_call_disabled
-	  || b->enable_state == bp_startup_disabled
-	  || !loc->enabled
-	  || loc->shlib_disabled
-	  || !breakpoint_address_is_meaningful (b)
-	  || is_tracepoint (b))
+      if (!should_be_inserted (loc)
+	  || !breakpoint_address_is_meaningful (b))
 	continue;
 
       /* Permanent breakpoint should always be inserted.  */
@@ -10891,8 +10840,7 @@ static struct breakpoint_ops base_breakpoint_ops =
 static void
 bkpt_re_set (struct breakpoint *b)
 {
-  /* Do not attempt to re-set breakpoints disabled during startup.  */
-  if (b->enable_state == bp_startup_disabled)
+  if (current_program_space->executing_startup)
     return;
 
   /* FIXME: is this still reachable?  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fe381df..d9060cf 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -186,14 +186,6 @@ enum enable_state
 			    automatically enabled and reset when the
 			    call "lands" (either completes, or stops
 			    at another eventpoint).  */
-    bp_startup_disabled, /* The eventpoint has been disabled during
-			    inferior startup.  This is necessary on
-			    some targets where the main executable
-			    will get relocated during startup, making
-			    breakpoint addresses invalid.  The
-			    eventpoint will be automatically enabled
-			    and reset once inferior startup is
-			    complete.  */
     bp_permanent	 /* There is a breakpoint instruction
 			    hard-wired into the target's code.  Don't
 			    try to write another breakpoint
-- 
1.7.6.4

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

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

Tom> No problem, here it is.

Here is the split off part of the patch.

I plan to put this in with the rest of the ambiguous linespec change.

Tom

2011-11-10  Tom Tromey  <tromey@redhat.com>

	* elfread.c (elf_gnu_ifunc_resolver_return_stop): Allow
	breakpoint's pspace to be NULL.
	* breakpoint.h (struct breakpoint) <pspace>: Update comment.
	* breakpoint.c (init_raw_breakpoint): Conditionally set
	breakpoint's pspace.
	(init_breakpoint_sal): Don't set breakpoint's pspace.
	(prepare_re_set_context): Conditionally switch program space.
	(addr_string_to_sals): Check executing_startup on location's
	program space.

From 9930b7e45aed489de6055ed960c13aff1bd8aff9 Mon Sep 17 00:00:00 2001
From: Tom Tromey <tromey@redhat.com>
Date: Thu, 10 Nov 2011 08:56:39 -0700
Subject: [PATCH 2/4] make pspace optional

---
 gdb/ChangeLog    |   12 ++++++++++++
 gdb/breakpoint.c |   19 ++++++++++---------
 gdb/breakpoint.h |    4 +++-
 gdb/elfread.c    |    2 +-
 4 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f2e1c57..01ca912 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5792,9 +5792,11 @@ init_raw_breakpoint (struct breakpoint *b, struct gdbarch *gdbarch,
   b->loc->address = adjusted_address;
   b->loc->pspace = sal.pspace;
 
-  /* Store the program space that was used to set the breakpoint, for
-     breakpoint resetting.  */
-  b->pspace = sal.pspace;
+  /* Store the program space that was used to set the breakpoint,
+     except for ordinary breakpoints, which are independent of the
+     program space.  */
+  if (bptype != bp_breakpoint && bptype != bp_hardware_breakpoint)
+    b->pspace = sal.pspace;
 
   if (sal.symtab == NULL)
     b->source_file = NULL;
@@ -7179,7 +7181,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	  b->ignore_count = ignore_count;
 	  b->enable_state = enabled ? bp_enabled : bp_disabled;
 	  b->disposition = disposition;
-	  b->pspace = sals.sals[0].pspace;
 
 	  if (type == bp_static_tracepoint)
 	    {
@@ -7939,7 +7940,8 @@ create_breakpoint (struct gdbarch *gdbarch,
       b->disposition = tempflag ? disp_del : disp_donttouch;
       b->condition_not_parsed = 1;
       b->enable_state = enabled ? bp_enabled : bp_disabled;
-      b->pspace = current_program_space;
+      if (type_wanted != bp_breakpoint && type_wanted != bp_hardware_breakpoint)
+	b->pspace = current_program_space;
       b->py_bp_object = NULL;
 
       if (!internal)
@@ -10840,9 +10842,6 @@ static struct breakpoint_ops base_breakpoint_ops =
 static void
 bkpt_re_set (struct breakpoint *b)
 {
-  if (current_program_space->executing_startup)
-    return;
-
   /* FIXME: is this still reachable?  */
   if (b->addr_string == NULL)
     {
@@ -11773,6 +11772,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
       if (e.error == NOT_FOUND_ERROR
 	  && (b->condition_not_parsed 
 	      || (b->loc && b->loc->shlib_disabled)
+	      || (b->loc && b->loc->pspace->executing_startup)
 	      || b->enable_state == bp_disabled))
 	not_found_and_ok = 1;
 
@@ -11861,7 +11861,8 @@ prepare_re_set_context (struct breakpoint *b)
 
   input_radix = b->input_radix;
   cleanups = save_current_space_and_thread ();
-  switch_to_program_space_and_thread (b->pspace);
+  if (b->pspace != NULL)
+    switch_to_program_space_and_thread (b->pspace);
   set_language (b->language);
 
   return cleanups;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index d9060cf..94e324a 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -563,7 +563,9 @@ struct breakpoint
        equals this.  */
     struct frame_id frame_id;
 
-    /* The program space used to set the breakpoint.  */
+    /* The program space used to set the breakpoint.  This is only set
+       for breakpoints which are specific to a program space; for
+       ordinary breakpoints this is NULL.  */
     struct program_space *pspace;
 
     /* String we used to set the breakpoint (malloc'd).  */
diff --git a/gdb/elfread.c b/gdb/elfread.c
index a309a2c..067c77f 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1032,7 +1032,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
     }
   gdb_assert (b->type == bp_gnu_ifunc_resolver);
 
-  gdb_assert (current_program_space == b->pspace);
+  gdb_assert (current_program_space == b->pspace || b->pspace == NULL);
   elf_gnu_ifunc_record_cache (b->addr_string, resolved_pc);
 
   sal = find_pc_line (resolved_pc, 0);
-- 
1.7.6.4

Re: RFC: don't set the pspace on ordinary breakpoints

[Ulrich Weigand]

Tom Tromey wrote:
> Ulrich> Ah, yes, I intended my comment to apply to both loops in the same way.
> 
> Ulrich> The same reasons apply for the loop in enable_breakpoints_after_startup,
> Ulrich> and just for consistency and simplicity I'd prefer to remove that second
> Ulrich> loop as well ...
> 
> No problem, here it is.

Sorry for the delay; I wanted to test your patch on Cell to make sure it doesn't
regress in the scenario bp_startup_disabled was introduced to fix.

Unfortunately, I've been blocked by a number of SPU regressions that make
results on Cell currently bad anyway, see e.g.
http://sourceware.org/ml/gdb-patches/2011-11/msg00437.html
http://sourceware.org/ml/gdb-patches/2011-11/msg00439.html

I'm still planning on doing the run once SPU is stable again.

Bye,
Ulrich

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

Re: RFC: don't set the pspace on ordinary breakpoints

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> Sorry for the delay; I wanted to test your patch on Cell to make
Ulrich> sure it doesn't regress in the scenario bp_startup_disabled was
Ulrich> introduced to fix.

It is no problem.

Ulrich> Unfortunately, I've been blocked by a number of SPU regressions
Ulrich> that make results on Cell currently bad anyway, see e.g.
Ulrich> http://sourceware.org/ml/gdb-patches/2011-11/msg00437.html
Ulrich> http://sourceware.org/ml/gdb-patches/2011-11/msg00439.html

Ulrich> I'm still planning on doing the run once SPU is stable again.

Ok.  I will wait for that then.

Tom

Re: RFC: don't set the pspace on ordinary breakpoints

[Ulrich Weigand]

Tom Tromey wrote:
> >>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> 
> Ulrich> Sorry for the delay; I wanted to test your patch on Cell to make
> Ulrich> sure it doesn't regress in the scenario bp_startup_disabled was
> Ulrich> introduced to fix.
> 
> It is no problem.
> 
> Ulrich> Unfortunately, I've been blocked by a number of SPU regressions
> Ulrich> that make results on Cell currently bad anyway, see e.g.
> Ulrich> http://sourceware.org/ml/gdb-patches/2011-11/msg00437.html
> Ulrich> http://sourceware.org/ml/gdb-patches/2011-11/msg00439.html
> 
> Ulrich> I'm still planning on doing the run once SPU is stable again.
> 
> Ok.  I will wait for that then.

OK, I've now completed a run with your patch in the situation where
bp_startup_disabled was necessary (stand-alone SPU executables on
a Cell/B.E. system).   Everything still works fine.

Bye,
Ulrich

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

[7.4 regression] Stand-alone Cell debugging broken (Re: RFC: don't set the pspace on ordinary breakpoints)

[Ulrich Weigand]

I wrote:
> Tom Tromey wrote:
> > >>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> > 
> > Ulrich> Sorry for the delay; I wanted to test your patch on Cell to make
> > Ulrich> sure it doesn't regress in the scenario bp_startup_disabled was
> > Ulrich> introduced to fix.
> > 
> > It is no problem.
> > 
> > Ulrich> Unfortunately, I've been blocked by a number of SPU regressions
> > Ulrich> that make results on Cell currently bad anyway, see e.g.
> > Ulrich> http://sourceware.org/ml/gdb-patches/2011-11/msg00437.html
> > Ulrich> http://sourceware.org/ml/gdb-patches/2011-11/msg00439.html
> > 
> > Ulrich> I'm still planning on doing the run once SPU is stable again.
> > 
> > Ok.  I will wait for that then.
> 
> OK, I've now completed a run with your patch in the situation where
> bp_startup_disabled was necessary (stand-alone SPU executables on
> a Cell/B.E. system).   Everything still works fine.

While this patch by itself didn't break stand-alone SPU debugging,
the whole "ambiguous linespec" series in total apparently did.

What happens is that if I set a breakpoint on "main", and then start
execution, the first time breakpoints are re-set, addr_string_to_sals
(as called from breakpoint_re_set_default) correctly recognizes that
even though "main" cannot be found at this moment, this is OK since
the address space is currently going through startup.

However, while this means that no error is thrown at this point,
addr_string_to_sals still returns an empty location list, and the
existing list of locations for the "main" breakpoint now gets thrown
away in update_breakpoint_locations.

This in turn means that the *next* time breakpoints are re-set, the
"main" breakpoint now no longer has any locations associated to it,
and therefore it is no longer recognized as being related to an
address space going through startup.

Thus, an error is thrown and the breakpoint is marked disabled, and
therefore once startup is completed and "main" now *could* be found
again, it is no longer even attempted to insert the breakpoint again.

This is a serious regression that bascially means debugging stand-
alone SPU programs with a Cell/B.E. debugger is impossible.  Thus
it would be really good to get this fixed before 7.4 is released ...

The following patch fixes the problem for me.  It hooks into an
existing mechanism to fix a similar problem for breakpoints in
shared libraries that are being unloaded:  update_breakpoint_locations
does:

  /* If there's no new locations, and all existing locations are
     pending, don't do anything.  This optimizes the common case where
     all locations are in the same shared library, that was unloaded.
     We'd like to retain the location, so that when the library is
     loaded again, we don't loose the enabled/disabled status of the
     individual locations.  */
  if (all_locations_are_pending (existing_locations) && sals.nelts == 0)
    return;

which has the effect to keep locations from such libraries present
even though the library is unloaded.  The patch below now extends
this to the executing_startup case by having all_locations_are_pending
consider locations in address spaces currently going through startup
as "pending" as well.

With this change, the breakpoint location is kept active until startup
is complete, and then replaced with the correct location.


Does this look reasonable?  Joel, would this be OK for 7.4 as well?

Thanks,
Ulrich


ChangeLog:

	* breakpoint.c (all_locations_are_pending): Consider locations
	in program spaces executing during startup pending as well.

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.633.2.2
diff -u -p -r1.633.2.2 breakpoint.c
--- gdb/breakpoint.c	23 Dec 2011 17:55:20 -0000	1.633.2.2
+++ gdb/breakpoint.c	2 Jan 2012 15:16:20 -0000
@@ -11500,7 +11500,8 @@ static int
 all_locations_are_pending (struct bp_location *loc)
 {
   for (; loc; loc = loc->next)
-    if (!loc->shlib_disabled)
+    if (!loc->shlib_disabled
+	&& !loc->pspace->executing_startup)
       return 0;
   return 1;
 }


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

Re: [7.4 regression] Stand-alone Cell debugging broken (Re: RFC: don't set the pspace on ordinary breakpoints)

[Joel Brobecker]

> This is a serious regression that bascially means debugging stand-
> alone SPU programs with a Cell/B.E. debugger is impossible.  Thus
> it would be really good to get this fixed before 7.4 is released ...

Agreed. This is a blocking regression.

> Does this look reasonable?  Joel, would this be OK for 7.4 as well?

The patch seems reasonable to me, but I have little experience
in this area. It's OK for 7.4 unless someone finds a problem
with it.

> ChangeLog:
> 
> 	* breakpoint.c (all_locations_are_pending): Consider locations
> 	in program spaces executing during startup pending as well.
-- 
Joel

Re: [7.4 regression] Stand-alone Cell debugging broken (Re: RFC: don't set the pspace on ordinary breakpoints)

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> While this patch by itself didn't break stand-alone SPU debugging,
Ulrich> the whole "ambiguous linespec" series in total apparently did.

Sorry about that.

Ulrich> Does this look reasonable?  Joel, would this be OK for 7.4 as well?

Ulrich> 	* breakpoint.c (all_locations_are_pending): Consider locations
Ulrich> 	in program spaces executing during startup pending as well.

It looks good to me.

Tom

Re: [7.4 regression] Stand-alone Cell debugging broken (Re: RFC: don't set the pspace on ordinary breakpoints)

[Ulrich Weigand]

Tom Tromey wrote:
> >>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> 
> Ulrich> While this patch by itself didn't break stand-alone SPU debugging,
> Ulrich> the whole "ambiguous linespec" series in total apparently did.
> 
> Sorry about that.
> 
> Ulrich> Does this look reasonable?  Joel, would this be OK for 7.4 as well?
> 
> Ulrich> 	* breakpoint.c (all_locations_are_pending): Consider locations
> Ulrich> 	in program spaces executing during startup pending as well.
> 
> It looks good to me.

Thanks!   I've checked this in to mainline and 7.4 now.

Bye,
Ulrich

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