[PATCH v4 0/6] Fixes to gdb.threads/threadcrash.exp

[PATCH v4 0/6] Fixes to gdb.threads/threadcrash.exp

[Tom de Vries]

The test was recently pushed to master but it had a couple of small issues.
This is a quick series to fix the major issues.

Tested on arm-linux and x86_64-linux.

Changes for v4:
* added patch to remove unnecessary indentation
* added patch to simplify test_list usage
* added patch to fix check-readmore issues
* split off patch with _inferior_thread_count fix
* updated patch "Reduce gdb.threads/threadcrash.exp reliance on libc symbols"
  to allow matching of libc symbols, if they're available.
* updated kfail patch to be both more specific, and only emit the kfail it it
  actually triggers.

Changes for v3:
* use get_valueof to get thread count, to stop a failure on Linaro's CI.

Changes for v2:
* changed xfail from patch 1 to a kfail

Guinevere Larsen (2):
  gdb/testsuite: Use _inferior_thread_count in
    gdb.threads/threadcrash.exp
  gdb/testsuite: Reduce gdb.threads/threadcrash.exp reliance on libc
    symbols

Tom de Vries (4):
  gdb/testsuite: Reduce indentation in gdb.threads/threadcrash.exp
  gdb/testsuite: Fix gdb.threads/threadcrash.exp with check-readmore
  gdb/testsuite: Simplify gdb.threads/threadcrash.exp
  gdb/testsuite: Add kfail in gdb.threads/threadcrash.exp on 32-bit arm
    targets

 gdb/testsuite/gdb.threads/threadcrash.exp | 236 ++++++++++++++--------
 1 file changed, 150 insertions(+), 86 deletions(-)


base-commit: 5b95198e2e40b0301d37d989edc344a334c26b12
-- 
2.35.3

[PATCH v4 1/6] gdb/testsuite: Reduce indentation in gdb.threads/threadcrash.exp

[Tom de Vries]

In test-case gdb.threads/threadcrash.exp we have an unnecessarily indented
gdb_test_multiple:
...
    gdb_test_multiple "thread apply all backtrace" \
	"Get thread information" -lbl {
	    -re "#\[0-9\]+\\\?\\\?\[^\n\]*" {
...

Fix this by moving the command into a variable, allowing the
"gdb_test_multiple ... {" to fit on a single 80 chars line.

Tested on arm-linux and x86_64-linux.
---
 gdb/testsuite/gdb.threads/threadcrash.exp | 116 +++++++++++-----------
 1 file changed, 58 insertions(+), 58 deletions(-)

diff --git a/gdb/testsuite/gdb.threads/threadcrash.exp b/gdb/testsuite/gdb.threads/threadcrash.exp
index 996e020d1e8..3d269ec88c5 100644
--- a/gdb/testsuite/gdb.threads/threadcrash.exp
+++ b/gdb/testsuite/gdb.threads/threadcrash.exp
@@ -53,64 +53,64 @@ proc thread_apply_all {} {
 
     set unwind_fail false
 
-    gdb_test_multiple "thread apply all backtrace" \
-	"Get thread information" -lbl {
-	    -re "#\[0-9\]+\\\?\\\?\[^\n\]*" {
-		set unwind_fail true
-		exp_continue
-	    }
-	    -re "\[^\n\]*syscall_task .location=SIGNAL_ALT_STACK\[^\n\]*" {
-		lappend test_list [multi_line ".*sleep.*" \
-					      ".*do_syscall_task .location=SIGNAL_ALT_STACK.*" \
-					      ".*signal_handler.*" \
-					      ".*signal handler called.*" \
-					      ".*pthread_kill.*" \
-					      ".*thread_function.*"]
-		exp_continue
-	    }
-	    -re "\[^\n\]*syscall_task .location=SIGNAL_HANDLER\[^\n\]*" {
-		lappend test_list [multi_line ".*sleep.*" \
-					      ".*do_syscall_task .location=SIGNAL_HANDLER.*" \
-					      ".*signal_handler.*" \
-					      ".*signal handler called.*" \
-					      ".*pthread_kill.*" \
-					      ".*thread_function.*"]
-		exp_continue
-	    }
-	    -re "\[^\n\]*syscall_task .location=NORMAL\[^\n\]*" {
-		lappend test_list [multi_line ".*sleep.*" \
-					      ".*do_syscall_task .location=NORMAL.*" \
-					      ".*thread_function.*"]
-		exp_continue
-	    }
-	    -re "\[^\n\]*spin_task .location=SIGNAL_ALT_STACK\[^\n\]*" {
-		lappend test_list [multi_line ".*do_spin_task .location=SIGNAL_ALT_STACK.*" \
-					      ".*signal_handler.*" \
-					      ".*signal handler called.*" \
-					      ".*pthread_kill.*" \
-					      ".*thread_function.*"]
-		exp_continue
-	    }
-	    -re "\[^\n\]*spin_task .location=SIGNAL_HANDLER\[^\n\]*" {
-		lappend test_list [multi_line ".*do_spin_task .location=SIGNAL_HANDLER.*" \
-					      ".*signal_handler.*" \
-					      ".*signal handler called.*" \
-					      ".*pthread_kill.*" \
-					      ".*thread_function.*"]
-		exp_continue
-	    }
-	    -re "\[^\n\]*spin_task .location=NORMAL\[^\n\]*" {
-		lappend test_list [multi_line ".*do_spin_task .location=NORMAL..*" \
-					      ".*thread_function.*"]
-		exp_continue
-	    }
-	    -re "\[^\n\]*main\[^\n\]*" {
-		lappend test_list ".*main.*"
-		exp_continue
-	    }
-	    -re "$::gdb_prompt " {
-		pass $gdb_test_name
-	    }
+    set cmd "thread apply all backtrace"
+    gdb_test_multiple $cmd "Get thread information" -lbl {
+	-re "#\[0-9\]+\\\?\\\?\[^\n\]*" {
+	    set unwind_fail true
+	    exp_continue
+	}
+	-re "\[^\n\]*syscall_task .location=SIGNAL_ALT_STACK\[^\n\]*" {
+	    lappend test_list [multi_line ".*sleep.*" \
+				   ".*do_syscall_task .location=SIGNAL_ALT_STACK.*" \
+				   ".*signal_handler.*" \
+				   ".*signal handler called.*" \
+				   ".*pthread_kill.*" \
+				   ".*thread_function.*"]
+	    exp_continue
+	}
+	-re "\[^\n\]*syscall_task .location=SIGNAL_HANDLER\[^\n\]*" {
+	    lappend test_list [multi_line ".*sleep.*" \
+				   ".*do_syscall_task .location=SIGNAL_HANDLER.*" \
+				   ".*signal_handler.*" \
+				   ".*signal handler called.*" \
+				   ".*pthread_kill.*" \
+				   ".*thread_function.*"]
+	    exp_continue
+	}
+	-re "\[^\n\]*syscall_task .location=NORMAL\[^\n\]*" {
+	    lappend test_list [multi_line ".*sleep.*" \
+				   ".*do_syscall_task .location=NORMAL.*" \
+				   ".*thread_function.*"]
+	    exp_continue
+	}
+	-re "\[^\n\]*spin_task .location=SIGNAL_ALT_STACK\[^\n\]*" {
+	    lappend test_list [multi_line ".*do_spin_task .location=SIGNAL_ALT_STACK.*" \
+				   ".*signal_handler.*" \
+				   ".*signal handler called.*" \
+				   ".*pthread_kill.*" \
+				   ".*thread_function.*"]
+	    exp_continue
+	}
+	-re "\[^\n\]*spin_task .location=SIGNAL_HANDLER\[^\n\]*" {
+	    lappend test_list [multi_line ".*do_spin_task .location=SIGNAL_HANDLER.*" \
+				   ".*signal_handler.*" \
+				   ".*signal handler called.*" \
+				   ".*pthread_kill.*" \
+				   ".*thread_function.*"]
+	    exp_continue
+	}
+	-re "\[^\n\]*spin_task .location=NORMAL\[^\n\]*" {
+	    lappend test_list [multi_line ".*do_spin_task .location=NORMAL..*" \
+				   ".*thread_function.*"]
+	    exp_continue
+	}
+	-re "\[^\n\]*main\[^\n\]*" {
+	    lappend test_list ".*main.*"
+	    exp_continue
+	}
+	-re "$::gdb_prompt " {
+	    pass $gdb_test_name
+	}
     }
 
     gdb_assert {$unwind_fail == false}
-- 
2.35.3

Re: [PATCH v4 1/6] gdb/testsuite: Reduce indentation in gdb.threads/threadcrash.exp

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> In test-case gdb.threads/threadcrash.exp we have an unnecessarily indented
Tom> gdb_test_multiple:
Tom> ...
Tom>     gdb_test_multiple "thread apply all backtrace" \
Tom> 	"Get thread information" -lbl {
Tom> 	    -re "#\[0-9\]+\\\?\\\?\[^\n\]*" {
Tom> ...

Tom> Fix this by moving the command into a variable, allowing the
Tom> "gdb_test_multiple ... {" to fit on a single 80 chars line.

I like it.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

[PATCH v4 2/6] gdb/testsuite: Fix gdb.threads/threadcrash.exp with check-readmore

[Tom de Vries]

With check-readmore, I run into:
...
FAIL: gdb.threads/threadcrash.exp: test_corefile: \
  $thread_count == [llength $test_list]
...

The problem is that the clauses in the gdb_test_multiple for
"thread apply all backtrace" intent to match one line, but actually can
match more than one line, and consequently a match for one type of thread can
consume a line that was supposed to match another thread.

For instance, there's this regexp:
...
	    -re "\[^\n\]*syscall_task .location=SIGNAL_ALT_STACK\[^\n\]*" {
...

It's limited at the end by \[^\n\]*, meaning the match stops at the end of the
line.

But it doesn't start with a ^, and consequently can match more than one line.
The "\[^\n\]*" at the start doesn't prevent this, there's an implicit .* at
the start of each pattern, unless it's anchored using a ^.

Fix this by rewriting the regexps in a "^\r\n$hs$regexp$hs$eol" style, where:
- hs is: \[^\n\]* (horizontal space), and
- eol is (?=\r\n) (look-ahead end-of-line).

It also turned out to be necessary to drop the -lbl switch, and introduce a
corresponding explicit clause.  The -lbl clause is placed ALAP, and
consequently allowed the default fail clause to trigger.

Tested on arm-linux and x86_64-linux.
---
 gdb/testsuite/gdb.threads/threadcrash.exp | 29 +++++++++++++++--------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/gdb.threads/threadcrash.exp b/gdb/testsuite/gdb.threads/threadcrash.exp
index 3d269ec88c5..592098b9c38 100644
--- a/gdb/testsuite/gdb.threads/threadcrash.exp
+++ b/gdb/testsuite/gdb.threads/threadcrash.exp
@@ -53,13 +53,19 @@ proc thread_apply_all {} {
 
     set unwind_fail false
 
+    set eol "(?=\r\n)"
+    set hs "\[^\r\n\]*"
+
     set cmd "thread apply all backtrace"
-    gdb_test_multiple $cmd "Get thread information" -lbl {
-	-re "#\[0-9\]+\\\?\\\?\[^\n\]*" {
+    gdb_test_multiple $cmd "Get thread information" {
+	-re "^$cmd$eol" {
+	    exp_continue
+	}
+	-re "^\r\n#$::decimal\\\?\\\?$hs$eol" {
 	    set unwind_fail true
 	    exp_continue
 	}
-	-re "\[^\n\]*syscall_task .location=SIGNAL_ALT_STACK\[^\n\]*" {
+	-re "^\r\n${hs}syscall_task .location=SIGNAL_ALT_STACK$hs$eol" {
 	    lappend test_list [multi_line ".*sleep.*" \
 				   ".*do_syscall_task .location=SIGNAL_ALT_STACK.*" \
 				   ".*signal_handler.*" \
@@ -68,7 +74,7 @@ proc thread_apply_all {} {
 				   ".*thread_function.*"]
 	    exp_continue
 	}
-	-re "\[^\n\]*syscall_task .location=SIGNAL_HANDLER\[^\n\]*" {
+	-re "^\r\n${hs}syscall_task .location=SIGNAL_HANDLER$hs$eol" {
 	    lappend test_list [multi_line ".*sleep.*" \
 				   ".*do_syscall_task .location=SIGNAL_HANDLER.*" \
 				   ".*signal_handler.*" \
@@ -77,13 +83,13 @@ proc thread_apply_all {} {
 				   ".*thread_function.*"]
 	    exp_continue
 	}
-	-re "\[^\n\]*syscall_task .location=NORMAL\[^\n\]*" {
+	-re "^\r\n${hs}syscall_task .location=NORMAL$hs$eol" {
 	    lappend test_list [multi_line ".*sleep.*" \
 				   ".*do_syscall_task .location=NORMAL.*" \
 				   ".*thread_function.*"]
 	    exp_continue
 	}
-	-re "\[^\n\]*spin_task .location=SIGNAL_ALT_STACK\[^\n\]*" {
+	-re "^\r\n${hs}spin_task .location=SIGNAL_ALT_STACK$hs$eol" {
 	    lappend test_list [multi_line ".*do_spin_task .location=SIGNAL_ALT_STACK.*" \
 				   ".*signal_handler.*" \
 				   ".*signal handler called.*" \
@@ -91,7 +97,7 @@ proc thread_apply_all {} {
 				   ".*thread_function.*"]
 	    exp_continue
 	}
-	-re "\[^\n\]*spin_task .location=SIGNAL_HANDLER\[^\n\]*" {
+	-re "^\r\n${hs}spin_task .location=SIGNAL_HANDLER$hs$eol" {
 	    lappend test_list [multi_line ".*do_spin_task .location=SIGNAL_HANDLER.*" \
 				   ".*signal_handler.*" \
 				   ".*signal handler called.*" \
@@ -99,16 +105,19 @@ proc thread_apply_all {} {
 				   ".*thread_function.*"]
 	    exp_continue
 	}
-	-re "\[^\n\]*spin_task .location=NORMAL\[^\n\]*" {
+	-re "^\r\n${hs}spin_task .location=NORMAL$hs$eol" {
 	    lappend test_list [multi_line ".*do_spin_task .location=NORMAL..*" \
 				   ".*thread_function.*"]
 	    exp_continue
 	}
-	-re "\[^\n\]*main\[^\n\]*" {
+	-re "^\r\n${hs}main$hs$eol" {
 	    lappend test_list ".*main.*"
 	    exp_continue
 	}
-	-re "$::gdb_prompt " {
+	-re "^\r\n$hs$eol" {
+	    exp_continue
+	}
+	-re "^\r\n$::gdb_prompt $" {
 	    pass $gdb_test_name
 	}
     }
-- 
2.35.3

Re: [PATCH v4 2/6] gdb/testsuite: Fix gdb.threads/threadcrash.exp with check-readmore

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Fix this by rewriting the regexps in a "^\r\n$hs$regexp$hs$eol" style, where:
Tom> - hs is: \[^\n\]* (horizontal space), and
Tom> - eol is (?=\r\n) (look-ahead end-of-line).

Tom> It also turned out to be necessary to drop the -lbl switch, and introduce a
Tom> corresponding explicit clause.  The -lbl clause is placed ALAP, and
Tom> consequently allowed the default fail clause to trigger.

I feel like often we'd like tests to work "line by line" in the sense
that each pattern is matched against each line in isolation.  Maybe
someday we should add a new proc that works this way.

Tom

[PATCH v4 3/6] gdb/testsuite: Use _inferior_thread_count in gdb.threads/threadcrash.exp

[Tom de Vries]

From: Guinevere Larsen <blarsen@redhat.com>

A linaro PR [1] reports that the gdb.threads/threadcrash.exp test-case fails
to cout the number of threads in the inferior:
...
FAIL: gdb.threads/threadcrash.exp: test_gcore: $thread_count == 7
FAIL: gdb.threads/threadcrash.exp: test_gcore: $thread_count == [llength $test_list]
...

Fix this by getting the convenience variable _inferior_thread_count as opposed
to calculating it based on the output of "info threads".

Tested on arm-linux and x86_64-linux.

Reviewed-By: Lancelot Six <lancelot.six@amd.com>

[1] https://linaro.atlassian.net/browse/GNU-1120
---
 gdb/testsuite/gdb.threads/threadcrash.exp | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/gdb/testsuite/gdb.threads/threadcrash.exp b/gdb/testsuite/gdb.threads/threadcrash.exp
index 592098b9c38..966708acd96 100644
--- a/gdb/testsuite/gdb.threads/threadcrash.exp
+++ b/gdb/testsuite/gdb.threads/threadcrash.exp
@@ -20,26 +20,6 @@
 # a gcore.
 
 
-# Check that the inferior has 7 threads, and return the number of threads (7).
-# We return the thread count so that, even if there is some error in the test,
-# the final log doesn't get flooded with failures.
-
-proc test_thread_count {} {
-    set thread_count 0
-
-    gdb_test_multiple "info threads" "getting thread count" -lbl {
-	-re "Thread" {
-	    incr thread_count
-	    exp_continue
-	}
-	-re "$::gdb_prompt " {
-	    gdb_assert {$thread_count == 7}
-	}
-    }
-
-    return $thread_count
-}
-
 # Use 'thread apply all backtrace' to check if all expected threads
 # are present, and stopped in the expected locations.  Set the global
 # TEST_LIST to be the a list of regexps expected to match all the
@@ -132,7 +112,8 @@ proc thread_apply_all {} {
 
 proc do_full_test {} {
     global test_list
-    set thread_count [test_thread_count]
+    set thread_count [get_valueof "" "\$_inferior_thread_count" 0]
+    gdb_assert {$thread_count == 7}
 
     thread_apply_all
 
-- 
2.35.3

[PATCH v4 4/6] gdb/testsuite: Simplify gdb.threads/threadcrash.exp

[Tom de Vries]

I noticed in gdb.threads/threadcrash.exp that the usage of test_list is
somewhat convoluted.

Simplify the test-case by storing a classification instead of a pattern in
test_list.

Tested on arm-linux and x86_64-linux.
---
 gdb/testsuite/gdb.threads/threadcrash.exp | 89 +++++++++++++++--------
 1 file changed, 60 insertions(+), 29 deletions(-)

diff --git a/gdb/testsuite/gdb.threads/threadcrash.exp b/gdb/testsuite/gdb.threads/threadcrash.exp
index 966708acd96..16fd7eb4e23 100644
--- a/gdb/testsuite/gdb.threads/threadcrash.exp
+++ b/gdb/testsuite/gdb.threads/threadcrash.exp
@@ -46,52 +46,31 @@ proc thread_apply_all {} {
 	    exp_continue
 	}
 	-re "^\r\n${hs}syscall_task .location=SIGNAL_ALT_STACK$hs$eol" {
-	    lappend test_list [multi_line ".*sleep.*" \
-				   ".*do_syscall_task .location=SIGNAL_ALT_STACK.*" \
-				   ".*signal_handler.*" \
-				   ".*signal handler called.*" \
-				   ".*pthread_kill.*" \
-				   ".*thread_function.*"]
+	    lappend test_list 1
 	    exp_continue
 	}
 	-re "^\r\n${hs}syscall_task .location=SIGNAL_HANDLER$hs$eol" {
-	    lappend test_list [multi_line ".*sleep.*" \
-				   ".*do_syscall_task .location=SIGNAL_HANDLER.*" \
-				   ".*signal_handler.*" \
-				   ".*signal handler called.*" \
-				   ".*pthread_kill.*" \
-				   ".*thread_function.*"]
+	    lappend test_list 2
 	    exp_continue
 	}
 	-re "^\r\n${hs}syscall_task .location=NORMAL$hs$eol" {
-	    lappend test_list [multi_line ".*sleep.*" \
-				   ".*do_syscall_task .location=NORMAL.*" \
-				   ".*thread_function.*"]
+	    lappend test_list 3
 	    exp_continue
 	}
 	-re "^\r\n${hs}spin_task .location=SIGNAL_ALT_STACK$hs$eol" {
-	    lappend test_list [multi_line ".*do_spin_task .location=SIGNAL_ALT_STACK.*" \
-				   ".*signal_handler.*" \
-				   ".*signal handler called.*" \
-				   ".*pthread_kill.*" \
-				   ".*thread_function.*"]
+	    lappend test_list 4
 	    exp_continue
 	}
 	-re "^\r\n${hs}spin_task .location=SIGNAL_HANDLER$hs$eol" {
-	    lappend test_list [multi_line ".*do_spin_task .location=SIGNAL_HANDLER.*" \
-				   ".*signal_handler.*" \
-				   ".*signal handler called.*" \
-				   ".*pthread_kill.*" \
-				   ".*thread_function.*"]
+	    lappend test_list 5
 	    exp_continue
 	}
 	-re "^\r\n${hs}spin_task .location=NORMAL$hs$eol" {
-	    lappend test_list [multi_line ".*do_spin_task .location=NORMAL..*" \
-				   ".*thread_function.*"]
+	    lappend test_list 6
 	    exp_continue
 	}
 	-re "^\r\n${hs}main$hs$eol" {
-	    lappend test_list ".*main.*"
+	    lappend test_list 7
 	    exp_continue
 	}
 	-re "^\r\n$hs$eol" {
@@ -122,7 +101,59 @@ proc do_full_test {} {
     for {set i 0} {$i < $thread_count } {incr i} {
 	set thread_num [expr [llength $test_list] - $i]
 
-	gdb_test "thread apply $thread_num backtrace" [lindex $test_list $i]
+	set type [lindex $test_list $i]
+	if { $type == 1 } {
+	    set re \
+		[multi_line \
+		     ".sleep.*" \
+		     ".*do_syscall_task .location=SIGNAL_ALT_STACK.*" \
+		     ".*signal_handler.*" \
+		     ".*signal handler called.*" \
+		     ".*pthread_kill.*" \
+		     ".*thread_function.*"]
+	} elseif { $type == 2 } {
+	    set re \
+		[multi_line \
+		     ".*sleep.*" \
+		     ".*do_syscall_task .location=SIGNAL_HANDLER.*" \
+		     ".*signal_handler.*" \
+		     ".*signal handler called.*" \
+		     ".*pthread_kill.*" \
+		     ".*thread_function.*"]
+	} elseif { $type == 3 } {
+	    set re \
+		[multi_line \
+		     ".*sleep.*" \
+		     ".*do_syscall_task .location=NORMAL.*" \
+		     ".*thread_function.*"]
+	} elseif { $type == 4 } {
+	    set re \
+		[multi_line \
+		     ".*do_spin_task .location=SIGNAL_ALT_STACK.*" \
+		     ".*signal_handler.*" \
+		     ".*signal handler called.*" \
+		     ".*pthread_kill.*" \
+		     ".*thread_function.*"]
+	} elseif { $type == 5 } {
+	    set re \
+		[multi_line \
+		     ".*do_spin_task .location=SIGNAL_HANDLER.*" \
+		     ".*signal_handler.*" \
+		     ".*signal handler called.*" \
+		     ".*pthread_kill.*" \
+		     ".*thread_function.*"]
+	} elseif { $type == 6 } {
+	    set re \
+		[multi_line \
+		     ".*do_spin_task .location=NORMAL..*" \
+		     ".*thread_function.*"]
+	} elseif { $type == 7 } {
+	    set re ".*main.*"
+	} else {
+	    error "invalid type: $type"
+	}
+
+	gdb_test "thread apply $thread_num backtrace" $re
     }
 }
 
-- 
2.35.3

[PATCH v4 5/6] gdb/testsuite: Reduce gdb.threads/threadcrash.exp reliance on libc symbols

[Tom de Vries]

From: Guinevere Larsen <blarsen@redhat.com>

The test gdb.threads/threadcrash.exp demanded GDB to fully unwind and
print the names of all functions. However, some of the functions are
from the libc library, and so the test implicitly demanded libc symbols
to be available, and would fail otherwise, as was raised in PR
gdb/31293.

This commit changes it so we only explicitly check for functions that
are not provided by threadcrash.c if they are indeed available.

Tested on arm-linux and x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31293
---
 gdb/testsuite/gdb.threads/threadcrash.exp | 48 +++++++++++++++++++----
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/gdb.threads/threadcrash.exp b/gdb/testsuite/gdb.threads/threadcrash.exp
index 16fd7eb4e23..c72ce73fd6d 100644
--- a/gdb/testsuite/gdb.threads/threadcrash.exp
+++ b/gdb/testsuite/gdb.threads/threadcrash.exp
@@ -20,6 +20,9 @@
 # a gcore.
 
 
+set have_sleep -1
+set have_pthread_kill -1
+
 # Use 'thread apply all backtrace' to check if all expected threads
 # are present, and stopped in the expected locations.  Set the global
 # TEST_LIST to be the a list of regexps expected to match all the
@@ -27,6 +30,8 @@
 # GDB sees the threads.
 
 proc thread_apply_all {} {
+    global have_sleep
+    global have_pthread_kill
     global test_list
 
     set test_list { }
@@ -73,6 +78,14 @@ proc thread_apply_all {} {
 	    lappend test_list 7
 	    exp_continue
 	}
+	-re "^\r\n${hs} in sleep $hs$eol" {
+	    set have_sleep 1
+	    exp_continue
+	}
+	-re "^\r\n${hs} in pthread_kill $hs$eol" {
+	    set have_pthread_kill 1
+	    exp_continue
+	}
 	-re "^\r\n$hs$eol" {
 	    exp_continue
 	}
@@ -82,6 +95,13 @@ proc thread_apply_all {} {
     }
 
     gdb_assert {$unwind_fail == false}
+
+    if { $have_sleep == -1 } {
+	set have_sleep 0
+    }
+    if { $have_pthread_kill == -1 } {
+	set have_pthread_kill 0
+    }
 }
 
 # Perform all the tests we're interested in.  They are:
@@ -90,6 +110,8 @@ proc thread_apply_all {} {
 # * testing if GDB recreated the full backtrace we expect for all threads
 
 proc do_full_test {} {
+    global have_sleep
+    global have_pthread_kill
     global test_list
     set thread_count [get_valueof "" "\$_inferior_thread_count" 0]
     gdb_assert {$thread_count == 7}
@@ -98,6 +120,18 @@ proc do_full_test {} {
 
     gdb_assert {$thread_count == [llength $test_list]}
 
+    if { $have_sleep } {
+	set sleep ".*sleep.*"
+    } else {
+	set sleep ".*"
+    }
+
+    if { $have_pthread_kill } {
+	set pthread_kill ".*pthread_kill.*"
+    } else {
+	set pthread_kill ".*"
+    }
+
     for {set i 0} {$i < $thread_count } {incr i} {
 	set thread_num [expr [llength $test_list] - $i]
 
@@ -105,25 +139,25 @@ proc do_full_test {} {
 	if { $type == 1 } {
 	    set re \
 		[multi_line \
-		     ".sleep.*" \
+		     $sleep \
 		     ".*do_syscall_task .location=SIGNAL_ALT_STACK.*" \
 		     ".*signal_handler.*" \
 		     ".*signal handler called.*" \
-		     ".*pthread_kill.*" \
+		     $pthread_kill \
 		     ".*thread_function.*"]
 	} elseif { $type == 2 } {
 	    set re \
 		[multi_line \
-		     ".*sleep.*" \
+		     $sleep \
 		     ".*do_syscall_task .location=SIGNAL_HANDLER.*" \
 		     ".*signal_handler.*" \
 		     ".*signal handler called.*" \
-		     ".*pthread_kill.*" \
+		     $pthread_kill \
 		     ".*thread_function.*"]
 	} elseif { $type == 3 } {
 	    set re \
 		[multi_line \
-		     ".*sleep.*" \
+		     $sleep \
 		     ".*do_syscall_task .location=NORMAL.*" \
 		     ".*thread_function.*"]
 	} elseif { $type == 4 } {
@@ -132,7 +166,7 @@ proc do_full_test {} {
 		     ".*do_spin_task .location=SIGNAL_ALT_STACK.*" \
 		     ".*signal_handler.*" \
 		     ".*signal handler called.*" \
-		     ".*pthread_kill.*" \
+		     $pthread_kill \
 		     ".*thread_function.*"]
 	} elseif { $type == 5 } {
 	    set re \
@@ -140,7 +174,7 @@ proc do_full_test {} {
 		     ".*do_spin_task .location=SIGNAL_HANDLER.*" \
 		     ".*signal_handler.*" \
 		     ".*signal handler called.*" \
-		     ".*pthread_kill.*" \
+		     $pthread_kill \
 		     ".*thread_function.*"]
 	} elseif { $type == 6 } {
 	    set re \
-- 
2.35.3

[PATCH v4 6/6] gdb/testsuite: Add kfail in gdb.threads/threadcrash.exp on 32-bit arm targets

[Tom de Vries]

There is an issue with the test gdb.threads/threadcrash.exp on arm
targets, relating to how the targets handles gcores.  The issue is that GDB
can't properly backtrace from a gcore.

Add a kfail referring back to PR corefiles/31294, which tracks the issues with
gcores in 32-bit arm targets.

I cannot reproduce the problem, so this is a best effort based on the logs in
the PR.

Tested on arm-linux and x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31294
---
 gdb/testsuite/gdb.threads/threadcrash.exp | 27 +++++++++++++++--------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/gdb.threads/threadcrash.exp b/gdb/testsuite/gdb.threads/threadcrash.exp
index c72ce73fd6d..7839f5c358e 100644
--- a/gdb/testsuite/gdb.threads/threadcrash.exp
+++ b/gdb/testsuite/gdb.threads/threadcrash.exp
@@ -94,14 +94,14 @@ proc thread_apply_all {} {
 	}
     }
 
-    gdb_assert {$unwind_fail == false}
-
     if { $have_sleep == -1 } {
 	set have_sleep 0
     }
     if { $have_pthread_kill == -1 } {
 	set have_pthread_kill 0
     }
+
+    return [expr !$unwind_fail]
 }
 
 # Perform all the tests we're interested in.  They are:
@@ -109,16 +109,25 @@ proc thread_apply_all {} {
 # * Creating the list of backtraces for all threads seen
 # * testing if GDB recreated the full backtrace we expect for all threads
 
-proc do_full_test {} {
+proc do_full_test { phase } {
     global have_sleep
     global have_pthread_kill
     global test_list
     set thread_count [get_valueof "" "\$_inferior_thread_count" 0]
     gdb_assert {$thread_count == 7}
 
-    thread_apply_all
-
-    gdb_assert {$thread_count == [llength $test_list]}
+    set test thread_apply_all
+    set res [$test]
+    if { $res && $thread_count == [llength $test_list] } {
+	pass $test
+    } else {
+	set have_kfail [expr [string equal $phase gcore] && [is_aarch32_target]]
+	if { $have_kfail } {
+	    kfail $test
+	} else {
+	    fail $test
+	}
+    }
 
     if { $have_sleep } {
 	set sleep ".*sleep.*"
@@ -209,7 +218,7 @@ proc_with_prefix test_live_inferior {} {
     gdb_breakpoint "breakpt"
     gdb_continue_to_breakpoint "running to breakpoint" ".*"
 
-    do_full_test
+    do_full_test live_inferior
 }
 
 # Do all preparation steps for running the corefile tests, then
@@ -229,7 +238,7 @@ proc_with_prefix test_corefile {} {
 	     "A program is being debugged already\\\.  Kill it\\\? \\\(y or n\\\) " \
 	     "y"
 
-    do_full_test
+    do_full_test corefile
 }
 
 # Do all preparation steps for running the gcore tests, then
@@ -267,7 +276,7 @@ proc_with_prefix test_gcore {} {
 	     "A program is being debugged already\\\.  Kill it\\\? \\\(y or n\\\) " \
 	     "y"
 
-    do_full_test
+    do_full_test gcore
 }
 
 standard_testfile
-- 
2.35.3

Re: [PATCH v4 6/6] gdb/testsuite: Add kfail in gdb.threads/threadcrash.exp on 32-bit arm targets

[Tom de Vries]

On 3/8/24 10:33, Tom de Vries wrote:
> +	if { $have_kfail } {
> +	    kfail $test
> +	} else {

This needs and additional fixup:
...
diff --git a/gdb/testsuite/gdb.threads/threadcrash.exp 
b/gdb/testsuite/gdb.threads/threadcrash.exp
index 7839f5c358e..3679cdc69fa 100644
--- a/gdb/testsuite/gdb.threads/threadcrash.exp
+++ b/gdb/testsuite/gdb.threads/threadcrash.exp
@@ -123,7 +123,8 @@ proc do_full_test { phase } {
      } else {
  	set have_kfail [expr [string equal $phase gcore] && [is_aarch32_target]]
  	if { $have_kfail } {
-	    kfail $test
+	    kfail corefiles/31294 $test
+	    return
  	} else {
  	    fail $test
  	}
...

Thiago, you mentioned you could reproduce the problem, can you check 
whether this catches it?

Thanks,
- Tom

Re: [PATCH v4 6/6] gdb/testsuite: Add kfail in gdb.threads/threadcrash.exp on 32-bit arm targets

[Thiago Jung Bauermann]

Hello Tom,

Thank you for this series!

Tom de Vries <tdevries@suse.de> writes:

> On 3/8/24 10:33, Tom de Vries wrote:
>> +	if { $have_kfail } {
>> +	    kfail $test
>> +	} else {
>
> This needs and additional fixup:
> ...
> diff --git a/gdb/testsuite/gdb.threads/threadcrash.exp
> b/gdb/testsuite/gdb.threads/threadcrash.exp
> index 7839f5c358e..3679cdc69fa 100644
> --- a/gdb/testsuite/gdb.threads/threadcrash.exp
> +++ b/gdb/testsuite/gdb.threads/threadcrash.exp
> @@ -123,7 +123,8 @@ proc do_full_test { phase } {
>      } else {
>  	set have_kfail [expr [string equal $phase gcore] && [is_aarch32_target]]
>  	if { $have_kfail } {
> -	    kfail $test
> +	    kfail corefiles/31294 $test
> +	    return
>  	} else {
>  	    fail $test
>  	}
> ...
>
> Thiago, you mentioned you could reproduce the problem, can you check whether this catches
> it?

I applied the patch series plus the fixup above to upstream commit
2755241d02d7 ("Add return value to DAP scope"), and I actually can't
reproduce the problem anymore, not sure why. The threads' backtraces
look reasonable. The only "??" are in linux/arm/clone.S:74 from
libc.so.6.

I put the gdb.log here, if there's interest:

https://people.linaro.org/~thiago.bauermann/gdb-pr-31294/gdb.log

--
Thiago

Re: [PATCH v4 6/6] gdb/testsuite: Add kfail in gdb.threads/threadcrash.exp on 32-bit arm targets

[Tom de Vries]

On 3/9/24 00:57, Thiago Jung Bauermann wrote:
> 
> Hello Tom,
> 
> Thank you for this series!
> 
> Tom de Vries <tdevries@suse.de> writes:
> 
>> On 3/8/24 10:33, Tom de Vries wrote:
>>> +	if { $have_kfail } {
>>> +	    kfail $test
>>> +	} else {
>>
>> This needs and additional fixup:
>> ...
>> diff --git a/gdb/testsuite/gdb.threads/threadcrash.exp
>> b/gdb/testsuite/gdb.threads/threadcrash.exp
>> index 7839f5c358e..3679cdc69fa 100644
>> --- a/gdb/testsuite/gdb.threads/threadcrash.exp
>> +++ b/gdb/testsuite/gdb.threads/threadcrash.exp
>> @@ -123,7 +123,8 @@ proc do_full_test { phase } {
>>       } else {
>>   	set have_kfail [expr [string equal $phase gcore] && [is_aarch32_target]]
>>   	if { $have_kfail } {
>> -	    kfail $test
>> +	    kfail corefiles/31294 $test
>> +	    return
>>   	} else {
>>   	    fail $test
>>   	}
>> ...
>>
>> Thiago, you mentioned you could reproduce the problem, can you check whether this catches
>> it?
> 
> I applied the patch series plus the fixup above to upstream commit
> 2755241d02d7 ("Add return value to DAP scope"), and I actually can't
> reproduce the problem anymore, not sure why. The threads' backtraces
> look reasonable. The only "??" are in linux/arm/clone.S:74 from
> libc.so.6.
> 
> I put the gdb.log here, if there's interest:
> 
> https://people.linaro.org/~thiago.bauermann/gdb-pr-31294/gdb.log

Hi Thiago,

thanks for checking.

Ideally, we need to either:
- confirm that the problem still exists, and test and commit the kfail,
   or
- confirm that the problem doesn't exist, drop the kfail patch and close
   the PR.

So, can you try to confirm one or the other, f.i. going back to an 
earlier commit where you were able to reproduce the problem, running in 
a loop, try with/without debug info, etc.  AFAIU, you're the only one 
who can do this because the behaviour hasn't been spotted outside linaro.

If it turns out that we can't confirm either existence or non-existence, 
I suppose we'll have to drop the kfail patch, leave the PR open and wait 
for it to appear again.

I went ahead and committed the series apart from this patch, which 
reflects that situation.

Thanks,
- Tom

Re: [PATCH v4 6/6] gdb/testsuite: Add kfail in gdb.threads/threadcrash.exp on 32-bit arm targets

[Thiago Jung Bauermann]

Hello Tom,

Tom de Vries <tdevries@suse.de> writes:
> On 3/9/24 00:57, Thiago Jung Bauermann wrote:
>>> On 3/8/24 10:33, Tom de Vries wrote:
>>>
>>> Thiago, you mentioned you could reproduce the problem, can you check whether this catches
>>> it?
>> I applied the patch series plus the fixup above to upstream commit
>> 2755241d02d7 ("Add return value to DAP scope"), and I actually can't
>> reproduce the problem anymore, not sure why. The threads' backtraces
>> look reasonable. The only "??" are in linux/arm/clone.S:74 from
>> libc.so.6.
>> I put the gdb.log here, if there's interest:
>> https://people.linaro.org/~thiago.bauermann/gdb-pr-31294/gdb.log
>
> thanks for checking.
>
> Ideally, we need to either:
> - confirm that the problem still exists, and test and commit the kfail,
>   or
> - confirm that the problem doesn't exist, drop the kfail patch and close
>   the PR.
>
> So, can you try to confirm one or the other, f.i. going back to an earlier commit where
> you were able to reproduce the problem, running in a loop, try with/without debug info,
> etc.  AFAIU, you're the only one who can do this because the behaviour hasn't been spotted
> outside linaro.

Yes, of course. I'll see what's going on and determine whether the
problem has really been fixed.

> I went ahead and committed the series apart from this patch, which reflects that
> situation.

Thank you!

--
Thiago

Re: [PATCH v4 6/6] gdb/testsuite: Add kfail in gdb.threads/threadcrash.exp on 32-bit arm targets

[Thiago Jung Bauermann]

Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
> Tom de Vries <tdevries@suse.de> writes:
>> Ideally, we need to either:
>> - confirm that the problem still exists, and test and commit the kfail,
>>   or
>> - confirm that the problem doesn't exist, drop the kfail patch and close
>>   the PR.
>>
>> So, can you try to confirm one or the other, f.i. going back to an earlier commit where
>> you were able to reproduce the problem, running in a loop, try with/without debug info,
>> etc.  AFAIU, you're the only one who can do this because the behaviour hasn't been
>> spotted
>> outside linaro.
>
> Yes, of course. I'll see what's going on and determine whether the
> problem has really been fixed.

I confirmed that the problem doesn't happen anymore, and a git bisect
found that commit 9c0aa4c53104 ("Fix disabling of year 2038 support on
32-bit hosts by default") fixed it.

I updated PR 31294 with this information.

--
Thiago