gdb/python: avoid switching threads for send_packet where possible
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Test passed
|
Commit Message
Bug PR gdb/32120 identifies a case where a user tried to write a
custom frame unwinder that made use of
RemoteTargetConnection.send_packet, however, this was triggering an
assert like this:
value.c:3954: internal-error: fetch_lazy_register: Assertion `next_frame != NULL' failed.
The implementation of RemoteTargetConnection.send_packet can be found
in the function `connpy_send_packet` in python/py-connection.c. This
function includes this code:
scoped_restore_current_thread restore_thread;
switch_to_target_no_thread (conn->target);
The purpose of this is to ensure that the currently selected inferior
has the required remote connection pushed on its target stack. This
is necessary because `send_remote_packet` (see remote.c) still relies
on global state read from the currently selected inferior.
Switching inferior and thread like this, and then switching back
thanks to the scoped restore, will flush the frame cache.
Flushing the frame cache while attempting to build the backtrace is
not something that GDB currently supports, and it's not clear to me if
this can even be supported in the general case.
However, in this specific case there's really no need to flush the
frame cache. The user's example unwinder does this:
class TestCaseUnwinder(Unwinder):
def __init__(self):
super().__init__("TestCaseUnwinder")
def __call__(self, pending_frame):
gdb.selected_inferior().connection.send_packet("qtest.case")
return None
Notice that we're calling send_packet on the connection of the
currently selected inferior. As such, the switch_to_target_no_thread
will (usually) just be re-switching to the current inferior, and
clearing the currently selected thread. The clearing the thread bit
isn't really important, the only important bit is that the current
inferior is set correctly.
I propose that we special case connpy_send_packet so if the correct
inferior is already current then we avoid the switch completely. This
avoids flushing the frame cache, and all the associated problems.
There are two tests. First gdb.python/py-send-packet-unwinder.exp is
a new test the checks the specific case the user was reporting.
Then there is an update to gdb.python/py-send-packet.exp. Now the
test runs first with a single inferior and performs some send_packet
checks, then the test creates a second inferior and makes that the
current inferior, and checks that we can still perform send_packet
calls from a Python variable containing the cached remote connection.
This is checking that the switch_to_target_no_thread call is still
triggering when needed.
While working on gdb.python/py-send-packet.py I spotted a couple of
places where we had:
raise "some string"
which isn't correct, I replaced this with:
raise gdb.GdbError("some string")
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32120
---
gdb/python/py-connection.c | 3 +-
.../gdb.python/py-send-packet-unwinder.c | 47 ++++++
.../gdb.python/py-send-packet-unwinder.exp | 74 +++++++++
.../gdb.python/py-send-packet-unwinder.py | 46 ++++++
gdb/testsuite/gdb.python/py-send-packet.exp | 146 ++++++++++++------
gdb/testsuite/gdb.python/py-send-packet.py | 39 +++--
6 files changed, 285 insertions(+), 70 deletions(-)
create mode 100644 gdb/testsuite/gdb.python/py-send-packet-unwinder.c
create mode 100644 gdb/testsuite/gdb.python/py-send-packet-unwinder.exp
create mode 100644 gdb/testsuite/gdb.python/py-send-packet-unwinder.py
base-commit: adbfdef7131011d1ec85060128a2467c4f18fe0a
Comments
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
Andrew> Bug PR gdb/32120 identifies a case where a user tried to write a
Andrew> custom frame unwinder that made use of
Andrew> RemoteTargetConnection.send_packet
Just for the record this seems like a weird and bad choice.
Andrew> scoped_restore_current_thread restore_thread;
Andrew> switch_to_target_no_thread (conn->target);
Andrew> The purpose of this is to ensure that the currently selected inferior
Andrew> has the required remote connection pushed on its target stack. This
Andrew> is necessary because `send_remote_packet` (see remote.c) still relies
Andrew> on global state read from the currently selected inferior.
Andrew> Switching inferior and thread like this, and then switching back
Andrew> thanks to the scoped restore, will flush the frame cache.
...
Andrew> I propose that we special case connpy_send_packet so if the correct
Andrew> inferior is already current then we avoid the switch completely. This
Andrew> avoids flushing the frame cache, and all the associated problems.
No problem with the patch at all, but I wonder about the logic of
putting this check here and not elsewhere. Like would we want some sort
of "switch to this inferior and we don't care which thread" -- which
would be a no-op if the inferior is already selected?
Tom
Tom Tromey <tom@tromey.com> writes:
>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> Bug PR gdb/32120 identifies a case where a user tried to write a
> Andrew> custom frame unwinder that made use of
> Andrew> RemoteTargetConnection.send_packet
>
> Just for the record this seems like a weird and bad choice.
Probably, I ended up looking at this because I added the send_packet
API, and I was curious why this didn't "just work".
>
> Andrew> scoped_restore_current_thread restore_thread;
> Andrew> switch_to_target_no_thread (conn->target);
>
> Andrew> The purpose of this is to ensure that the currently selected inferior
> Andrew> has the required remote connection pushed on its target stack. This
> Andrew> is necessary because `send_remote_packet` (see remote.c) still relies
> Andrew> on global state read from the currently selected inferior.
>
> Andrew> Switching inferior and thread like this, and then switching back
> Andrew> thanks to the scoped restore, will flush the frame cache.
>
> ...
>
> Andrew> I propose that we special case connpy_send_packet so if the correct
> Andrew> inferior is already current then we avoid the switch completely. This
> Andrew> avoids flushing the frame cache, and all the associated problems.
>
> No problem with the patch at all, but I wonder about the logic of
> putting this check here and not elsewhere. Like would we want some sort
> of "switch to this inferior and we don't care which thread" -- which
> would be a no-op if the inferior is already selected?
I posted a v2 that moves the conditional logic into a new helper
function. It's still a little weird because when we switch we likely
want to use switch_to_target_no_thread which ends with no thread
selected, while in the case where we don't need to switch we leave the
current thread selected. That thread / no-thread difference is strange,
but hopefully with a good comment it shouldn't cause too many problems.
Anyway, feel free to checkout the V2 and see if that looks OK to you.
Thanks,
Andrew
@@ -405,7 +405,8 @@ connpy_send_packet (PyObject *self, PyObject *args, PyObject *kw)
try
{
scoped_restore_current_thread restore_thread;
- switch_to_target_no_thread (conn->target);
+ if (!current_inferior ()->target_is_pushed (conn->target))
+ switch_to_target_no_thread (conn->target);
gdb::array_view<const char> view (packet_str, packet_len);
py_send_packet_callbacks callbacks;
new file mode 100644
@@ -0,0 +1,47 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2026 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+void
+breakpt ()
+{
+ /* Nothing. */
+}
+
+void
+foo ()
+{
+ breakpt ();
+}
+
+void
+bar ()
+{
+ foo ();
+}
+
+void
+baz ()
+{
+ bar ();
+}
+
+int
+main (void)
+{
+ baz ();
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,74 @@
+# Copyright (C) 2026 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Test for a specific use case of RemoteTargetConnection.send_packet
+# that we'd like to support. In general the send_packet method will
+# cause GDB to switch inferior in order to ensure that the remote
+# target connection is on the target stack of the current inferior.
+#
+# The problem is that this causes the currently selected frame to
+# change, which in turn causes the frame cache to be flushed.
+#
+# If a user wants to make send_packet calls from a custom frame
+# unwinder then this is not possible as we end up flushing the frame
+# cache while trying to build the backtrace.
+#
+# The solution is that we have a special case, if when the send_packet
+# call is made, the currently selected inferior already has the
+# required remote target connection on its target stack, then it is
+# not necessary to change the currently selected inferior or thread.
+#
+# By not switching we avoid the frame cache flush, and so calling
+# send_packet from a frame unwinder is supported, so long as we only
+# send the packet to the currently selected inferior's remote target.
+
+load_lib gdb-python.exp
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+require allow_gdbserver_tests allow_python_tests
+
+if { [build_executable "failed to build" ${testfile} ${srcfile}] } {
+ return
+}
+
+set remote_python_file [gdb_remote_download host \
+ ${srcdir}/${subdir}/${testfile}.py]
+
+clean_restart $testfile
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+gdbserver_run ""
+
+gdb_breakpoint "breakpt"
+gdb_continue_to_breakpoint "breakpt"
+
+gdb_test "source $remote_python_file" "Sourcing complete\\." \
+ "source ${testfile}.py script"
+
+gdb_test "bt" \
+ [multi_line \
+ "#0 (?:$hex in )?breakpt \\(\\) at \[^\r\n\]+" \
+ "#1 (?:$hex in )?foo \\(\\) at \[^\r\n\]+" \
+ "#2 (?:$hex in )?bar \\(\\) at \[^\r\n\]+" \
+ "#3 (?:$hex in )?baz \\(\\) at \[^\r\n\]+" \
+ "#4 (?:$hex in )?main \\(\\) at \[^\r\n\]+"]
+
+# Check that the custom unwinder was actually called.
+gdb_test "python print(TestUnwinder.call_count > 0)" "True"
new file mode 100644
@@ -0,0 +1,46 @@
+# Copyright (C) 2026 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+import gdb
+from gdb.unwinder import Unwinder
+
+
+# Count the number of times TestUnwinder.__call__ is called.
+TestUnwinder_Call_Count = 0
+
+
+# A frame unwinder that never claims any frame, but does send a packet
+# when sniffing the frame. As the packet is only sent to the
+# currently selected inferior then we never need to switch thread to
+# send the packet, and so we should never need to flush the frame
+# cache.
+class TestUnwinder(Unwinder):
+ def __init__(self):
+ Unwinder.__init__(self, "send packet unwinder")
+
+ call_count = 0
+
+ def __call__(self, pending_frame):
+ TestUnwinder.call_count += 1
+
+ gdb.selected_inferior().connection.send_packet("vMustReplyEmpty")
+
+ # This unwinder never claims any frames.
+ return None
+
+
+gdb.unwinder.register_unwinder(None, TestUnwinder(), True)
+
+print("Sourcing complete.")
@@ -26,69 +26,117 @@ standard_testfile
require allow_gdbserver_tests allow_python_tests
-if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+if { [build_executable "failed to build" ${testfile} ${srcfile}] } {
return
}
-# Make sure we're disconnected, in case we're testing with an
-# extended-remote board, therefore already connected.
-gdb_test "disconnect" ".*"
-
-gdbserver_run ""
-
-gdb_breakpoint "breakpt"
-gdb_continue_to_breakpoint "breakpt"
-
-# Source the python script.
set remote_python_file [gdb_remote_download host \
${srcdir}/${subdir}/${testfile}.py]
-gdb_test "source $remote_python_file" "Sourcing complete\\." \
- "source ${testfile}.py script"
-# The test is actually written in the Python script. Run it now.
-gdb_test "python run_send_packet_test()" "Send packet test passed"
+# Start GDB debugging the global TESTFILE. Exercise the
+# RemoteTargetConnection.send_packet API to read the auxv data for the
+# inferior, the thread list for the inferior, and to set a global
+# within the inferior.
+#
+# When MULTI_INFERIOR is true a second inferior is started and
+# selected; an inferior with no remote connection. In this case we
+# are checking that calling RemoteTargetConnection.send_packet on a
+# stored RemoteTargetConnection object works as expected.
+proc run_test { multi_inferior } {
+ clean_restart $::testfile
-# Check the string representation of a remote target connection.
-gdb_test "python print(gdb.selected_inferior().connection)" \
- "<gdb.RemoteTargetConnection num=$decimal, what=\".*\">"
+ # Make sure we're disconnected, in case we're testing with an
+ # extended-remote board, therefore already connected.
+ gdb_test "disconnect" ".*"
-# Check to see if there's any auxv data for this target.
-gdb_test_multiple "info auxv" "" {
- -re -wrap "The program has no auxiliary information now\\. " {
- set skip_auxv_test true
- }
- -re -wrap "0\\s+AT_NULL\\s+End of vector\\s+0x0" {
- set skip_auxv_test false
- }
-}
+ gdbserver_run ""
-if { ! $skip_auxv_test } {
- # Use 'maint packet' to fetch the auxv data.
- set reply_data ""
- gdb_test_multiple "maint packet qXfer:auxv:read::0,1000" "" {
- -re "sending: \"qXfer:auxv:read::0,1000\"\r\n" {
- exp_continue
+ gdb_breakpoint "breakpt"
+ gdb_continue_to_breakpoint "breakpt"
+
+ # Source the python script.
+ gdb_test "source $::remote_python_file" "Sourcing complete\\." \
+ "source ${::testfile}.py script"
+
+ # Check to see if there's any auxv data for this target.
+ set skip_auxv_test false
+ gdb_test_multiple "info auxv" "" {
+ -re -wrap "The program has no auxiliary information now\\." {
+ set skip_auxv_test true
}
- -re -wrap "received: \"(.*)\"" {
- set reply_data $expect_out(1,string)
+ -re -wrap "0\\s+AT_NULL\\s+End of vector\\s+0x0" {
+ set skip_auxv_test false
}
}
- # Escape any backslash characters in the output, so we can safely
- # pass a string through to Python.
- set reply_data [string map {\\ \\\\} $reply_data]
- gdb_assert { ![string equal "$reply_data" ""] }
+ set auxv_reply_data ""
+ if { ! $skip_auxv_test } {
+ # Use 'maint packet' to collect the auxv data. This only
+ # works on the current inferior, so needs to be done before we
+ # might add a new inferior.
+ gdb_test_multiple "maint packet qXfer:auxv:read::0,1000" "" {
+ -re "sending: \"qXfer:auxv:read::0,1000\"\r\n" {
+ exp_continue
+ }
+ -re -wrap "received: \"(.*)\"" {
+ set auxv_reply_data $expect_out(1,string)
+ }
+ }
- # Run the test, fetches the auxv data in Python and confirm it
- # matches the expected results.
- gdb_test "python run_auxv_send_packet_test(\"$reply_data\")" \
- "auxv send packet test passed" \
- "call python run_auxv_send_packet_test function"
+ # Escape any backslash characters in the output, so we can safely
+ # pass a string through to Python.
+ set auxv_reply_data [string map {\\ \\\\} $auxv_reply_data]
+ gdb_assert { ![string equal "$auxv_reply_data" ""] } \
+ "have got some auxv data"
+ }
+
+ # If the test requires it, add a second inferior, and make it the
+ # current inferior.
+ if { $multi_inferior } {
+ gdb_test_no_output "python inf=gdb.selected_inferior()" \
+ "capture inferior 1"
+ set inferior "inf"
+
+ gdb_test "add-inferior -no-connection" ".*"
+ gdb_test "inferior 2" ".*"
+ } else {
+ set inferior "gdb.selected_inferior()"
+ }
+
+ # The test is actually written in the Python script. Run it now.
+ gdb_test "python run_send_packet_test($inferior)" "Send packet test passed"
+
+ # Check the string representation of a remote target connection.
+ gdb_test "python print($inferior.connection)" \
+ "<gdb.RemoteTargetConnection num=$::decimal, what=\".*\">"
+
+ if { ! $skip_auxv_test } {
+ # Run the test, fetches the auxv data in Python and confirm it
+ # matches the expected results.
+ gdb_test "python run_auxv_send_packet_test($inferior, \"$auxv_reply_data\")" \
+ "auxv send packet test passed" \
+ "call python run_auxv_send_packet_test function"
+ }
+
+ # The final test relies on gdb.parse_and_eval, so needs the
+ # correct inferior selected.
+ if { $multi_inferior } {
+ gdb_test "inferior 1" ".*" "return to remote inferior"
+ }
+
+ set sizeof_global_var [get_valueof "/d" "sizeof(global_var)" "UNKNOWN"]
+ if { $sizeof_global_var == 4 } {
+ gdb_test_no_output "set debug remote 1"
+ gdb_test "python run_set_global_var_test()" \
+ "set global_var test passed"
+ }
}
-set sizeof_global_var [get_valueof "/d" "sizeof(global_var)" "UNKNOWN"]
-if { $sizeof_global_var == 4 } {
- gdb_test_no_output "set debug remote 1"
- gdb_test "python run_set_global_var_test()" \
- "set global_var test passed"
+set modes { false }
+if {[allow_multi_inferior_tests]} {
+ lappend modes true
+}
+
+foreach_with_prefix multi_inferior $modes {
+ run_test $multi_inferior
}
@@ -19,7 +19,7 @@ import gdb
# Make use of gdb.RemoteTargetConnection.send_packet to fetch the
-# thread list from the remote target.
+# thread list from the remote target for inferior INF.
#
# Sending existing serial protocol packets like this is not a good
# idea, there should be better ways to get this information using an
@@ -28,10 +28,10 @@ import gdb
# Really, the send_packet API would be used to send target
# specific packets to the target, but these are, by definition, target
# specific, so hard to test in a general testsuite.
-def get_thread_list_str():
+def get_thread_list_str(inf):
start_pos = 0
thread_desc = ""
- conn = gdb.selected_inferior().connection
+ conn = inf.connection
if not isinstance(conn, gdb.RemoteTargetConnection):
raise gdb.GdbError("connection is the wrong type")
while True:
@@ -46,19 +46,19 @@ def get_thread_list_str():
# Use gdb.RemoteTargetConnection.send_packet to manually fetch the
-# thread list, then extract the thread list using the gdb.Inferior and
-# gdb.InferiorThread API. Compare the two results to ensure we
-# managed to successfully read the thread list from the remote.
-def run_send_packet_test():
+# thread list for inferior INF. Then extract the thread list using
+# the gdb.Inferior and gdb.InferiorThread API. Compare the two
+# results to ensure we managed to successfully read the thread list
+# from the remote.
+def run_send_packet_test(inf):
# Find the IDs of all current threads.
all_threads = {}
- for inf in gdb.inferiors():
- for thr in inf.threads():
- id = "p%x.%x" % (thr.ptid[0], thr.ptid[1])
- all_threads[id] = False
+ for thr in inf.threads():
+ id = "p%x.%x" % (thr.ptid[0], thr.ptid[1])
+ all_threads[id] = False
# Now fetch the thread list from the remote, and parse the XML.
- str = get_thread_list_str()
+ str = get_thread_list_str(inf)
threads_xml = ET.fromstring(str)
# Look over all threads in the XML list and check we expected to
@@ -66,14 +66,14 @@ def run_send_packet_test():
for thr in threads_xml:
id = thr.get("id")
if id not in all_threads:
- raise "found unexpected thread in remote thread list"
+ raise gdb.GdbError("found unexpected thread in remote thread list")
else:
all_threads[id] = True
# Check that all the threads were found in the XML list.
for id in all_threads:
if not all_threads[id]:
- raise "thread missingt from remote thread list"
+ raise gdb.GdbError("thread missing from remote thread list")
# Test complete.
print("Send packet test passed")
@@ -93,12 +93,11 @@ def bytes_to_string(byte_array):
return res
-# A very simple test for sending the packet that reads the auxv data.
-# We convert the result to a string and expect to find some
-# hex-encoded bytes in the output. This test will only work on
-# targets that actually supply auxv data.
-def run_auxv_send_packet_test(expected_result):
- inf = gdb.selected_inferior()
+# A very simple test for sending the packet that reads the auxv data
+# for inferior INF. We convert the result to a string and expect to
+# find some hex-encoded bytes in the output. This test will only work
+# on targets that actually supply auxv data.
+def run_auxv_send_packet_test(inf, expected_result):
conn = inf.connection
assert isinstance(conn, gdb.RemoteTargetConnection)
res = conn.send_packet("qXfer:auxv:read::0,1000")