Re: [rfc] Infrastructure to disable breakpoints during inferior startup

["Ulrich Weigand" <uweigand@de.ibm.com>]

Tom Tromey wrote:

> FWIW, I took a look at the PIE patch from the Fedora SRPM.  It has
> almost identical functions disable_breakpoints_before_startup and
> re_enable_breakpoints_at_startup.

Yes, it's the same concept, but those functions in the PIE patch have
some code that seems PIE specific (e.g. the entry point checks) that
should be moved to the caller (presumably solib-svr4.c in the PIE case)
to make the same infrastructure usable for both scenarios.
 
> There are some differences, but I don't know whether they are relevant
> or not.
> 
> The Fedora disable_breakpoints_before_startup has a check like this:
> 
> +	  if (((b->type == bp_breakpoint) ||
> +	       (b->type == bp_hardware_breakpoint)) &&
> +	      b->enable_state == bp_enabled &&
> +	      !b->loc->duplicate)
> 
> This differs from yours because it checks `loc->duplicate'.

I don't see why a breakpoint shouldn't be disabled just because its
first location has a duplicate ...  If the breakpoint has another
location, or if the duplicate gets removed later on, GDB would attempt
to insert this breakpoint and fail ...

See also the comments in disable_breakpoints_in_shlibs that explain
why the shlib_disabled flag is explicitly set also for duplicates.

> The Fedora re_enable_breakpoints_at_startup does this:
> 
> +	    /* Do not reenable the breakpoint if the shared library
> +	       is still not mapped in.  */
> +	    if (target_read_memory (b->loc->address, buf, 1) == 0)
> +	      {
> +		/*printf ("enabling breakpoint at 0x%s\n", paddr_nz(b->loc->address));*/
> +		b->enable_state = bp_enabled;
> +	      }
> 
> I have no idea about this either.  Perhaps it is something specific to
> PIE on Linux.

This seems odd to me; breakpoint disabled at startup cannot really be
within a shared library.  (Even if they were, a unmapped shared library
ought to be handled via the shlib_disabled mechanism ...)

> Ulrich> +/* Are we executing startup code?  */
> Ulrich> +static int executing_startup;
> 
> This seems like it should be a field in struct inferior.

Agreed.

> I seem to say that a lot :-).  I don't actually know .. should we be
> doing this sort of thing now, or are we waiting for Pedro's
> multi-inferior patches to land first?

I don't know; what's the timeline for Pedro's patches?

(In any case, moving this variable over to a struct inferior field
can be trivially done after Pedro's patches are merged; I'm not sure
we have to wait because of that ...)

> Ulrich> +    bp_startup_disabled,/* The eventpoint has been disabled during inferior
> 
> I think a new bp_ constant probably needs an entry in the array in
> print_one_breakpoint_location.  Otherwise if something funny happens,
> and we try to print one, gdb will get an internal error.

Unless I'm missing someting, the array in print_one_breakpoint_location
is about enum bptype member; I've added a enum enable_state member here ...

Bye,
Ulrich

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