diff mbox series

[gdb/testsuite] Fix race in gdb.threads/detach-step-over.exp

Message ID 20221213140237.32325-1-tdevries@suse.de
State Committed
Headers
Series [gdb/testsuite] Fix race in gdb.threads/detach-step-over.exp |

Commit Message

Tom de Vries Dec. 13, 2022, 2:02 p.m. UTC 
  Once in a while I run into:
...
FAIL: gdb.threads/detach-step-over.exp: \
  breakpoint-condition-evaluation=host: target-non-stop=off: non-stop=off: \
  displaced=off: iter 1: all threads running
...

In can easily reproduce this by doing:
...
     # Wait a bit, to give time for the threads to hit the
     # breakpoint.
-    sleep 1

     return true
...

Fix this by counting the running threads in a loop, effectively allowing 10
seconds (instead of 1) for the threads to start running, but only sleeping if
needed.

Reduces total execution time from 1m27s to 56s.

Tested on x86_64-linux.
---
 .../gdb.threads/detach-step-over.exp          | 34 ++++++++++++++-----
 1 file changed, 26 insertions(+), 8 deletions(-)


base-commit: fa59ab98685e4b5431d2be423f449df5069a454e

Comments

Pedro Alves Dec. 16, 2022, 12:41 p.m. UTC | #1 
On 2022-12-13 2:02 p.m., Tom de Vries via Gdb-patches wrote:
> Once in a while I run into:
> ...
> FAIL: gdb.threads/detach-step-over.exp: \
>   breakpoint-condition-evaluation=host: target-non-stop=off: non-stop=off: \
>   displaced=off: iter 1: all threads running
> ...
> 
> In can easily reproduce this by doing:
> ...
>      # Wait a bit, to give time for the threads to hit the
>      # breakpoint.
> -    sleep 1
> 
>      return true
> ...
> 
> Fix this by counting the running threads in a loop, effectively allowing 10
> seconds (instead of 1) for the threads to start running, but only sleeping if
> needed.
> 
> Reduces total execution time from 1m27s to 56s.

Cool, thanks.  I've run into this race too once in a while, but never managed to find time
to look into it.

LGTM, with just one nit:

> +	    if { $interrupted == 0 } {

Just a bit below we have:

	    if {!$interrupted} {

I'd rather the same form is used in both places, and I think the second form is better.
diff mbox series

Patch

diff --git a/gdb/testsuite/gdb.threads/detach-step-over.exp b/gdb/testsuite/gdb.threads/detach-step-over.exp
index 38d796620cb..225d3ab5313 100644
--- a/gdb/testsuite/gdb.threads/detach-step-over.exp
+++ b/gdb/testsuite/gdb.threads/detach-step-over.exp
@@ -187,10 +187,6 @@  proc prepare_test_iter {testpid non_stop attempt attempts tid_re} {
 	}
     }
 
-    # Wait a bit, to give time for the threads to hit the
-    # breakpoint.
-    sleep 1
-
     return true
 }
 
@@ -233,7 +229,8 @@  proc_with_prefix test_detach_command {condition_eval target_non_stop non_stop di
 
 	    set running_count 0
 	    set interrupted 0
-	    gdb_test_multiple "info threads" "all threads running" {
+	    set running_expected [expr ($::n_threads + 1) * 2]
+	    gdb_test_multiple "info threads" "threads running" {
 		-re "\\(running\\)" {
 		    incr running_count
 		    exp_continue
@@ -254,10 +251,31 @@  proc_with_prefix test_detach_command {condition_eval target_non_stop non_stop di
 			}
 		    }
 		}
-		-re "$::gdb_prompt $" {
-		    gdb_assert {$running_count == ($::n_threads + 1) * 2} \
-			$gdb_test_name
+		-re "$::gdb_prompt " {
+		}
+	    }
+
+	    if { $interrupted == 0 } {
+		set iterations 0
+		set max_iterations 10
+		while { $running_count < $running_expected } {
+		    sleep 1
+		    set running_count 0
+		    gdb_test_multiple "info threads" "threads running" {
+			-re "\\(running\\)" {
+			    incr running_count
+			    exp_continue
+			}
+			-re "$::gdb_prompt " {
+			}
+		    }
+		    incr iterations
+		    if { $iterations == $max_iterations } {
+			break
+		    }
 		}
+		gdb_assert {$running_count == $running_expected} \
+		    "all threads running"
 	    }
 
 	    gdb_test "detach" "Detaching from.*"