Re: [patch] Fix a crash when displaying variables from shared library.

[Paul Pluzhnikov <ppluzhnikov@google.com>]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

On Mon, Mar 9, 2009 at 11:33 AM, Joel Brobecker <brobecker@adacore.com> wrote:

>> 2009-03-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
>>
>>           * solib.c (solib_contains_address_p): New function.
>
> Looks great to me. Go ahead and check it in.

Thanks, so committed.

And now we come back to the shlib_disable question.

With the change above, I believe it to be quite reasonable to update
breakpoint.c as attached patch does.

But doing that breaks attach.exp again:
  FAIL: gdb.base/attach.exp: attach1 detach
due to:
  warning: Temporarily disabling breakpoints for unloaded shared
library "/lib64/ld-linux-x86-64.so.2"

With current code we don't issue the warning, but only because
ld-linux has already been removed from the so_list_head, and so
solib_name_from_address() returns NULL.

It looks to me that this works purely by accident, and so I ask
again: shouldn't all types of breakpoints, and not just
bp_breakpoint and bp_hardware_breakpoint, be shlib_disabled in
disable_breakpoints_in_shlibs?

I went ahead and did this:

diff -u -p -u -r1.382 breakpoint.c
--- breakpoint.c        6 Mar 2009 18:51:05 -0000       1.382
+++ breakpoint.c        9 Mar 2009 22:55:02 -0000
@@ -4434,8 +4434,7 @@ disable_breakpoints_in_shlibs (void)
        becomes enabled, or the duplicate is removed, gdb will try to insert
        all breakpoints.  If we don't set shlib_disabled here, we'll try
        to insert those breakpoints and fail.  */
-    if (((b->type == bp_breakpoint) || (b->type == bp_hardware_breakpoint))
-       && !loc->shlib_disabled
+    if (!loc->shlib_disabled
 #ifdef PC_SOLIB
        && PC_SOLIB (loc->address)
 #else

And that did not produce any new failures on Linux/x86_64.

It appears to me that this is the logically correct thing to do.

Thanks,
-- 
Paul Pluzhnikov

[-- Attachment #2: gdb-display-crash-20090309.txt --]
[-- Type: text/plain, Size: 1768 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.382
diff -u -p -u -r1.382 breakpoint.c
--- breakpoint.c	6 Mar 2009 18:51:05 -0000	1.382
+++ breakpoint.c	9 Mar 2009 22:51:45 -0000
@@ -4470,28 +4470,21 @@ disable_breakpoints_in_unloaded_shlib (s
     struct breakpoint *b = loc->owner;
     if ((loc->loc_type == bp_loc_hardware_breakpoint
 	 || loc->loc_type == bp_loc_software_breakpoint)
-	&& !loc->shlib_disabled)
+	&& !loc->shlib_disabled
+	&& solib_contains_address_p (solib, loc->address))
       {
-#ifdef PC_SOLIB
-	char *so_name = PC_SOLIB (loc->address);
-#else
-	char *so_name = solib_name_from_address (loc->address);
-#endif
-	if (so_name && !strcmp (so_name, solib->so_name))
-          {
-	    loc->shlib_disabled = 1;
-	    /* 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.  */
-	    loc->inserted = 0;
-	    if (!disabled_shlib_breaks)
-	      {
-		target_terminal_ours_for_output ();
-		warning (_("Temporarily disabling breakpoints for unloaded shared library \"%s\""),
-			  so_name);
-	      }
-	    disabled_shlib_breaks = 1;
+	loc->shlib_disabled = 1;
+	/* 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.  */
+	loc->inserted = 0;
+	if (!disabled_shlib_breaks)
+	  {
+	    target_terminal_ours_for_output ();
+	    warning (_("Temporarily disabling breakpoints for unloaded shared library \"%s\""),
+		     solib->so_name);
 	  }
+	disabled_shlib_breaks = 1;
       }
   }
 }