Re: [RFA]: Fix for pending breakpoints in manually loaded/unloaded shlibs

[Daniel Jacobowitz <drow@false.org>]

On Wed, Aug 11, 2004 at 04:12:07PM -0400, Jeff Johnston wrote:
> Ok?

Sorry to keep picking nits; while we discuss the issue of the new
observer I went over the patch again for minor problems.

> +/* Disable any breakpoints that are in in an unloaded shared library.  Only
> +   apply to enabled breakpoints, disabled ones can just stay disabled.  */
> +
> +void
> +disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
> +{
> +  struct breakpoint *b;
> +  int disabled_shlib_breaks = 0;
> +
> +  /* See also: insert_breakpoints, under DISABLE_UNSETTABLE_BREAK. */

Two spaces after a period, please.

> +  ALL_BREAKPOINTS (b)
> +  {
> +#if defined (PC_SOLIB)

I think someone pointed out that you've #ifdef'd out the entire body of
this loop.  Might as well include the whole loop.  The #ifdef is nasty,
but that's a preexisting problem.

> +    if ((b->loc->loc_type == bp_loc_hardware_breakpoint
> +	|| b->loc->loc_type == bp_loc_software_breakpoint)
> +	&& breakpoint_enabled (b) 
> +	&& !b->loc->duplicate)
> +      {
> +	char *so_name = PC_SOLIB (b->loc->address);

While I'm ranting about preexisting problems, it would be nice if
PC_SOLIB returned the solib, instead of just its name... but enh.

> +	if (so_name 
> +	    && !strcmp (so_name, solib->so_name))
> +          {
> +	    b->enable_state = bp_shlib_disabled;
> +	    /* At this point, we cannot rely on remove_breakpoint
> +	       succeeding so we must mark the breakpoint as not inserted
> +	       to prevent future errors occurring in remove_breakpoints.  */
> +	    b->loc->inserted = 0;
> +	    if (!disabled_shlib_breaks)
> +	      {
> +		target_terminal_ours_for_output ();
> +		warning ("Temporarily disabling unloaded shared library breakpoints:");
> +	      }
> +	    disabled_shlib_breaks = 1;
> +	    warning ("breakpoint #%d ", b->number);

I think you're missing a space after the colon, in the first warning.
Also, this use of multiple warning() statements is neither i18n
friendly nor MI/GUI friendly - you may get a separate dialog box for
each.  I believe other places do this with sprintf; still not 100% i18n
friendly, but avoids the MI/GUI problems.  I can't find an example
offhand.

-- 
Daniel Jacobowitz