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

[Pedro Alves <pedro at codesourcery dot com>]

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