It was pointed out on the mailing list that a recently added
test (gdb.python/py-progspace-events.exp) was failing when run with
the native-extended-gdbserver board. This test was added with this
commit:
commit
59912fb2d22f8a4bb0862487f12a5cc65b6a013f
Date: Tue Sep 19 11:45:36 2023 +0100
gdb: add Python events for program space addition and removal
It turns out though that the test is failing due to a existing bug
in GDB, the new test just exposes the problem. Additionally, the
failure really doesn't even rely on the new functionality added in the
above commit. I reduced the test to a simple set of steps that
reproduced the failure and tested against GDB 13, and the test passes;
so the bug was introduced since then. In fact, the bug was introduced
with this commit:
commit
a2827364e2bf19910fa5a54364a594a5ba3033b8
Date: Fri Sep 8 15:48:16 2023 +0100
gdb: remove final user of the executable_changed observer
This commit changed how the per-inferior auxv data cache is managed,
specifically, when the cache is cleared, and it is this that leads to
the failure.
This bug is interesting because it exposes a number of issues with
GDB, I'll explain all of the problems I see, though ultimately, I only
propose fixing one problem in this commit, which is enough to resolve
the crash we are currently seeing.
The crash that we are seeing manifests like this:
...
[Inferior 2 (process
3970384) exited normally]
+inferior 1
[Switching to inferior 1 [process
3970383] (/tmp/build/gdb/testsuite/outputs/gdb.python/py-progspace-events/py-progspace-events)]
[Switching to thread 1.1 (Thread
3970383.
3970383)]
#0 breakpt () at /tmp/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.python/py-progspace-events.c:28
28 { /* Nothing. */ }
(gdb) step
+step
terminate called after throwing an instance of 'gdb_exception_error'
Fatal signal: Aborted
... etc ...
What's happening is that GDB attempts to refill the auxv cache as a
result of the gdbarch_has_shared_address_space call in
program_space::~program_space, the backtrace looks like this:
#0 0x00007fb4f419a9a5 in raise () from /lib64/libpthread.so.0
#1 0x00000000008b635d in handle_fatal_signal (sig=6) at ../../src/gdb/event-top.c:912
#2 <signal handler called>
#3 0x00007fb4f38e3625 in raise () from /lib64/libc.so.6
#4 0x00007fb4f38cc8d9 in abort () from /lib64/libc.so.6
#5 0x00007fb4f3c70756 in __gnu_cxx::__verbose_terminate_handler() [clone .cold] () from /lib64/libstdc++.so.6
#6 0x00007fb4f3c7c6dc in __cxxabiv1::__terminate(void (*)()) () from /lib64/libstdc++.so.6
#7 0x00007fb4f3c7b6e9 in __cxa_call_terminate () from /lib64/libstdc++.so.6
#8 0x00007fb4f3c7c094 in __gxx_personality_v0 () from /lib64/libstdc++.so.6
#9 0x00007fb4f3a80c63 in _Unwind_RaiseException_Phase2 () from /lib64/libgcc_s.so.1
#10 0x00007fb4f3a8154e in _Unwind_Resume () from /lib64/libgcc_s.so.1
#11 0x0000000000e8832d in target_read_alloc_1<unsigned char> (ops=0x408a3a0, object=TARGET_OBJECT_AUXV, annex=0x0) at ../../src/gdb/target.c:2266
#12 0x0000000000e73dea in target_read_alloc (ops=0x408a3a0, object=TARGET_OBJECT_AUXV, annex=0x0) at ../../src/gdb/target.c:2315
#13 0x000000000058248c in target_read_auxv_raw (ops=0x408a3a0) at ../../src/gdb/auxv.c:379
#14 0x000000000058243d in target_read_auxv () at ../../src/gdb/auxv.c:368
#15 0x000000000058255c in target_auxv_search (match=0x0, valp=0x7ffdee17c598) at ../../src/gdb/auxv.c:415
#16 0x0000000000a464bb in linux_is_uclinux () at ../../src/gdb/linux-tdep.c:433
#17 0x0000000000a464f6 in linux_has_shared_address_space (gdbarch=0x409a2d0) at ../../src/gdb/linux-tdep.c:440
#18 0x0000000000510eae in gdbarch_has_shared_address_space (gdbarch=0x409a2d0) at ../../src/gdb/gdbarch.c:4889
#19 0x0000000000bc7558 in program_space::~program_space (this=0x4544aa0, __in_chrg=<optimized out>) at ../../src/gdb/progspace.c:124
#20 0x00000000009b245d in delete_inferior (inf=0x47b3de0) at ../../src/gdb/inferior.c:290
#21 0x00000000009b2c10 in prune_inferiors () at ../../src/gdb/inferior.c:480
#22 0x00000000009c5e3e in fetch_inferior_event () at ../../src/gdb/infrun.c:4558
#23 0x000000000099b4dc in inferior_event_handler (event_type=INF_REG_EVENT) at ../../src/gdb/inf-loop.c:42
#24 0x0000000000cbc64f in remote_async_serial_handler (scb=0x4090a30, context=0x408a6b0) at ../../src/gdb/remote.c:14859
#25 0x0000000000d83d3a in run_async_handler_and_reschedule (scb=0x4090a30) at ../../src/gdb/ser-base.c:138
#26 0x0000000000d83e1f in fd_event (error=0, context=0x4090a30) at ../../src/gdb/ser-base.c:189
So this is problem #1, if we throw an exception while deleting a
program_space then this is not caught, and is going to crash GDB.
Problem #2 becomes evident when we ask why GDB is throwing an error in
this case; the error is thrown because the remote target, operating in
non-async mode, can't read the auxv data while an inferior is running
and GDB is waiting for a stop reply. The problem here then, is why
does GDB get into a position where it tries to interact with the
remote target in this way, at this time? The problem is caused by the
prune_inferiors call which can be seen in the above backtrace.
In prune_inferiors we check if the inferior is deletable, and if it
is, we delete it. The problem is, I think, we should also check if
the target is currently in a state that would allow us to delete the
inferior. We don't currently have such a check available, we'd need
to add one, but for the remote target, this would return false if the
remote is in async mode and the remote is currently waiting for a stop
reply. With this change in place GDB would defer deleting the
inferior until the remote target has stopped, at which point GDB would
be able to refill the auxv cache successfully.
And then, problem #3 becomes evident when we ask why GDB is needing to
refill the auxv cache now when it didn't need to for GDB 13. This is
where the second commit mentioned above (
a2827364e2bf) comes in.
Prior to this commit, the auxv cache was cleared by the
executable_changed observer, while after that commit the auxv cache
was cleared by the new_objfile observer -- but only when the
new_objfile observer is used in the special mode that actually means
that all objfiles have been unloaded (I know, the overloading of the
new_objfile observer is horrible, and unnecessary, but it's not really
important for this bug).
The difference arises because the new_objfile observer is triggered
from clear_symtab_users, which in turn is called from
program_space::~program_space. The new_objfile observer for auxv does
this:
static void
auxv_new_objfile_observer (struct objfile *objfile)
{
if (objfile == nullptr)
invalidate_auxv_cache_inf (current_inferior ());
}
That is, when all the objfiles are unloaded, we clear the auxv cache
for the current inferior.
The problem is, then when we look at the prune_inferiors ->
delete_inferior -> ~program_space path, we see that the current
inferior is not going to be an inferior that exists within the
program_space being deleted; delete_inferior removes the deleted
inferior from the global inferior list, and then only deletes the
program_space if program_space::empty() returns true, which is only
the case if the current inferior isn't within the program_space to
delete, and no other inferior exists within that program_space
either.
What this means is that when the new_objfile observer is called we
can't rely on the current inferior having any relationship with the
program space in which the objfiles were removed. This was an error
in the commit
a2827364e2bf, the only thing we can rely on is the
current program space. As a result of this mistake, after commit
a2827364e2bf, GDB was sometimes clearing the auxv cache for a random
inferior. In the native target case this was harmless as we can
always refill the cache when needed, but in the remote target case, if
we need to refill the cache when the remote target is executing, then
we get the crash we observed.
And additionally, if we think about this a little more, we see that
commit
a2827364e2bf made another mistake. When all the objfiles are
removed, they are removed from a program_space, a program_space might
contain multiple inferiors, so surely, we should clear the auxv cache
for all of the matching inferiors?
Given these two insights, that the current_inferior is not relevant,
only the current_program_space, and that we should be clearing the
cache for all inferiors in the current_program_space, we can update
auxv_new_objfile_observer to:
if (objfile == nullptr)
{
for (inferior *inf : all_inferiors ())
{
if (inf->pspace == current_program_space)
invalidate_auxv_cache_inf (inf);
}
}
With this change we now correctly clear the auxv cache for the correct
inferiors, and GDB no longer needs to refill the cache at an
inconvenient time, this avoids the crash we were seeing.
And finally, we reach problem #4. Inspired by the observation that
using the current_inferior from within the ~program_space function was
not correct, I added some debug to see if current_inferior() was
called anywhere else (below ~program_space), and the answer is yes,
it's called a often. Mostly the culprit is GDB doing:
current_inferior ()->top_target ()-> ....
But I think all of these calls are most likely doing the wrong thing,
and only work because the top target in all these cases is shared
between all inferiors, e.g. it's the native target, or the remote
target for all inferiors. But if we had a truly multi-connection
setup, then we might start to see odd behaviour.
Problem #1 I'm just ignoring for now, I guess at some point we might
run into this again, and then we'd need to solve this. But in this
case I wasn't sure what a "good" solution would look like. We need
the auxv data in order to implement the linux_is_uclinux() function.
If we can't get the auxv data then what should we do, assume yes, or
assume no? The right answer would probably be to propagate the error
back up the stack, but then we reach ~program_space, and throwing
exceptions from a destructor is problematic, so we'd need to catch and
deal at this point. The linux_is_uclinux() call is made from within
gdbarch_has_shared_address_space(), which is used like:
if (!gdbarch_has_shared_address_space (target_gdbarch ()))
delete this->aspace;
So, we would have to choose; delete the address space or not. If we
delete it on error, then we might delete an address space that is
shared within another program space. If we don't delete the address
space, then we might leak it. Neither choice is great.
A better solution might be to have the address spaces be reference
counted, then we could remove the gdbarch_has_shared_address_space
call completely, and just rely on the reference count to auto-delete
the address space when appropriate.
The solution for problem #2 I already hinted at above, we should have
a new target_can_delete_inferiors() call, which should be called from
prune_inferiors, this would prevent GDB from trying to delete
inferiors when a (remote) target is in a state where we know it can't
delete the inferior. Deleting an inferior often (always?) requires
sending packets to the remote, and if the remote is waiting for a stop
reply then this will never work, so the pruning should be deferred in
this case.
The solution for problem #3 is included in this commit.
And, for problem #4, I'm not sure what the right solution is. Maybe
delete_inferior should ensure the inferior to be deleted is in place
when ~program_space is called? But that seems a little weird, as the
current inferior would, in theory, still be using the current
program_space...
Anyway, after this commit, the gdb.python/py-progspace-events.exp test
now passes when run with the native-extended-remote board.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30935
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: I41f0e6e2d7ecc1e5e55ec170f37acd4052f46eaf
projects / binutils-gdb.git / commitdiff