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

[Jeff Johnston <jjohnstn@redhat.com>]

Daniel Jacobowitz wrote:
> 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.
>

What you do want to see so I don't waste my time on this.  As you already know, 
this routine was copied from the routine which disables shared library 
breakpoints in breakpoint.c.  Is it sufficient to just issue the warning that I 
am temporarily disabling unloaded shared library breakpoints and not spell out 
each breakpoint in turn?  I can see this as really annoying and pointless to an 
end-user if there are hundreds or thousands of breakpoints.

-- Jeff J.