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

Paul Pluzhnikov
2009-03-09T23:31:13Z

[-- 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;
}
}
}