RE: [PATCH v6 7/7] adapt btrace testcases for arm target

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Zied,

>+   This file has been generated on an armv8 machine using:
>+   gcc -m32 -S -O2 -dA -g tailcall-only.c -o aarch64-tailcall-only.S

Does the aarch64- prefix still hold when compiled with -m32?


>diff --git a/gdb/testsuite/gdb.btrace/buffer-size.exp
>b/gdb/testsuite/gdb.btrace/buffer-size.exp
>index ea4e36c1593..23f859dce9c 100644
>--- a/gdb/testsuite/gdb.btrace/buffer-size.exp
>+++ b/gdb/testsuite/gdb.btrace/buffer-size.exp
>@@ -32,10 +32,17 @@ if ![runto_main] {
>     return -1
> }
>
>-gdb_test_no_output "set record btrace bts buffer-size 1"
>-gdb_test_no_output "set record btrace pt buffer-size 1"
>-gdb_test "show record btrace bts buffer-size" "The record/replay bts buffer size
>is 1\.\r"
>-gdb_test "show record btrace pt buffer-size" "The record/replay pt buffer size is
>1\.\r"
>+set btrace_type ""
>+if { [istarget "i?86-*-*"] || [istarget "x86_64-*-*"]} {
>+    set btrace_type { "bts" "pt" }
>+} elseif {[istarget "arm*-*-*"]|| [istarget "aarch64*-*-*"]} {
>+    set btrace_type { "etm" }
>+}

There shouldn't be a need for this.  The config should work for all formats, even
if the target does not support it.  We should be able to run the below loop over
all existing formats.


>diff --git a/gdb/testsuite/gdb.btrace/instruction_history.exp
>b/gdb/testsuite/gdb.btrace/instruction_history.exp
>index 403085c083f..3a6e7362f74 100644
>--- a/gdb/testsuite/gdb.btrace/instruction_history.exp
>+++ b/gdb/testsuite/gdb.btrace/instruction_history.exp
>@@ -22,7 +22,17 @@ if { [skip_btrace_tests] } {
>     return -1
> }
>
>-standard_testfile .c .S
>+set test_prefix ""
>+if { [istarget "i?86-*-*"] || [istarget "x86_64-*-*"]} {
>+    set test_prefix "x86"
>+} elseif {[istarget "arm*-*-*"]} {
>+    set test_prefix "arm"
>+} elseif {[istarget "aarch64*-*-*"]} {
>+    set test_prefix "aarch64"
>+}

We should error out to give a more meaningful error message in case
we cannot determine the prefix.


>diff --git a/gdb/testsuite/gdb.btrace/non-stop.exp
>b/gdb/testsuite/gdb.btrace/non-stop.exp
>index e509d65d660..fbc4cda7dd6 100644
>--- a/gdb/testsuite/gdb.btrace/non-stop.exp
>+++ b/gdb/testsuite/gdb.btrace/non-stop.exp
>@@ -31,6 +31,12 @@ save_vars { GDBFLAGS } {
>     clean_restart $testfile
> }
>
>+if {[istarget "i?86-*-*"] || [istarget "x86_64-*-*"]} {
>+    set loop_position 2
>+} elseif {[istarget "arm*-*-*"] || [istarget "aarch64*-*-*"]} {
>+    set loop_position 3
>+}
>+
> if ![runto_main] {
>     untested "failed to run to main"
>     return -1
>@@ -111,87 +117,99 @@ gdb_test "thread apply all info rec" ".*"
> gdb_test "info threads" ".*"
>
> with_test_prefix "navigate" {
>-    gdb_test "thread apply 1 record goto 3" "$loop_line"
>-    gdb_test "thread apply 2 record goto 4" "$loop_line"
>+    gdb_test "thread apply 1 record goto [expr {$loop_position + 1}]"
>"$loop_line"
>+    gdb_test "thread apply 2 record goto [expr {$loop_position + 2}]"
>"$loop_line"

This test is already making assumptions about the compiled code.

I'd rather we started at 4 for all architectures instead of having different
starting points.  We may also adjust the tests so we don't go beyond 12,
which seems to be working for all IA targets we're running this on.  We
can also try to stay below 10 to be on the safe side.

Could you propose something that works for ARM in v7 of the series?
I can validate that on IA, then, before this gets merged.  If we stay between
3 and 12, however, I don't see why it shouldn't continue working.


>+if {[istarget "i?86-*-*"] || [istarget "x86_64-*-*"]} {
>+    set function_positions(0) 19
>+    set function_positions(1) 27
>+    set function_positions(2) 2
>+    set function_positions(end) 40
>+    set function_positions(3) 39
>+
>+    set sequence_begin(1) 1
>+    set sequence_end(1) 1
>+    set sequence_begin(2) 2
>+    set sequence_end(2) 4

That's too cryptic.  If we cannot tweak the assembly files to work
with the same .exp, I'd rather split the latter into arch-specific .exp's.

Also for other tests below.


> if [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] {
>@@ -62,6 +70,10 @@ gdb_test_no_output "set record function-call-history-size
>0"
>
> # trace foo
> gdb_test "step" ".*" "prepare for recording"
>+# make sure we get out of function epilogue
>+if { [istarget "arm*-*-*"] } {
>+  gdb_test "stepi"
>+}

We can also work with breakpoints if stepping isn't producing reliable
results.


>     lappend opts debug optimize=-O2
>-} elseif {[istarget "i?86-*-*"] || [istarget "x86_64-*-*"]} {
>+} else {
>+  set test_prefix ""
>+  if { [istarget "i?86-*-*"] || [istarget "x86_64-*-*"]} {

Extra space between { and [.  Let's be consistent in formatting.


> if [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] {
>     return -1
> }
>+
>+if {[istarget "i?86-*-*"] || [istarget "x86_64-*-*"]} {
>+    set bar_return_line 24
>+    set bar_return_position 4
>+    set main_return_line 38
>+} elseif {[istarget "arm*-*-*"]} {
>+    set bar_return_line 24
>+    set bar_return_position 5
>+    set main_return_line 41
>+    } elseif {[istarget "aarch64*-*-*"]} {
>+    set bar_return_line 23
>+    set bar_return_position 6
>+    set main_return_line 40
>+}

Why would compilers not agree on the source line of the return statement?

I can see that the "record goto 4" is causing trouble.  Can we just tweak that
number?  I.e. find another place inside the trace that produces the same bt?

We can also try to work with breakpoints again, instead of hard-coding an
instruction number.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928