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

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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