gdb: fixes for code_breakpoint::disabled_by_cond logic

I spotted that the code_breakpoint::disabled_by_cond flag doesn't work
how I'd expect it too.  The flag appears to be "sticky" in some
situations; once a code_breakpoint::disabled_by_cond flag is marked
true, then, in some cases the flag wont automatically become false
again, even when you'd think it should.

The problem is in update_breakpoint_locations.  In this function,
which is called as a worker of code_breakpoint::re_set, GDB computes a
new set of locations for a breakpoint, the new locations are then
installed into the breakpoint.

However, before installing the new locations GDB attempts to copy the
bp_location::enabled and bp_location::disabled_by_cond flag from the
old locations into the new locations.

The reason for copying the ::enabled flag makes sense.  This flag is
controlled by the user.  When we create the new locations if GDB can
see that a new location is equivalent to one of the old locations, and
if the old location was disabled by the user, then the new location
should also be disabled.

However, I think the logic behind copying the ::disabled_by_cond flag
is wrong.  The disabled_by_cond flag is controlled by GDB and should
toggle automatically.  If the condition string can be parsed then the
flag should be false (b/p enabled), if the condition string can't be
parsed then the flag should be true (b/p disabled).

As we always parse the condition string in update_breakpoint_locations
before we try to copy the ::enabled flag value then the
::disabled_by_cond flag should already be correct, there's no need to
copy over the ::disabled_by_cond value from the old location.

As a concrete example, consider a b/p placed within the main
executable, but with a condition that depends on a variable within a
shared library.

When the b/p is initially created the b/p will be disabled as the
condition string will be invalid (the shared library variable isn't
available yet).

When the inferior starts the shared library is loaded and the
condition variable becomes available to GDB.  When the shared library
is loaded breakpoint_re_set is called which (eventually) calls
update_breakpoint_locations.

A new location is computed for the breakpoint and the condition string
is parsed.  As the shared library variable is now know the expression
parses correctly and ::disabled_by_cond is left false for the new
location.

But currently GDB spots that the new location is at the same address
as the old location and copies disabled_by_cond over from the old
location, which marks the b/p location as disabled.  This is not what
I would expect.

The solution is simple, don't copy over disabled_by_cond.

While writing a test I found another problem though.  The
disabled_by_cond flag doesn't get set true when it should!  This is
the exact opposite of the above.

The problem here is in solib_add which is (despite the name) called
whenever the shared library set changes, including when a shared
library is unloaded.

Imagine an executable that uses dlopen/dlclose to load a shared
library.  Given an example of a b/p in the main executable that has a
condition that uses a variable from our shared library, a library
which might be unloaded with dlclose.

My expectation is that, when the library is unloaded, GDB will
automatically mark the breakpoint as disabled_by_cond, however, this
was not happening.

The problem is that in solib_add we only call breakpoint_re_set when
shared libraries are added, not when shared libraries are removed.

The solution I think is to just call breakpoint_re_set in both cases,
now the disabled_by_cond flag is updated as I'd expect.

Unfortunately, making this change causes a regression when running:

  make check-gdb \
     TESTS="gdb.trace/change-loc.exp" \
     RUNTESTFLAGS="--target_board=native-gdbserver"

This test unloads a shared library and expects breakpoints within the
shared library to enter the PENDING state (because the bp_location's
shlib_disabled flag will be set).  However, the new call to
breakpoint_re_set means that this is no longer the case.

The breakpoint_re_set call means that update_breakpoint_locations is
called, which then checks if all locations for a breakpoint are
pending or not.  In this test not all locations are pending, and so
GDB recalculates the locations of each breakpoint, this means that
pending locations are discarded.

There is a but report PR gdb/32404 which mentions the problems with
shlib_disabled pending breakpoints, and how they are prone to being
randomly deleted if the user can cause GDB to trigger a call to
breakpoint_re_set.  This patch just adds another call to
breakpoint_re_set, which triggers this bug in this one test case.

For now I have marked this test as KFAIL.  I do plan to try and
address the pending (shlib_disabled) breakpoint problem in the future,
but I'm not sure when that will be right now.

There are, of course, tests to cover all these cases.

During review I was pointed at bug PR gdb/32079 as something that this
commit might fix, or help in fixing.

And this commit is part of the fix for that bug, but is not the
complete solution.  However, the remaining parts of the fix for that
bug are not really related to the content of this commit.

The problem in PR gdb/32079 is that the inferior maps multiple copies
of libc in different linker namespaces using dlmopen (actually libc is
loaded as a consequence of loading some other library into a different
namespace, but that's just a detail).  The user then uses a 'next'
command to move the inferior forward.

GDB sets up internal breakpoints on the longjmp symbols, of which
there are multiple copies (there is a copy in every loaded libc).

However, the 'next' command is, in the problem case, stepping over a
dlclose call which unloads one of the loaded libc libraries.

In current HEAD GDB in solib_add we fail to call breakpoint_re_set()
when the library is unloaded; breakpoint_re_set() would delete and
then recreate the longjmp breakpoints.  As breakpoint_re_set() is not
called GDB thinks that the the longjmp breakpoint in the now unloaded
libc still exists, and is still inserted.

When the inferior stops after the 'next' GDB tries to delete and
remove the longjmp breakpoint which fails as the libc in which the
breakpoint was inserted is no longer mapped in.

When the user tries to 'next' again GDB tries to re-insert the still
existing longjmp breakpoint which again fails as the memory in which
the b/p should be inserted is no longer part of the inferior memory
space.

This commit helps a little.  Now when the libc library is unmapped GDB
does call breakpoint_re_set().  This deletes the longjmp breakpoints
including the one in the unmapped library, then, when we try to
recreate the longjmp breakpoints (at the end of breakpoint_re_set) we
don't create a b/p in the now unmapped copy of libc.

However GDB does still think that the deleted breakpoint is inserted.
The breakpoint location remains in GDB's data structures until the
next time the inferior stops, at which point GDB tries to remove the
breakpoint .... and fails.

However, as the b/p is now deleted, when the user tries to 'next' GDB
no longer tries to re-insert the b/p, and so one of the problems
reported in PR gdb/32079 is resolved.

I'll fix the remaining issues from PR gdb/32079 in a later commit in
this series.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32079

Tested-By: Hannes Domani <ssbssa@yahoo.de>
Approved-By: Tom Tromey <tom@tromey.com>

0 files changed, 0 insertions, 0 deletions