[commit/RFC] fix gdb.trace/collection.exp bitrot

[commit/RFC] fix gdb.trace/collection.exp bitrot

[Pedro Alves]

I was adding a small test to gdb/trace/collection.exp for some
other random tracing bug fix, and noticed that it actually
never ran against gdbserver with tracepoints support.  The
test assumes that after gdb_load, which isn't true for all
board files, or native testing.  It's also not true with
the local gdbserver testing boardfile found in the wiki,
that most people use.  The test was then always being
skipped with that common pattern in gdb.trace/ tests:

 # We generously give ourselves one "pass" if we successfully
 # detect that this test cannot be run on this target!
 if { ![gdb_target_supports_trace] } then {
     pass "Current target does not support trace"
     return 1;
 }

The other bitrot issue fixed by the patch, is, on some
targets, printing $sp prints the pointer type in addition
to the value, like so:

 (top-gdb) p $sp
 $1 = (void *) 0x7fffffffe030

Printing $pc prints even more things:

 (top-gdb) p $pc
 $1 = (void (*)()) 0x456d53 <main+15>

The test isn't expecting this.  The fix is just to 
do "print /x" instead:

 (top-gdb) p /x $sp
 $1 = 0x7fffffffe030

 (top-gdb) p /x $pc
 $1 = 0x456d53

I've applied the patch below to fix these issues.

However, the test still has these FAILs:

 Running ../../../src/gdb/testsuite/gdb.trace/collection.exp ...
 FAIL: gdb.trace/collection.exp: collect args collectively: collected argarray #0
 FAIL: gdb.trace/collection.exp: collect args collectively: collected argarray #1
 FAIL: gdb.trace/collection.exp: collect args collectively: collected argarray #2
 FAIL: gdb.trace/collection.exp: collect args collectively: collected argarray #3
 FAIL: gdb.trace/collection.exp: collect args individually: collected argarray #0
 FAIL: gdb.trace/collection.exp: collect args individually: collected argarray #1
 FAIL: gdb.trace/collection.exp: collect args individually: collected argarray #2
 FAIL: gdb.trace/collection.exp: collect args individually: collected argarray #3
 FAIL: gdb.trace/collection.exp: collect argarray collectively: collected argarray #0
 FAIL: gdb.trace/collection.exp: collect argarray collectively: collected argarray #1
 FAIL: gdb.trace/collection.exp: collect argarray collectively: collected argarray #2
 FAIL: gdb.trace/collection.exp: collect argarray collectively: collected argarray #3
 FAIL: gdb.trace/collection.exp: collect argarray individually: collected argarray #0
 FAIL: gdb.trace/collection.exp: collect argarray individually: collected argarray #1
 FAIL: gdb.trace/collection.exp: collect argarray individually: collected argarray #2
 FAIL: gdb.trace/collection.exp: collect argarray individually: collected argarray #3
 
                 === gdb Summary ===
 
 # of expected passes            361
 # of unexpected failures        16

[to be clear, you can't see these if your gdbserver
doesn't supports tracepoints yet, nor can you see these with
native testing]

All the failures in the test look similar to this:

 print argarray[0]
 Cannot access memory at address 0x7fff138c9aa0
 (gdb) FAIL: gdb.trace/collection.exp: collect args collectively: collected argarray #0

That is, the tracepoint was set to collect the `argarray' argument, of:

 /* Test collecting args. */
 int args_test_func (argc, argi, argf, argd, argstruct, argarray)
      char   argc;
      int    argi;
      float  argf;
      double argd;
      test_struct argstruct;
      int argarray[4];
 {

But, array passing in C is special; even though the argument is
declared like an array, only a pointer to the array passed in.

So, collecting `argarray' only collects the array address, not its
contents.  But, the test tried to print the array's contents, and,
that fails.

The question is.  What to do then?  Should the test just be
fixed to not assume that collecting an array argument, collects
the whole array?  I believe so.

Regular printing also just prints the pointer:

 (gdb) frame
 #0  args_test_func (argc=1 '\001', argi=2, argf=3.29999995, argd=4.4000000000000004, argstruct=...,
     argarray=0x7fffffffe010) at ../../../src/gdb/testsuite/gdb.trace/collection.c:65
 (gdb) p argarray
 $1 = (int *) 0x7fffffffe010
 (gdb) up
 #1  0x0000000000400cb0 in main (argc=1, argv=0x7fffffffe118, envp=0x7fffffffe128)
     at ../../../src/gdb/testsuite/gdb.trace/collection.c:242
 (gdb) p myarray
 $2 = {111, 112, 113, 114}
 (gdb) p &
 $3 = (int (*)[4]) 0x7fffffffe010
 (gdb)

This matches the language semantics.  If the user
wants to collect the array contents, she should do
so explicitly.

I also tried compiling the test with stabs+, just in case and
I see the same.  Maybe it was different at some point.

-- 
Pedro Alves

2010-03-29  Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
	* gdb.trace/collection.exp (executable): New.
	(binfile): Use it.
	(fpreg, spreg, pcreg): New.
	(test_register): Use gdb_test_multiple.  Pass /x to print.
	(prepare_for_trace_test): New.
	(run_trace_experiment): Use "continue", not gdb_run_cmd.
	(gdb_collect_args_test, gdb_collect_argstruct_test)
	(gdb_collect_argarray_test, gdb_collect_locals_test): Use
	prepare_for_trace_test.
	(gdb_collect_registers_test): Use prepare_for_trace_test.  Use
	fpreg, spreg and pcreg.
	(gdb_collect_expression_test, gdb_collect_globals_test): Use
	prepare_for_trace_test.
	(gdb_trace_collection_test): Use fpreg, spreg and pcreg.  Don't
	try to detect tracing support here.  Don't set breakpoints at
	`begin' or `end' here.
	<global scope>: Use clean_restart.  Run to main before checking
	for tracing support.  Check for for tracing support here.

---
 gdb/testsuite/gdb.trace/collection.exp |  143 ++++++++++++++-------------------
 1 file changed, 62 insertions(+), 81 deletions(-)

Index: src/gdb/testsuite/gdb.trace/collection.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.trace/collection.exp	2010-03-28 22:47:37.000000000 +0100
+++ src/gdb/testsuite/gdb.trace/collection.exp	2010-03-28 23:38:28.000000000 +0100
@@ -24,7 +24,8 @@ set bug_id 0
 
 set testfile "collection"
 set srcfile ${testfile}.c
-set binfile $objdir/$subdir/$testfile
+set executable $testfile
+set binfile $objdir/$subdir/$executable
 
 if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
 	executable {debug nowarnings}] != "" } {
@@ -45,6 +46,20 @@ if { [gdb_compile "$srcdir/$subdir/$srcf
 set ws "\[\r\n\t \]+"
 set cr "\[\r\n\]+"
 
+if [istarget "x86_64-*"] then {
+    set fpreg "rbp"
+    set spreg "rsp"
+    set pcreg "rip"
+} elseif [istarget "i?86-*"] then {
+    set fpreg "ebp"
+    set spreg "esp"
+    set pcreg "eip"
+} else {
+    set fpreg "fp"
+    set spreg "sp"
+    set pcreg "pc"
+}
+
 #
 # Utility procs
 #
@@ -53,36 +68,35 @@ proc test_register { reg test_id } {
     global cr
     global gdb_prompt
 
-    send_gdb "print $reg\n"
-    gdb_expect {
-	-re "\\$\[0-9\]+ = \[x0\]+$cr$gdb_prompt " {
+    gdb_test_multiple "print /x $reg" "" {
+	-re "\\$\[0-9\]+ = \[x0\]+$cr$gdb_prompt $" {
 	    fail "collect $test_id: collected $reg (zero)"
 	}
-	-re "\\$\[0-9\]+ = \[x0-9a-fA-F\]+$cr$gdb_prompt " {
+	-re "\\$\[0-9\]+ = \[x0-9a-fA-F\]+$cr$gdb_prompt $" {
 	    pass "collect $test_id: collected $reg"
 	}
-	-re "\[Ee\]rror.*$gdb_prompt " {
+	-re "\[Ee\]rror.*$gdb_prompt $" {
 	    fail "collect $test_id: collected $reg (error)"
 	}
-	timeout {
-	    fail "collect $test_id: collected $reg (timeout)"
-	}
     }
 }
 
+proc prepare_for_trace_test {} {
+    global executable
+
+    clean_restart $executable
+
+    runto_main
+
+    gdb_test "break begin" "" ""
+    gdb_test "break end" "" ""
+}
+
 proc run_trace_experiment { msg test_func } {
-    global gdb_prompt
-    gdb_run_cmd
-    gdb_expect {
-	-re ".*Breakpoint \[0-9\]+, begin .*$gdb_prompt $" { 
-	}
-	-re ".*$gdb_prompt $" { 
-	    fail "collect $msg: advance to go"
-	}
-	timeout { 
-	    fail "collect $msg: advance to go (timeout)"
-	}
-    }
+    gdb_test "continue" \
+	".*Breakpoint \[0-9\]+, begin .*" \
+	"collect $msg: advance to begin"
+
     gdb_test "tstart" \
 	    "\[\r\n\]+" \
 	    "collect $msg: start trace experiment"
@@ -106,10 +120,7 @@ proc gdb_collect_args_test { myargs msg 
     global cr
     global gdb_prompt
 
-    # Make sure we're in a sane starting state.
-    gdb_test "tstop" "" ""
-    gdb_test "tfind none" "" ""
-    gdb_delete_tracepoints
+    prepare_for_trace_test
 
     gdb_test "trace args_test_func" \
 	    "Tracepoint \[0-9\]+ at .*" \
@@ -171,10 +182,7 @@ proc gdb_collect_argstruct_test { myargs
     global cr
     global gdb_prompt
 
-    # Make sure we're in a sane starting state.
-    gdb_test "tstop" "" ""
-    gdb_test "tfind none" "" ""
-    gdb_delete_tracepoints
+    prepare_for_trace_test
 
     gdb_test "trace argstruct_test_func" \
 	    "Tracepoint \[0-9\]+ at .*" \
@@ -210,10 +218,7 @@ proc gdb_collect_argarray_test { myargs 
     global cr
     global gdb_prompt
 
-    # Make sure we're in a sane starting state.
-    gdb_test "tstop" "" ""
-    gdb_test "tfind none" "" ""
-    gdb_delete_tracepoints
+    prepare_for_trace_test
 
     gdb_test "trace argarray_test_func" \
 	    "Tracepoint \[0-9\]+ at .*" \
@@ -249,10 +254,7 @@ proc gdb_collect_locals_test { func mylo
     global cr
     global gdb_prompt
 
-    # Make sure we're in a sane starting state.
-    gdb_test "tstop" "" ""
-    gdb_test "tfind none" "" ""
-    gdb_delete_tracepoints
+    prepare_for_trace_test
 
     # Find the comment-identified line for setting this tracepoint.
     set testline 0
@@ -330,11 +332,11 @@ proc gdb_collect_locals_test { func mylo
 proc gdb_collect_registers_test { myregs } {
     global cr
     global gdb_prompt
+    global fpreg
+    global spreg
+    global pcreg
 
-    # Make sure we're in a sane starting state.
-    gdb_test "tstop" "" ""
-    gdb_test "tfind none" "" ""
-    gdb_delete_tracepoints
+    prepare_for_trace_test
 
     # We'll simply re-use the args_test_function for this test
     gdb_test "trace args_test_func" \
@@ -347,9 +349,9 @@ proc gdb_collect_registers_test { myregs
     # Begin the test.
     run_trace_experiment $myregs args_test_func
 
-    test_register "\$fp" $myregs
-    test_register "\$sp" $myregs
-    test_register "\$pc" $myregs
+    test_register "\$$fpreg" $myregs
+    test_register "\$$spreg" $myregs
+    test_register "\$$pcreg" $myregs
 
     gdb_test "tfind none" \
 	    "#0  end .*" \
@@ -360,10 +362,7 @@ proc gdb_collect_expression_test { func 
     global cr
     global gdb_prompt
 
-    # Make sure we're in a sane starting state.
-    gdb_test "tstop" "" ""
-    gdb_test "tfind none" "" ""
-    gdb_delete_tracepoints
+    prepare_for_trace_test
 
     # Find the comment-identified line for setting this tracepoint.
     set testline 0
@@ -406,10 +405,7 @@ proc gdb_collect_globals_test { } {
     global cr
     global gdb_prompt
 
-    # Make sure we're in a sane starting state.
-    gdb_test "tstop" "" ""
-    gdb_test "tfind none" "" ""
-    gdb_delete_tracepoints
+    prepare_for_trace_test
 
     # Find the comment-identified line for setting this tracepoint.
     set testline 0
@@ -485,21 +481,11 @@ proc gdb_collect_globals_test { } {
 	    "collect globals: cease trace debugging"
 }
 
-proc gdb_trace_collection_test { } {
-    global gdb_prompt;
-
-    gdb_test "set width 0" "" ""
-    delete_breakpoints
-
-    # We generously give ourselves one "pass" if we successfully 
-    # detect that this test cannot be run on this target!
-    if { ![gdb_target_supports_trace] } then { 
-	pass "Current target does not support trace"
-	return 1;
-    }
+proc gdb_trace_collection_test {} {
+    global fpreg
+    global spreg
+    global pcreg
 
-    gdb_test "break begin" "" ""
-    gdb_test "break end"   "" ""
     gdb_collect_args_test "\$args" \
 	    "args collectively"
     gdb_collect_args_test "argc, argi, argf, argd, argstruct, argarray" \
@@ -527,9 +513,8 @@ proc gdb_trace_collection_test { } {
     gdb_collect_locals_test statlocal_test_func \
 	    "locc, loci, locf, locd, locst, locar" \
 	    "static locals individually"
-    
     gdb_collect_registers_test "\$regs"
-    gdb_collect_registers_test "\$fp, \$sp, \$pc"
+    gdb_collect_registers_test "\$$fpreg, \$$spreg, \$$pcreg"
     gdb_collect_globals_test
     
     #
@@ -605,22 +590,18 @@ proc gdb_trace_collection_test { } {
 
 }
 
-# Start with a fresh gdb.
- 
-gdb_exit
-gdb_start
-gdb_reinitialize_dir $srcdir/$subdir
-gdb_load $binfile
- 
-if [target_info exists gdb_stub] {
-    gdb_step_for_stub;
+clean_restart $executable
+runto_main
+
+# We generously give ourselves one "pass" if we successfully
+# detect that this test cannot be run on this target!
+if { ![gdb_target_supports_trace] } then {
+    pass "Current target does not support trace"
+    return 1;
 }
- 
+
 # Body of test encased in a proc so we can return prematurely.
 gdb_trace_collection_test
 
 # Finished!
 gdb_test "tfind none" "" ""
-
-
-

Re: [commit/RFC] fix gdb.trace/collection.exp bitrot

[Pedro Alves]

On Monday 29 March 2010 00:45:17, Pedro Alves wrote:
> I was adding a small test to gdb/trace/collection.exp for some

I meant, gdb.trace/collection.exp.

> other random tracing bug fix, and noticed that it actually
> never ran against gdbserver with tracepoints support.  The
> test assumes that after gdb_load, which isn't true for all

... test assumes that after gdb_load, GDB is already connected
to a running target, ... which isn't true ...

> board files, or native testing.  It's also not true with
> the local gdbserver testing boardfile found in the wiki,
> that most people use.  The test was then always being
> skipped with that common pattern in gdb.trace/ tests:

-- 
Pedro Alves

Re: [commit/RFC] fix gdb.trace/collection.exp bitrot

[Stan Shebs]

Pedro Alves wrote:
> That is, the tracepoint was set to collect the `argarray' argument, of:
>
>  /* Test collecting args. */
>  int args_test_func (argc, argi, argf, argd, argstruct, argarray)
>       char   argc;
>       int    argi;
>       float  argf;
>       double argd;
>       test_struct argstruct;
>       int argarray[4];
>  {
>
> But, array passing in C is special; even though the argument is
> declared like an array, only a pointer to the array passed in.
>
> So, collecting `argarray' only collects the array address, not its
> contents.  But, the test tried to print the array's contents, and,
> that fails.
>
> The question is.  What to do then?  Should the test just be
> fixed to not assume that collecting an array argument, collects
> the whole array?  I believe so.
>   

My offhand guess is that the ABI is playing a role somehow, where 
sufficiently large arrays are being passed by reference, and the type 
info going into the collection is describing it as a pointer, rather 
than an array.

I don't have a good sense about what collection *should* do though.  For 
instance, in the case of structures, a field that is an array is 
collected in its entirety.  Now collection does that as a sort of 
accident of the implementation; an array in a struct occupies bytes M 
through N of the struct's value, and so it just happens to ride along 
with the rest of the struct's fields.  But conversely, when it comes to 
strings, collection doesn't try to guess at how many bytes of the string 
to collect, it just takes the pointer.

There are a couple user-friendliness reasons to collect all the array 
elements. One, it matches what the user sees in the source code - users 
don't want to have to guess at the implementation du jour, or worse, 
have to write different tracepoint actions depending on whether a 
#define for array size expands into 2 or 20.  Second, "collect $args" 
will work as expected, and not have to be written as "collect $args, 
argarray[0]@4" (which would be hard to get right if the 4 came from a 
macro).  Third, if collection collects a little more than is strictly 
necessary, then it improves the chances that trace frames will have the 
right data on the first try, instead of requiring a re-run.

My inclination for now is just to mark as known failures, so as not to 
get too bogged down.

Stan

Re: [commit/RFC] fix gdb.trace/collection.exp bitrot

[Pedro Alves]

On Wednesday 31 March 2010 22:31:36, Stan Shebs wrote:
> Pedro Alves wrote:
> > That is, the tracepoint was set to collect the `argarray' argument, of:
> >
> >  /* Test collecting args. */
> >  int args_test_func (argc, argi, argf, argd, argstruct, argarray)
> >       char   argc;
> >       int    argi;
> >       float  argf;
> >       double argd;
> >       test_struct argstruct;
> >       int argarray[4];
> >  {
> >
> > But, array passing in C is special; even though the argument is
> > declared like an array, only a pointer to the array passed in.
> >
> > So, collecting `argarray' only collects the array address, not its
> > contents.  But, the test tried to print the array's contents, and,
> > that fails.
> >
> > The question is.  What to do then?  Should the test just be
> > fixed to not assume that collecting an array argument, collects
> > the whole array?  I believe so.
> >   
> 
> My offhand guess is that the ABI is playing a role somehow, where 
> sufficiently large arrays are being passed by reference, and the type 
> info going into the collection is describing it as a pointer, rather 
> than an array.

I don't think this depends on ABI.  AFAIK, arrays in C are always
passed to functions as pointers; you never get a copy of the
caller's array in the callee.  See:

 <http://c-faq.com/~scs/cclass/notes/sx10f.html>.

To do that (pass an array copy, for some reason), the trick is
usually to wrap the array in a struct.  One gets used to think
arrays and pointers are the same thing --- arrays do decay to
pointers as necessary, but they're not the same thing.

My guess is this is strictly debug info related.  Maybe the
compiler the test was written for described the "array"
argument really as an array.

> There are a couple user-friendliness reasons to collect all the array 
> elements. One, it matches what the user sees in the source code - users 
> don't want to have to guess at the implementation du jour, or worse, 
> have to write different tracepoint actions depending on whether a 
> #define for array size expands into 2 or 20.  

To clear up confusion, in:

 void function (int array[4096], int opt)
 {
    ...
 }

The `4096' is mostly meaningless.  The real array size depends
on what the caller passed in at a particular invocation.  It is
valid for this function to be passed an array with less than
4096 elements, provided `function' doesn't access the missing
elements (e.g., depending on OPT).  Collection may fail if
GDB attempts to always collect 4096 (or N whatever).
The passed in array may be also more than 4096 elements wide,
and `function' may legally access the excess elements.
The `4096' is just useful as API documentation.

> Second, "collect $args" 
> will work as expected, and not have to be written as "collect $args, 
> argarray[0]@4" (which would be hard to get right if the 4 came from a 
> macro).

( GDB supports macro expansion nowadays :-) )

> Third, if collection collects a little more than is strictly 
> necessary, then it improves the chances that trace frames will have the 
> right data on the first try, instead of requiring a re-run.

Yeah.  One of my worries is that convenience sometimes shoots
in the foot, as it may instead miscollect, or overcollect.
I guess that in a couple of years, when tracepoints get
more use, people will get used to these issues, and we'll
look back and wonder why was this even an issue --- it's
so obvious you need to specify the length!  or not!!

In any case, the only way GDB would be able to collect the
whole array, is for debug info to describe the `argarray[4]'
argument as an array instead of a pointer.   It doesn't on my
testing; it's always described as a pointer.  There's no way
for GDB to know the "array" size as written in the source
this way.

> My inclination for now is just to mark as known failures, so as not to 
> get too bogged down.

I've applied the below then.  I agree with xfailing,
because I'll be curious to see this xpass somewhere.

-- 
Pedro Alves
2010-04-01  Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
	* gdb.trace/collection.exp (gdb_collect_args_test)
	(gdb_collect_argarray_test): XFAIL the tests that assume the
	argarray argument's elements are collected.

---
 gdb/testsuite/gdb.trace/collection.exp |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Index: src/gdb/testsuite/gdb.trace/collection.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.trace/collection.exp	2010-04-01 01:26:37.000000000 +0100
+++ src/gdb/testsuite/gdb.trace/collection.exp	2010-04-01 01:43:58.000000000 +0100
@@ -166,15 +166,30 @@ proc gdb_collect_args_test { myargs msg 
 	    "collect $msg: collected arg struct member double"
 
     # array arg as one of several args (near end of list)
+
+    # It isn't clear why is the test assuming the array's elements are
+    # collected.  In C, an array as function parameters is a special
+    # case; it's just a pointer into the caller's array, and as such,
+    # that's what normally the debug info describes.  Maybe this was
+    # originaly written for a compiler where array parameters were
+    # really described as arrays in debug info.
+
+    setup_xfail "*-*-*"
     gdb_test "print argarray\[0\]" \
 	    "\\$\[0-9\]+ = 111$cr" \
 	    "collect $msg: collected argarray #0"
+
+    setup_xfail "*-*-*"
     gdb_test "print argarray\[1\]" \
 	    "\\$\[0-9\]+ = 112$cr" \
 	    "collect $msg: collected argarray #1"
+
+    setup_xfail "*-*-*"
     gdb_test "print argarray\[2\]" \
 	    "\\$\[0-9\]+ = 113$cr" \
 	    "collect $msg: collected argarray #2"
+
+    setup_xfail "*-*-*"
     gdb_test "print argarray\[3\]" \
 	    "\\$\[0-9\]+ = 114$cr" \
 	    "collect $msg: collected argarray #3"
@@ -237,15 +252,30 @@ proc gdb_collect_argarray_test { myargs 
     run_trace_experiment $msg argarray_test_func
 
     # array arg as only argument
+
+    # It isn't clear why is the test assuming the array's elements are
+    # collected.  In C, an array as function parameters is a special
+    # case; it's just a pointer into the caller's array, and as such,
+    # that's what normally the debug info describes.  Maybe this was
+    # originaly written for a compiler where array parameters were
+    # really described as arrays in debug info.
+
+    setup_xfail "*-*-*"
     gdb_test "print argarray\[0\]" \
 	    "\\$\[0-9\]+ = 111$cr" \
 	    "collect $msg: collected argarray #0"
+
+    setup_xfail "*-*-*"
     gdb_test "print argarray\[1\]" \
 	    "\\$\[0-9\]+ = 112$cr" \
 	    "collect $msg: collected argarray #1"
+
+    setup_xfail "*-*-*"
     gdb_test "print argarray\[2\]" \
 	    "\\$\[0-9\]+ = 113$cr" \
 	    "collect $msg: collected argarray #2"
+
+    setup_xfail "*-*-*"
     gdb_test "print argarray\[3\]" \
 	    "\\$\[0-9\]+ = 114$cr" \
 	    "collect $msg: collected argarray #3"