From 16a6f7d2ee33b50f4b6c35f8932379f963bc2beb Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Mon, 23 Sep 2024 15:22:58 +0100 Subject: [PATCH] gdb: avoid breakpoint::clear_locations calls in update_breakpoint_locations The commit: commit 6cce025114ccd0f53cc552fde12b6329596c6c65 Date: Fri Mar 3 19:03:15 2023 +0000 gdb: only insert thread-specific breakpoints in the relevant inferior added a couple of calls to breakpoint::clear_locations() inside update_breakpoint_locations(). The intention of these calls was to avoid leaving redundant locations around when a thread- or inferior-specific breakpoint was switched from one thread or inferior to another. Without the clear_locations() calls the tests gdb.multi/tids.exp and gdb.multi/pending-bp.exp have some failures. A b/p is changed such that the program space it is associated with changes. This triggers a call to breakpoint_re_set_one() but the FILTER_PSPACE argument will be the new program space. As a result GDB correctly calculates the new locations and adds these to the breakpoint, but the old locations, in the old program space, are incorrectly retained. The call to clear_locations() solves this by deleting the old locations. However, while working on another patch I realised that the approach taken here is not correct. The FILTER_PSPACE argument passed to breakpoint_re_set_one() and then on to update_breakpoint_locations() might not be the program space to which the breakpoint is associated. Consider this example: (gdb) file /tmp/hello.x Reading symbols from /tmp/hello.x... (gdb) start Temporary breakpoint 1 at 0x401198: file hello.c, line 18. Starting program: /tmp/hello.x Temporary breakpoint 1, main () at hello.c:18 18 printf ("Hello World\n"); (gdb) break main thread 1 Breakpoint 2 at 0x401198: file hello.c, line 18. (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y 0x0000000000401198 in main at hello.c:18 stop only in thread 1 (gdb) add-inferior -exec /tmp/hello.x [New inferior 2] Added inferior 2 on connection 1 (native) Reading symbols from /tmp/hello.x... (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y main stop only in thread 1.1 Notice that after creating the second inferior and loading a file the thread-specific breakpoint was incorrectly made pending. Loading the exec file in the second inferior triggered a call to breakpoint_re_set() with the new, second, program space as the current_program_space. This program space ends up being passed to update_breakpoint_locations(). In update_breakpoint_locations this condition is true: if (all_locations_are_pending (b, filter_pspace) && sals.empty ()) and so we end up discarding all of the locations for this breakpoint, making the breakpoint pending. What we really want to do in update_breakpoint_locations() is, for thread- or inferior- specific breakpoints, delete any locations which are associated with a program space that this breakpoint is NOT associated with. But then I realised the answer was easier than that. The ONLY time that a b/p can have locations associated with the "wrong" program space like this is at the moment we change the thread or inferior the b/p is associated with by calling breakpoint_set_thread() or breakpoint_set_inferior(). And so, I think the correct solution is to hoist the call to clear_locations() out of update_breakpoint_locations() and place a call in each of the breakpoint_set_{thread,inferior} functions. I've done this, and added a couple of new tests. All of which are now passing. Approved-By: Tom Tromey --- gdb/breakpoint.c | 55 +++++++-------- .../gdb.multi/bp-thread-specific.exp | 24 +++++++ .../gdb.multi/inferior-specific-bp-1.c | 2 +- .../gdb.multi/inferior-specific-bp-2.c | 2 +- .../gdb.multi/inferior-specific-bp.exp | 67 +++++++++++++++++++ 5 files changed, 121 insertions(+), 29 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 3abb596402c..10e2ef38523 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1578,10 +1578,20 @@ breakpoint_set_thread (struct breakpoint *b, int thread) /* If the program space has changed for this breakpoint, then re-evaluate it's locations. */ if (old_pspace != new_pspace) - breakpoint_re_set_one (b, new_pspace); + { + /* The breakpoint is now associated with a completely different + program space. Discard all of the current locations and then + re-set the breakpoint in the new program space, this will + create the new locations. */ + b->clear_locations (); + breakpoint_re_set_one (b, new_pspace); + } - /* Let others know the breakpoint has changed. */ - notify_breakpoint_modified (b); + /* If the program space didn't change, or the breakpoint didn't + acquire any new locations after the clear_locations call, then we + need to notify of the breakpoint modification now. */ + if (old_pspace == new_pspace || !b->has_locations ()) + notify_breakpoint_modified (b); } } @@ -1625,9 +1635,20 @@ breakpoint_set_inferior (struct breakpoint *b, int inferior) } if (old_pspace != new_pspace) - breakpoint_re_set_one (b, new_pspace); + { + /* The breakpoint is now associated with a completely different + program space. Discard all of the current locations and then + re-set the breakpoint in the new program space, this will + create the new locations. */ + b->clear_locations (); + breakpoint_re_set_one (b, new_pspace); + } - notify_breakpoint_modified (b); + /* If the program space didn't change, or the breakpoint didn't + acquire any new locations after the clear_locations call, then we + need to notify of the breakpoint modification now. */ + if (old_pspace == new_pspace || !b->has_locations ()) + notify_breakpoint_modified (b); } } @@ -12943,32 +12964,12 @@ update_breakpoint_locations (code_breakpoint *b, all locations are in the same shared library, that was unloaded. We'd like to retain the location, so that when the library is loaded again, we don't loose the enabled/disabled status of the - individual locations. - - Thread specific breakpoints will also trigger this case if the thread - is changed to a different program space, and all of the old locations - go out of scope. In this case we do (currently) discard the old - locations -- we assume the change in thread is permanent and the old - locations will never come back into scope. */ + individual locations. */ if (all_locations_are_pending (b, filter_pspace) && sals.empty ()) - { - if (b->thread != -1) - b->clear_locations (); - return; - } + return; bp_location_list existing_locations = b->steal_locations (filter_pspace); - /* If this is a thread-specific breakpoint then any locations left on the - breakpoint are for a program space in which the thread of interest - does not operate. This can happen when the user changes the thread of - a thread-specific breakpoint. - - We assume that the change in thread is permanent, and that the old - locations will never be used again, so discard them now. */ - if (b->thread != -1) - b->clear_locations (); - for (const auto &sal : sals) { struct bp_location *new_loc; diff --git a/gdb/testsuite/gdb.multi/bp-thread-specific.exp b/gdb/testsuite/gdb.multi/bp-thread-specific.exp index c1d87521ee9..11dc2483624 100644 --- a/gdb/testsuite/gdb.multi/bp-thread-specific.exp +++ b/gdb/testsuite/gdb.multi/bp-thread-specific.exp @@ -32,9 +32,33 @@ if {![runto_main]} { return -1 } +delete_breakpoints + +# Create a thread-specific b/p on main. +gdb_breakpoint "main thread 1" +set bpnum [get_integer_valueof "\$bpnum" "INVALID" \ + "get number for thread specific b/p on main"] + +# Check the b/p has a location and is displayed correctly. +gdb_test "info breakpoints" \ + [multi_line \ + "" \ + "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in main at \[^\r\n\]+/$srcfile:$decimal"\ + "\\s+stop only in thread 1"] \ + "check thread b/p on main has a location" + gdb_test "add-inferior -exec ${binfile}" "Added inferior 2.*" "add inferior 2" gdb_test "inferior 2" +# The breakpoint should still have a location, but should now display +# information indicating this breakpoint is only in inferior 1. +gdb_test "info breakpoints" \ + [multi_line \ + "" \ + "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in main at \[^\r\n\]+/$srcfile:$decimal inf 1"\ + "\\s+stop only in thread 1\\.1"] \ + "check thread b/p on main still has updated correctly" + if {![runto_main]} { return -1 } diff --git a/gdb/testsuite/gdb.multi/inferior-specific-bp-1.c b/gdb/testsuite/gdb.multi/inferior-specific-bp-1.c index 59a6e32c3c5..16db0629e92 100644 --- a/gdb/testsuite/gdb.multi/inferior-specific-bp-1.c +++ b/gdb/testsuite/gdb.multi/inferior-specific-bp-1.c @@ -35,7 +35,7 @@ foo (void) static void bar (void) { - global_var = 0; + global_var = 0; /* First location of bar. */ foo (); } diff --git a/gdb/testsuite/gdb.multi/inferior-specific-bp-2.c b/gdb/testsuite/gdb.multi/inferior-specific-bp-2.c index cbae7450117..bde6fbf8224 100644 --- a/gdb/testsuite/gdb.multi/inferior-specific-bp-2.c +++ b/gdb/testsuite/gdb.multi/inferior-specific-bp-2.c @@ -36,7 +36,7 @@ main (void) static int bar (void) { - return baz (); + return baz (); /* Second location of bar. */ } static int diff --git a/gdb/testsuite/gdb.multi/inferior-specific-bp.exp b/gdb/testsuite/gdb.multi/inferior-specific-bp.exp index 52f84183589..82cc9240e50 100644 --- a/gdb/testsuite/gdb.multi/inferior-specific-bp.exp +++ b/gdb/testsuite/gdb.multi/inferior-specific-bp.exp @@ -62,6 +62,73 @@ gdb_test "break foo inferior 1 inferior 2" \ # Clear out any other breakpoints. delete_breakpoints +# Create an inferior specific breakpoint and then change the inferior +# using the Python API. Use 'info breakpoint' to check that the +# breakpoint was updated as we expect. +if { [allow_python_tests] } { + with_test_prefix "update breakpoint inferior" { + # Create the b/p and grab its number. + gdb_breakpoint "bar inferior 1" + set bpnum [get_integer_valueof "\$bpnum" "INVALID" \ + "get b/p number for breakpoint on bar"] + + # Get the line number for the two locations, the first in + # inferior 1, the second in inferior 2. + set bar_lineno_1 \ + [gdb_get_line_number "First location of bar" $srcfile] + set bar_lineno_2 \ + [gdb_get_line_number "Second location of bar" $srcfile2] + + # Check the b/p was created with a single location where we + # expect it. + gdb_test "info breakpoint $bpnum" \ + [multi_line \ + "" \ + "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in bar at \[^\r\n\]+/$srcfile:$bar_lineno_1 inf 1" \ + "\\s+stop only in inferior 1"] \ + "check original details for breakpoint on bar" + + # Use the Python API to update the b/p's inferior. + gdb_test_no_output "python bp = gdb.breakpoints()\[0\]" + gdb_test_no_output "python bp.inferior = 2" + + # We should still only have a single location, but now in + # inferior 2. + gdb_test "info breakpoint $bpnum" \ + [multi_line \ + "" \ + "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in bar at \[^\r\n\]+/$srcfile2:$bar_lineno_2 inf 2" \ + "\\s+stop only in inferior 2"] \ + "check updated details for breakpoint on bar" + + # Use the Python API to remove the inferior restriction on the + # breakpoint. + gdb_test_no_output "python bp.inferior = None" + + # The breakpoint should now have multiple locations. + gdb_test "info breakpoint $bpnum" \ + [multi_line \ + "" \ + "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+\\s*" \ + "$bpnum.1\\s+y\\s+$hex\\s+in bar at\[^\r\n\]+$srcfile:$bar_lineno_1 inf 1" \ + "$bpnum.2\\s+y\\s+$hex\\s+in bar at\[^\r\n\]+$srcfile2:$bar_lineno_2 inf 2"] \ + "check breakpoint bar now inferior requirement is gone" + + # Finally, add the inferior requirement back. + gdb_test_no_output "python bp.inferior = 1" + + # Check the original location and restriction is restored. + gdb_test "info breakpoint $bpnum" \ + [multi_line \ + "" \ + "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in bar at \[^\r\n\]+/$srcfile:$bar_lineno_1 inf 1" \ + "\\s+stop only in inferior 1"] \ + "check original details for breakpoint on bar are back" + + delete_breakpoints + } +} + # Use 'info breakpoint' to check that the inferior specific breakpoint is # present in the breakpoint list. TESTNAME is the name used for this test, # BP_NUMBER is the number for the breakpoint, and EXPECTED_LOC_COUNT is the -- 2.39.5