[v2,3/4] gdb/testsuite: fix testing gdb.reverse/step-reverse.exp with clang
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
Commit Message
When testing using reverse-stepi to fully step through a function, the
code checks for an infinite loop by seeing if we land on the line that
contains the return statement multiple times. This assumption only works
if there is only one instruction associated with that line, which is how
GCC handles line information, but other compilers may handle it differently.
Clang-15, for instance, associates 6. Because of this, the inferior used
to get seriously out of sync with the test expectations, and result in 13
spurious failures. The same issue occurs with gdb.reverse/step-precsave.exp.
This commit changes the test so that we check for PC instead of line
number. The test still only happens when the same line is detected, to
simplify the resulting log. With this change, no new failures are
emitted when using clang.
It also adds a new parameter to get_hexadecimal_valueof, so that we can
use it without generating new passes, otherwise we'd get multiple
duplicate test names. This change shouldn't affect any other test using
this proc.
---
gdb/testsuite/gdb.reverse/step-precsave.exp | 18 +++++++++++++++++-
gdb/testsuite/gdb.reverse/step-reverse.exp | 20 +++++++++++++++++++-
gdb/testsuite/lib/gdb.exp | 6 ++++--
3 files changed, 40 insertions(+), 4 deletions(-)
Comments
>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
Bruno> It also adds a new parameter to get_hexadecimal_valueof, so that we can
Bruno> use it without generating new passes, otherwise we'd get multiple
Bruno> duplicate test names. This change shouldn't affect any other test using
Bruno> this proc.
You can just pass different test names instead.
Bruno> +proc get_current_pc {} {
Bruno> + set pc 0
Bruno> + gdb_test_multiple "print \$pc" "" {
Bruno> + -re -wrap ".*0x(\[0-9a-f\]+).*" {
Bruno> + set pc $expect_out(1,string)
Bruno> + }
Bruno> + }
Bruno> + return $pc
It seems to me that this shouldn't be needed.
Bruno> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
Bruno> index 4b78a8f8fb7..9ff97bfde42 100644
Bruno> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
Bruno> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
Bruno> @@ -28,6 +28,16 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
Bruno> return -1
Bruno> }
Bruno> +proc get_current_pc {} {
Bruno> + set pc 0
Bruno> + gdb_test_multiple "print \$pc" "" {
Bruno> + -re -wrap ".*0x(\[0-9a-f\]+).*" {
Bruno> + set pc $expect_out(1,string)
Bruno> + }
Bruno> + }
Bruno> + return $pc
Same with this one.
Tom
On 28/07/2023 15:14, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> Bruno> It also adds a new parameter to get_hexadecimal_valueof, so that we can
> Bruno> use it without generating new passes, otherwise we'd get multiple
> Bruno> duplicate test names. This change shouldn't affect any other test using
> Bruno> this proc.
>
> You can just pass different test names instead.
I mean, yeah I can, but since it is in a loop, the differences would
only be a counter at the end of the test case. Are we really getting any
value from that? To me it seems like it would just boggle down the sum
file with meaningless "tests" that aren't exercising any relevant code
paths. I can do it if you disagree, though, it isn't a big deal.
One thing I did think of was that I should test for default value when
getting the PC, to make sure that we didnt get a valid PC at first, then
started getting "no registers" or similar. I'll have that on v3
>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
Bruno> I mean, yeah I can, but since it is in a loop, the differences would
Bruno> only be a counter at the end of the test case. Are we really getting
Bruno> any value from that? To me it seems like it would just boggle down the
Bruno> sum file with meaningless "tests" that aren't exercising any relevant
Bruno> code paths. I can do it if you disagree, though, it isn't a big deal.
Yeah, I don't know if it's useful -- but are any of these passes useful?
The problem I think is that the proc can unconditionally emit a fail, so
having a corresponding pass seems to make sense.
Tom
On 28/07/2023 16:18, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> Bruno> I mean, yeah I can, but since it is in a loop, the differences would
> Bruno> only be a counter at the end of the test case. Are we really getting
> Bruno> any value from that? To me it seems like it would just boggle down the
> Bruno> sum file with meaningless "tests" that aren't exercising any relevant
> Bruno> code paths. I can do it if you disagree, though, it isn't a big deal.
>
> Yeah, I don't know if it's useful -- but are any of these passes useful?
> The problem I think is that the proc can unconditionally emit a fail, so
> having a corresponding pass seems to make sense.
>
Ah, yeah, in that angle it does make sense. I'll do it for v3!
@@ -30,6 +30,16 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
return -1
}
+proc get_current_pc {} {
+ set pc 0
+ gdb_test_multiple "print \$pc" "" {
+ -re -wrap ".*0x(\[0-9a-f\]+).*" {
+ set pc $expect_out(1,string)
+ }
+ }
+ return $pc
+}
+
runto_main
# Activate process record/replay
@@ -209,11 +219,17 @@ gdb_test_multiple "stepi" "$test_message" {
# stepi backward out of a function call
+set start_pc [get_current_pc]
set stepi_location [gdb_get_line_number "STEPI TEST" "$srcfile"]
set test_message "reverse stepi from a function call"
gdb_test_multiple "stepi" "$test_message" {
-re "ARRIVED IN CALLEE.*$gdb_prompt $" {
- fail "$test_message (start statement)"
+ if { [get_current_pc] == $start_pc } {
+ fail "$test_message (start statement)"
+ } else {
+ send_gdb "stepi\n"
+ exp_continue
+ }
}
-re "ENTER CALLEE.*$gdb_prompt $" {
send_gdb "stepi\n"
@@ -28,6 +28,16 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
return -1
}
+proc get_current_pc {} {
+ set pc 0
+ gdb_test_multiple "print \$pc" "" {
+ -re -wrap ".*0x(\[0-9a-f\]+).*" {
+ set pc $expect_out(1,string)
+ }
+ }
+ return $pc
+}
+
runto_main
if [supports_process_record] {
@@ -174,11 +184,19 @@ gdb_test_multiple "stepi" "$test_message" {
# stepi backward out of a function call
+# When testing stepi, we dont want to infinitely step if we're not moving
+# so we store the starting PC, in case we land on the same line as above
+set start_pc [get_hexadecimal_valueof "\$pc" 0 "" "false"]
set stepi_location [gdb_get_line_number "STEPI TEST" "$srcfile"]
set test_message "reverse stepi from a function call"
gdb_test_multiple "stepi" "$test_message" {
-re "ARRIVED IN CALLEE.*$gdb_prompt $" {
- fail "$test_message (start statement)"
+ if { [get_hexadecimal_valueof "\$pc" 0 "" "false"] == $start_pc } {
+ fail "$test_message (start statement)"
+ } else {
+ send_gdb "stepi\n"
+ exp_continue
+ }
}
-re "ENTER CALLEE.*$gdb_prompt $" {
send_gdb "stepi\n"
@@ -7919,7 +7919,7 @@ proc get_integer_valueof { exp default {test ""} } {
# TEST is the test message to use. It can be omitted, in which case
# a test message is built from EXP.
-proc get_hexadecimal_valueof { exp default {test ""} } {
+proc get_hexadecimal_valueof { exp default {test ""} {pass true} } {
global gdb_prompt
if {$test == ""} {
@@ -7930,7 +7930,9 @@ proc get_hexadecimal_valueof { exp default {test ""} } {
gdb_test_multiple "print /x ${exp}" $test {
-re "\\$\[0-9\]* = (0x\[0-9a-zA-Z\]+).*$gdb_prompt $" {
set val $expect_out(1,string)
- pass "$test"
+ if { "$pass" == "true" } {
+ pass "$test"
+ }
}
}
return ${val}