diff mbox series

[gdb/testsuite] Fix regexp in gdb.multi/pending-bp-del-inferior.exp

Message ID 20250414145025.32212-1-tdevries@suse.de
State Superseded
Headers
Series [gdb/testsuite] Fix regexp in gdb.multi/pending-bp-del-inferior.exp |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed

Commit Message

Tom de Vries April 14, 2025, 2:50 p.m. UTC 
  With test-case gdb.multi/pending-bp-del-inferior.exp, occasionally I run into:
...
(gdb) info breakpoints^M
Num     Type           Disp Enb Address    What^M
3       dprintf        keep y   <MULTIPLE> ^M
        printf "in foo"^M
3.1                         y   0x004004dc in foo at $c:21 inf 2^M
3.2                         y   0x004004dc in foo at $c:21 inf 1^M
(gdb) FAIL: $exp: bp_pending=false: info breakpoints before inferior removal
...

The FAIL happens because the test-case expects:
- breakpoint location 3.1 to be in inferior 1, and
- breakpoint location 3.2 to be in inferior 2
but it's the other way around.

I managed to reproduce this with a trigger patch in
compare_symbols from gdb/linespec.c:
...
   uia = (uintptr_t) a.symbol->symtab ()->compunit ()->objfile ()->pspace ();
   uib = (uintptr_t) b.symbol->symtab ()->compunit ()->objfile ()->pspace ();

-  if (uia < uib)
-    return true;
   if (uia > uib)
+    return true;
+  if (uia < uib)
     return false;
...

Fix this by allowing the alternative order.

Tested on x86_64-linux.

PR testsuite/32202
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32202
---
 .../gdb.multi/pending-bp-del-inferior.exp        | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)


base-commit: e25c84752c9df2bf3a999b53afb58e5bebaf3b7c

Comments

Tom Tromey April 15, 2025, 9:48 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom>    uia = (uintptr_t) a.symbol->symtab ()->compunit ()->objfile ()->pspace ();
Tom>    uib = (uintptr_t) b.symbol->symtab ()->compunit ()->objfile ()->pspace ();

Tom> -  if (uia < uib)
Tom> -    return true;
Tom>    if (uia > uib)
Tom> +    return true;
Tom> +  if (uia < uib)
Tom>      return false;
Tom> ...

I wonder if the results would be more deterministic if this comparison
used pspace->num rather than the address of the pspace object itself.

This might be a little nicer for users as well, perhaps sorting the
locations by inferior.

I feel like your example change here also shows that the comment before
compare_symbols is incorrect.

I suppose if this is true then compare_msymbols also needs the same fix.

Tom
Tom de Vries April 16, 2025, 2:51 p.m. UTC | #2 
On 4/15/25 23:48, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom>    uia = (uintptr_t) a.symbol->symtab ()->compunit ()->objfile ()->pspace ();
> Tom>    uib = (uintptr_t) b.symbol->symtab ()->compunit ()->objfile ()->pspace ();
> 
> Tom> -  if (uia < uib)
> Tom> -    return true;
> Tom>    if (uia > uib)
> Tom> +    return true;
> Tom> +  if (uia < uib)
> Tom>      return false;
> Tom> ...
> 
> I wonder if the results would be more deterministic if this comparison
> used pspace->num rather than the address of the pspace object itself.
> 
> This might be a little nicer for users as well, perhaps sorting the
> locations by inferior.
> 

Hi Tom,

thanks for the review.

Since you're suggesting to stabilize sorting order, I did a bit of 
further digging, and came up with an alternative solution: there is 
already a sorting function for bp_locations: bp_location_is_less_than, 
but it's only used for the bp_locations vector, not for 
breakpoint::m_locations.

There we use a simple address comparison:
...
       auto ub = std::upper_bound (m_locations.begin (),
                                   m_locations.end (),
                                   loc,
                                   [] (const bp_location &left,
                                       const bp_location &right)
                                     { return left.address
                                              < right.address; });
        m_locations.insert (ub, loc);
...
and consequently the compare_symbol sorting leaks through if the 
addresses are equal.

So, the alternative fix is to reuse bp_location_is_less_than for sorting
breakpoint::m_locations.

I've submitted a patch series here ( 
https://sourceware.org/pipermail/gdb-patches/2025-April/217222.html ).

The series still contains the change you're suggesting, just not as the 
primary fix.

> I feel like your example change here also shows that the comment before
> compare_symbols is incorrect.
> 
> I suppose if this is true then compare_msymbols also needs the same fix.

I've tried to rework the function comment for compare_symbols to make it 
clear what is required and why, rather than stating that "The resulting 
order does not actually matter", as well making it clear where we still 
assume that unstable sorting results do not matter.

Thanks,
- Tom

> 
> Tom
diff mbox series

Patch

diff --git a/gdb/testsuite/gdb.multi/pending-bp-del-inferior.exp b/gdb/testsuite/gdb.multi/pending-bp-del-inferior.exp
index a7055d7ed8c..7a358ca6bd1 100644
--- a/gdb/testsuite/gdb.multi/pending-bp-del-inferior.exp
+++ b/gdb/testsuite/gdb.multi/pending-bp-del-inferior.exp
@@ -183,12 +183,16 @@  proc do_dprintf_test { bp_pending } {
 		 "\\s+printf \"in $bp_func\""]
 	set bp_pattern_after $bp_pattern_before
     } else {
-	set bp_pattern_before \
-	    [multi_line \
-		 "$bp_number\\s+dprintf\\s+keep\\s+y\\s+<MULTIPLE>\\s*" \
-		 "\\s+printf \"in $bp_func\"" \
-		 "$bp_number\\.1\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+ inf 1" \
-		 "$bp_number\\.2\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+ inf 2"]
+	set res {}
+	foreach inf_a { 1 2 } inf_b { 2 1 } {
+	    lappend res \
+		[multi_line \
+		     "$bp_number\\s+dprintf\\s+keep\\s+y\\s+<MULTIPLE>\\s*" \
+		     "\\s+printf \"in $bp_func\"" \
+		     "$bp_number\\.1\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+ inf $inf_a" \
+		     "$bp_number\\.2\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+ inf $inf_b"]
+	}
+	set bp_pattern_before "([join $res "|"])"
 
 	set bp_pattern_after \
 	    [multi_line \