[PATCH 0/2] Test case on entry values

[PATCH 0/2] Test case on entry values

[Yao Qi]

Hi,
In the review of the patch which adds '--skip-unavailable' to skip
unavailable locals and arguments, Pedro pointed that "entry value" should be
considered too.  The code is not hard, but the test case is harder than the
code.  We have to make sure that 1) necessary DIEs are generated, 2) set up
a case that argument is available but entry value is not.

We choose the test to arguments and entry values when GDB is examining trace
frames, because something can be unavailable.  This situation is not tested
in current testsuite, and this test can be reviewed committed independently.

Patch 1/2 is to generate dwarf using Dwarf Assembler to test "entry values"
are shown correctly.  At this point, gdb.trace/entry-values.exp is still
a dwarf test, nothing to do with trace.  Patch 2/2 is to use tracepoint,
to collect data, to test what happen when argument is available and entry
value is not.

Most of gdb.trace/etnry-values.exp is a dwarf test, and we can move them
to gdb.dwarf2 and copy necessary bits in gdb.trace.  I didn't do that
because it will cause some duplication.

*** BLURB HERE ***

Yao Qi (2):
  Test case for entry values.
  Test entry values in trace frame

 gdb/testsuite/gdb.trace/entry-values.c   |   50 ++++++
 gdb/testsuite/gdb.trace/entry-values.exp |  274 ++++++++++++++++++++++++++++++
 gdb/testsuite/lib/dwarf.exp              |    8 +
 3 files changed, 332 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/entry-values.c
 create mode 100644 gdb/testsuite/gdb.trace/entry-values.exp

-- 
1.7.7.6

[PATCH 1/2] Test case for entry values.

[Yao Qi]

Hi,
This is a test case for "entry-values" in an arch-independent way.  The
first part of entry-values.exp is to calculate the length of functions
and the offset of instruction call in function bar.  Then, this test
uses Dwarf Assembler to emit some DIEs for "entry-values".

2013-08-13  Yao Qi  <yao@codesourcery.com>

	* lib/dwarf.exp (_location): Handle DW_OP_deref_size.
	* gdb.trace/entry-values.c: New.
	* gdb.trace/entry-values.exp: New.
---
 gdb/testsuite/gdb.trace/entry-values.c   |   45 ++++++
 gdb/testsuite/gdb.trace/entry-values.exp |  223 ++++++++++++++++++++++++++++++
 gdb/testsuite/lib/dwarf.exp              |    8 +
 3 files changed, 276 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/entry-values.c
 create mode 100644 gdb/testsuite/gdb.trace/entry-values.exp

diff --git a/gdb/testsuite/gdb.trace/entry-values.c b/gdb/testsuite/gdb.trace/entry-values.c
new file mode 100644
index 0000000..3f98615
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/entry-values.c
@@ -0,0 +1,45 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+foo (int i, int j)
+{
+  return 0;
+}
+
+int
+bar (int i)
+{
+  int j = 2;
+
+  return foo (i, j);
+}
+
+int global1 = 1;
+int global2 = 2;
+
+int
+main (void)
+{
+  int ret = 0;
+
+  global1++;
+  global2++;
+  ret = bar (0);
+
+  return ret;
+}
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
new file mode 100644
index 0000000..ba95e4f
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -0,0 +1,223 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c entry-values-dw.S
+
+if  {[gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile}1.o \
+	  object {nodebug}] != ""} {
+    return -1
+}
+
+# Start GDB and load object file, compute the function length and
+# the offset of branch instruction in function.  They are needed
+# in the Dwarf Assembler below.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}1.o
+
+set foo_length ""
+
+# Calculate the offset of the last instruction from the beginning.
+set test "disassemble foo"
+gdb_test_multiple $test $test {
+    -re ".*$hex <\\+($decimal)>:\[^\r\n\]+\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+	set foo_length $expect_out(1,string)
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+    }
+}
+
+# Calculate the size of the last instruction.  Single instruction
+# shouldn't be longer than 10 bytes.
+
+set test "disassemble foo+$foo_length,+10"
+gdb_test_multiple $test $test {
+    -re ".*($hex) <foo\\+$foo_length>:\[^\r\n\]+\r\n\[ \]+($hex) .*\.\r\n$gdb_prompt $" {
+	set start $expect_out(1,string)
+	set end $expect_out(2,string)
+
+	set foo_length [expr $foo_length + $end - $start]
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+    }
+}
+
+set bar_length ""
+set bar_call_foo ""
+
+# Calculate the offset of the last instruction from the beginning.
+set test "disassemble bar"
+gdb_test_multiple $test $test {
+    -re ".*$hex <\\+$decimal>:\[ \t\]+call\[^\r\n\]+\r\n\[ \]+$hex <\\+($decimal)>:" {
+	set bar_call_foo $expect_out(1,string)
+	exp_continue
+    }
+    -re ".*$hex <\\+($decimal)>:\[^\r\n\]+\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+	set bar_length $expect_out(1,string)
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+    }
+}
+
+if [string equal $bar_call_foo ""] {
+    fail "Find the call or branch instruction offset in bar"
+    # The following test makes no sense if the offset is unknown.  We need
+    # to update the pattern above to match call or branch instruction for
+    # the target architecture.
+    return -1
+}
+
+# Calculate the size of the last instruction.
+
+set test "disassemble bar+$bar_length,+10"
+gdb_test_multiple $test $test {
+    -re ".*($hex) <bar\\+$bar_length>:\[^\r\n\]+\r\n\[ \]+($hex) .*\.\r\n$gdb_prompt $" {
+	set start $expect_out(1,string)
+	set end $expect_out(2,string)
+
+	set bar_length [expr $bar_length + $end - $start]
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+    }
+}
+
+gdb_exit
+
+# Make some DWARF for the test.
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    declare_labels int_label foo_label
+    global foo_length bar_length bar_call_foo
+
+    cu {addr_size 4} {
+	compile_unit {{language @DW_LANG_C}} {
+	    int_label: base_type {
+		{name int}
+		{encoding @DW_ATE_signed}
+		{byte_size 4 DW_FORM_sdata}
+	    }
+
+	    foo_label: subprogram {
+		{name foo}
+		{decl_file 1}
+		{low_pc foo addr}
+		{high_pc "foo + $foo_length" addr}
+	    } {
+		formal_parameter {
+		    {type :$int_label}
+		    {name i}
+		    {location {DW_OP_reg0} SPECIAL_expr}
+		}
+		formal_parameter {
+		    {type :$int_label}
+		    {name j}
+		    {location {DW_OP_reg1} SPECIAL_expr}
+		}
+	    }
+
+	    subprogram {
+		{name bar}
+		{decl_file 1}
+		{low_pc bar addr}
+		{high_pc "bar + $bar_length" addr}
+		{GNU_all_call_sites 1}
+	    } {
+		formal_parameter {
+		    {type :$int_label}
+		    {name i}
+		}
+
+		GNU_call_site {
+		    {low_pc "bar + $bar_call_foo" addr}
+		    {abstract_origin :$foo_label}
+		} {
+		    # Faked entry values are reference to variables 'global1'
+		    # and 'global2' and faked locations are register 0 and
+		    # register 1.
+		    GNU_call_site_parameter {
+			{location {DW_OP_reg0} SPECIAL_expr}
+			{GNU_call_site_value {
+			    addr global1
+			    deref_size 4
+			} SPECIAL_expr}
+		    }
+		    GNU_call_site_parameter {
+			{location {DW_OP_reg1} SPECIAL_expr}
+			{GNU_call_site_value {
+			    addr global2
+			    deref_size 4
+			} SPECIAL_expr}
+		    }
+		}
+	    }
+	}
+    }
+}
+
+if  {[gdb_compile $asm_file ${binfile}2.o object {nodebug}] != ""} {
+    return -1
+}
+
+if  {[gdb_compile [list ${binfile}1.o ${binfile}2.o] \
+	  "${binfile}" executable {}] != ""} {
+    return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+gdb_breakpoint "foo"
+
+gdb_continue_to_breakpoint "foo"
+
+gdb_test_no_output "set print entry-values both"
+
+gdb_test_sequence "bt" "bt" {
+    "\[\r\n\]#0 .* foo \\(i=[-]?[0-9]+, i@entry=2, j=[-]?[0-9]+, j@entry=3\\)"
+    "\[\r\n\]#1 .* bar \\(i=<optimized out>, i@entry=<optimized out>\\)"
+    "\[\r\n\]#2 .* main \\(\\)"
+}
+
+# Update global variables 'global1' and 'global2' and test that the
+# entry values are updated too.
+
+gdb_test_no_output "set var global1=10"
+gdb_test_no_output "set var global2=11"
+
+gdb_test_sequence "bt" "bt" {
+    "\[\r\n\]#0 .* foo \\(i=[-]?[0-9]+, i@entry=10, j=[-]?[0-9]+, j@entry=11\\)"
+    "\[\r\n\]#1 .* bar \\(i=<optimized out>, i@entry=<optimized out>\\)"
+    "\[\r\n\]#2 .* main \\(\\)"
+}
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 5b19bb8..99f4b06 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -667,6 +667,14 @@ namespace eval Dwarf {
 		    _op .sleb128 [lindex $line 2]
 		}
 
+		DW_OP_deref_size {
+		    if {[llength $line] != 2} {
+			error "usage: DW_OP_deref_size SIZE"
+		    }
+
+		    _op .byte [lindex $line 1]
+		}
+
 		default {
 		    if {[llength $line] > 1} {
 			error "Unimplemented: operands in location for $opcode"
-- 
1.7.7.6

Re: [PATCH 1/2] Test case for entry values.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> This is a test case for "entry-values" in an arch-independent way.  The
Yao> first part of entry-values.exp is to calculate the length of functions
Yao> and the offset of instruction call in function bar.  Then, this test
Yao> uses Dwarf Assembler to emit some DIEs for "entry-values".

Thanks Yao.  This is cool.

Yao> +# Start GDB and load object file, compute the function length and
Yao> +# the offset of branch instruction in function.  They are needed
Yao> +# in the Dwarf Assembler below.

Nice technique!  I'll have to use that.

Yao> +# Calculate the offset of the last instruction from the beginning.
Yao> +set test "disassemble foo"
Yao> +gdb_test_multiple $test $test {
Yao> +    -re ".*$hex <\\+($decimal)>:\[^\r\n\]+\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
Yao> +	set foo_length $expect_out(1,string)
Yao> +	pass $test
Yao> +    }
Yao> +    -re ".*$gdb_prompt $" {
Yao> +	fail $test
Yao> +    }
Yao> +}

If this test fails then later on foo_length won't be set.
This will yield a Tcl error in the test suite.
It's probably better to just bail out here.

I think this applies elsewhere too.

Yao> +    cu {addr_size 4} {

Will it still work on x86-64?

Tom

Re: [PATCH 1/2] Test case for entry values.

[Yao Qi]

On 08/21/2013 01:39 AM, Tom Tromey wrote:
> Yao> +# Calculate the offset of the last instruction from the beginning.
> Yao> +set test "disassemble foo"
> Yao> +gdb_test_multiple $test $test {
> Yao> +    -re ".*$hex <\\+($decimal)>:\[^\r\n\]+\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
> Yao> +	set foo_length $expect_out(1,string)
> Yao> +	pass $test
> Yao> +    }
> Yao> +    -re ".*$gdb_prompt $" {
> Yao> +	fail $test
> Yao> +    }
> Yao> +}
> 
> If this test fails then later on foo_length won't be set.
> This will yield a Tcl error in the test suite.
> It's probably better to just bail out here.
> 
> I think this applies elsewhere too.
> 

Fixed.

> Yao> +    cu {addr_size 4} {
> 
> Will it still work on x86-64?

Yes, it works.

I didn't use "addr_size 4" at the beginning, but get the following
error.

gdb/testsuite/gdb.trace/entry-values-dw.S:19: Error: cannot represent relocation type BFD_RELOC_64
gdb/testsuite/gdb.trace/entry-values-dw.S:20: Error: cannot represent relocation type BFD_RELOC_64

I also tried the following stuff, but it seems I can't pass
$cu_addr_size correctly to cu.

if [is_lp64_target] {
    set cu_addr_size 8
} else {
    set cu_addr_size 4
}

...

cu {addr_size $cu_addr_size} {

Here is the updated patch in which "cu {addr_size 4}" is still
used.

-- 
Yao (齐尧)

2013-08-21  Yao Qi  <yao@codesourcery.com>

	* lib/dwarf.exp (_location): Handle DW_OP_deref_size.
	* gdb.trace/entry-values.c: New.
	* gdb.trace/entry-values.exp: New.
---
 gdb/testsuite/gdb.trace/entry-values.c   |   45 ++++++
 gdb/testsuite/gdb.trace/entry-values.exp |  232 ++++++++++++++++++++++++++++++
 gdb/testsuite/lib/dwarf.exp              |    8 +
 3 files changed, 285 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/entry-values.c
 create mode 100644 gdb/testsuite/gdb.trace/entry-values.exp

diff --git a/gdb/testsuite/gdb.trace/entry-values.c b/gdb/testsuite/gdb.trace/entry-values.c
new file mode 100644
index 0000000..3f98615
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/entry-values.c
@@ -0,0 +1,45 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+foo (int i, int j)
+{
+  return 0;
+}
+
+int
+bar (int i)
+{
+  int j = 2;
+
+  return foo (i, j);
+}
+
+int global1 = 1;
+int global2 = 2;
+
+int
+main (void)
+{
+  int ret = 0;
+
+  global1++;
+  global2++;
+  ret = bar (0);
+
+  return ret;
+}
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
new file mode 100644
index 0000000..30db4a6
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -0,0 +1,232 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c entry-values-dw.S
+
+if  {[gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile}1.o \
+	  object {nodebug}] != ""} {
+    return -1
+}
+
+# Start GDB and load object file, compute the function length and
+# the offset of branch instruction in function.  They are needed
+# in the Dwarf Assembler below.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}1.o
+
+set foo_length ""
+
+# Calculate the offset of the last instruction from the beginning.
+set test "disassemble foo"
+gdb_test_multiple $test $test {
+    -re ".*$hex <\\+($decimal)>:\[^\r\n\]+\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+	set foo_length $expect_out(1,string)
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+	# Bail out here, because we can't do the following tests if
+	# $foo_length is unknown.
+	return -1
+    }
+}
+
+# Calculate the size of the last instruction.  Single instruction
+# shouldn't be longer than 10 bytes.
+
+set test "disassemble foo+$foo_length,+10"
+gdb_test_multiple $test $test {
+    -re ".*($hex) <foo\\+$foo_length>:\[^\r\n\]+\r\n\[ \]+($hex) .*\.\r\n$gdb_prompt $" {
+	set start $expect_out(1,string)
+	set end $expect_out(2,string)
+
+	set foo_length [expr $foo_length + $end - $start]
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+	# Bail out here, because we can't do the following tests if
+	# $foo_length is unknown.
+	return -1
+    }
+}
+
+set bar_length ""
+set bar_call_foo ""
+
+# Calculate the offset of the last instruction from the beginning.
+set test "disassemble bar"
+gdb_test_multiple $test $test {
+    -re ".*$hex <\\+$decimal>:\[ \t\]+call\[^\r\n\]+\r\n\[ \]+$hex <\\+($decimal)>:" {
+	set bar_call_foo $expect_out(1,string)
+	exp_continue
+    }
+    -re ".*$hex <\\+($decimal)>:\[^\r\n\]+\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+	set bar_length $expect_out(1,string)
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+    }
+}
+
+if { [string equal $bar_call_foo ""] || [string equal $bar_length ""] } {
+    fail "Find the call or branch instruction offset in bar"
+    # The following test makes no sense if the offset is unknown.  We need
+    # to update the pattern above to match call or branch instruction for
+    # the target architecture.
+    return -1
+}
+
+# Calculate the size of the last instruction.
+
+set test "disassemble bar+$bar_length,+10"
+gdb_test_multiple $test $test {
+    -re ".*($hex) <bar\\+$bar_length>:\[^\r\n\]+\r\n\[ \]+($hex) .*\.\r\n$gdb_prompt $" {
+	set start $expect_out(1,string)
+	set end $expect_out(2,string)
+
+	set bar_length [expr $bar_length + $end - $start]
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+	# Bail out here, because we can't do the following tests if
+	# $bar_length is unknown.
+	return -1
+    }
+}
+
+gdb_exit
+
+# Make some DWARF for the test.
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    declare_labels int_label foo_label
+    global foo_length bar_length bar_call_foo
+
+    cu {addr_size 4} {
+	compile_unit {{language @DW_LANG_C}} {
+	    int_label: base_type {
+		{name int}
+		{encoding @DW_ATE_signed}
+		{byte_size 4 DW_FORM_sdata}
+	    }
+
+	    foo_label: subprogram {
+		{name foo}
+		{decl_file 1}
+		{low_pc foo addr}
+		{high_pc "foo + $foo_length" addr}
+	    } {
+		formal_parameter {
+		    {type :$int_label}
+		    {name i}
+		    {location {DW_OP_reg0} SPECIAL_expr}
+		}
+		formal_parameter {
+		    {type :$int_label}
+		    {name j}
+		    {location {DW_OP_reg1} SPECIAL_expr}
+		}
+	    }
+
+	    subprogram {
+		{name bar}
+		{decl_file 1}
+		{low_pc bar addr}
+		{high_pc "bar + $bar_length" addr}
+		{GNU_all_call_sites 1}
+	    } {
+		formal_parameter {
+		    {type :$int_label}
+		    {name i}
+		}
+
+		GNU_call_site {
+		    {low_pc "bar + $bar_call_foo" addr}
+		    {abstract_origin :$foo_label}
+		} {
+		    # Faked entry values are reference to variables 'global1'
+		    # and 'global2' and faked locations are register 0 and
+		    # register 1.
+		    GNU_call_site_parameter {
+			{location {DW_OP_reg0} SPECIAL_expr}
+			{GNU_call_site_value {
+			    addr global1
+			    deref_size 4
+			} SPECIAL_expr}
+		    }
+		    GNU_call_site_parameter {
+			{location {DW_OP_reg1} SPECIAL_expr}
+			{GNU_call_site_value {
+			    addr global2
+			    deref_size 4
+			} SPECIAL_expr}
+		    }
+		}
+	    }
+	}
+    }
+}
+
+if  {[gdb_compile $asm_file ${binfile}2.o object {nodebug}] != ""} {
+    return -1
+}
+
+if  {[gdb_compile [list ${binfile}1.o ${binfile}2.o] \
+	  "${binfile}" executable {}] != ""} {
+    return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+gdb_breakpoint "foo"
+
+gdb_continue_to_breakpoint "foo"
+
+gdb_test_no_output "set print entry-values both"
+
+gdb_test_sequence "bt" "bt (1)" {
+    "\[\r\n\]#0 .* foo \\(i=[-]?[0-9]+, i@entry=2, j=[-]?[0-9]+, j@entry=3\\)"
+    "\[\r\n\]#1 .* bar \\(i=<optimized out>, i@entry=<optimized out>\\)"
+    "\[\r\n\]#2 .* main \\(\\)"
+}
+
+# Update global variables 'global1' and 'global2' and test that the
+# entry values are updated too.
+
+gdb_test_no_output "set var global1=10"
+gdb_test_no_output "set var global2=11"
+
+gdb_test_sequence "bt" "bt (2)" {
+    "\[\r\n\]#0 .* foo \\(i=[-]?[0-9]+, i@entry=10, j=[-]?[0-9]+, j@entry=11\\)"
+    "\[\r\n\]#1 .* bar \\(i=<optimized out>, i@entry=<optimized out>\\)"
+    "\[\r\n\]#2 .* main \\(\\)"
+}
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 5b19bb8..99f4b06 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -667,6 +667,14 @@ namespace eval Dwarf {
 		    _op .sleb128 [lindex $line 2]
 		}
 
+		DW_OP_deref_size {
+		    if {[llength $line] != 2} {
+			error "usage: DW_OP_deref_size SIZE"
+		    }
+
+		    _op .byte [lindex $line 1]
+		}
+
 		default {
 		    if {[llength $line] > 1} {
 			error "Unimplemented: operands in location for $opcode"
-- 
1.7.7.6

Re: [PATCH 1/2] Test case for entry values.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Tom> Will it still work on x86-64?

Yao> Yes, it works.

Yao> I didn't use "addr_size 4" at the beginning, but get the following
Yao> error.

Yao> gdb/testsuite/gdb.trace/entry-values-dw.S:19: Error: cannot represent relocation type BFD_RELOC_64
Yao> gdb/testsuite/gdb.trace/entry-values-dw.S:20: Error: cannot represent relocation type BFD_RELOC_64

Jan pointed out that the new "dwz.exp" has this failure.

It seems to me that using 32 bit addresses in the DWARF can yield the
wrong results for the test.  What if the high bits matter?

Yao> I also tried the following stuff, but it seems I can't pass
Yao> $cu_addr_size correctly to cu.
Yao> if [is_lp64_target] {
Yao>     set cu_addr_size 8
Yao> } else {
Yao>     set cu_addr_size 4
Yao> }
Yao> ...
Yao> cu {addr_size $cu_addr_size} {

You need to use [list ...] instead.

However, what do you think of the appended?
It adds a "default" setting for addr_size.  This seems to be what we
usually want.

Tom

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 5b19bb8..1d3eb03 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -684,8 +684,8 @@ namespace eval Dwarf {
     #                default = 0 (32-bit)
     # version n    - DWARF version number to emit
     #                default = 4
-    # addr_size n  - the size of addresses, 32 or 64
-    #                default = 64
+    # addr_size n  - the size of addresses, 32, 64, or default
+    #                default = default
     # fission 0|1  - boolean indicating if generating Fission debug info
     #                default = 0
     # BODY is Tcl code that emits the DIEs which make up the body of
@@ -702,7 +702,7 @@ namespace eval Dwarf {
 	# Establish the defaults.
 	set is_64 0
 	set _cu_version 4
-	set _cu_addr_size 8
+	set _cu_addr_size default
 	set fission 0
 	set section ".debug_info"
 	set _abbrev_section ".debug_abbrev"
@@ -716,6 +716,13 @@ namespace eval Dwarf {
 		default { error "unknown option $name" }
 	    }
 	}
+	if {$_cu_addr_size == "default"} {
+	    if {[is_64_target]} {
+		set _cu_addr_size 8
+	    } else {
+		set _cu_addr_size 4
+	    }
+	}
 	set _cu_offset_size [expr { $is_64 ? 8 : 4 }]
 	if { $fission } {
 	    set section ".debug_info.dwo"
@@ -767,8 +774,8 @@ namespace eval Dwarf {
     #                default = 0 (32-bit)
     # version n    - DWARF version number to emit
     #                default = 4
-    # addr_size n  - the size of addresses, 32 or 64
-    #                default = 64
+    # addr_size n  - the size of addresses, 32, 64, or default
+    #                default = default
     # fission 0|1  - boolean indicating if generating Fission debug info
     #                default = 0
     # SIGNATURE is the 64-bit signature of the type.
@@ -788,7 +795,7 @@ namespace eval Dwarf {
 	# Establish the defaults.
 	set is_64 0
 	set _cu_version 4
-	set _cu_addr_size 8
+	set _cu_addr_size default
 	set fission 0
 	set section ".debug_types"
 	set _abbrev_section ".debug_abbrev"
@@ -802,6 +809,13 @@ namespace eval Dwarf {
 		default { error "unknown option $name" }
 	    }
 	}
+	if {$_cu_addr_size == "default"} {
+	    if {[is_64_target]} {
+		set _cu_addr_size 8
+	    } else {
+		set _cu_addr_size 4
+	    }
+	}
 	set _cu_offset_size [expr { $is_64 ? 8 : 4 }]
 	if { $fission } {
 	    set section ".debug_types.dwo"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 1a1fac2..ab41da0 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1854,6 +1854,34 @@ gdb_caching_proc is_lp64_target {
     return 1
 }
 
+# Return 1 if target has 64 bit addresses.
+# This cannot be decided simply from looking at the target string,
+# as it might depend on externally passed compiler options like -m64.
+gdb_caching_proc is_64_target {
+    set me "is_64_target"
+
+    set src [standard_temp_file is64[pid].c]
+    set obj [standard_temp_file is64[pid].o]
+
+    set f [open $src "w"]
+    puts $f "int function(void) { return 3; }"
+    puts $f "int dummy\[sizeof (&function) == 8 ? 1 : -1\];"
+    close $f
+
+    verbose "$me:  compiling testfile $src" 2
+    set lines [gdb_compile $src $obj object {quiet}]
+    file delete $src
+    file delete $obj
+
+    if ![string match "" $lines] then {
+        verbose "$me:  testfile compilation failed, returning 0" 2
+        return 0
+    }
+
+    verbose "$me:  returning 1" 2
+    return 1
+}
+
 # Return 1 if target has x86_64 registers - either amd64 or x32.
 # x32 target identifies as x86_64-*-linux*, therefore it cannot be determined
 # just from the target string.

Re: [PATCH 1/2] Test case for entry values.

[Yao Qi]

On 08/21/2013 11:02 PM, Tom Tromey wrote:
> However, what do you think of the appended?
> It adds a "default" setting for addr_size.  This seems to be what we
> usually want.

Tom,
It looks right to me.  Please commit, then I can update my patch on top
of it.

-- 
Yao (齐尧)

Re: [PATCH 1/2] Test case for entry values.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> On 08/21/2013 11:02 PM, Tom Tromey wrote:
>> However, what do you think of the appended?
>> It adds a "default" setting for addr_size.  This seems to be what we
>> usually want.

Yao> It looks right to me.  Please commit, then I can update my patch on top
Yao> of it.

It's in now.

Tom

Re: [PATCH 1/2] Test case for entry values.

[Yao Qi]

On 08/22/2013 10:04 PM, Tom Tromey wrote:
>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
> 
> Yao> On 08/21/2013 11:02 PM, Tom Tromey wrote:
>>> However, what do you think of the appended?
>>> It adds a "default" setting for addr_size.  This seems to be what we
>>> usually want.
> 
> Yao> It looks right to me.  Please commit, then I can update my patch on top
> Yao> of it.
> 
> It's in now.
> 

Here is the updated patch, without setting "addr_size" in cu.
Tested on x86 and x86_64 respectively.

-- 
Yao (齐尧)

2013-08-23  Yao Qi  <yao@codesourcery.com>

	* lib/dwarf.exp (_location): Handle DW_OP_deref_size.
	* gdb.trace/entry-values.c: New.
	* gdb.trace/entry-values.exp: New.
---
 gdb/testsuite/gdb.trace/entry-values.c   |   45 ++++++
 gdb/testsuite/gdb.trace/entry-values.exp |  232 ++++++++++++++++++++++++++++++
 gdb/testsuite/lib/dwarf.exp              |    8 +
 3 files changed, 285 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/entry-values.c
 create mode 100644 gdb/testsuite/gdb.trace/entry-values.exp

diff --git a/gdb/testsuite/gdb.trace/entry-values.c b/gdb/testsuite/gdb.trace/entry-values.c
new file mode 100644
index 0000000..3f98615
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/entry-values.c
@@ -0,0 +1,45 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+foo (int i, int j)
+{
+  return 0;
+}
+
+int
+bar (int i)
+{
+  int j = 2;
+
+  return foo (i, j);
+}
+
+int global1 = 1;
+int global2 = 2;
+
+int
+main (void)
+{
+  int ret = 0;
+
+  global1++;
+  global2++;
+  ret = bar (0);
+
+  return ret;
+}
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
new file mode 100644
index 0000000..0fb060a
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -0,0 +1,232 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c entry-values-dw.S
+
+if  {[gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile}1.o \
+	  object {nodebug}] != ""} {
+    return -1
+}
+
+# Start GDB and load object file, compute the function length and
+# the offset of branch instruction in function.  They are needed
+# in the Dwarf Assembler below.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}1.o
+
+set foo_length ""
+
+# Calculate the offset of the last instruction from the beginning.
+set test "disassemble foo"
+gdb_test_multiple $test $test {
+    -re ".*$hex <\\+($decimal)>:\[^\r\n\]+\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+	set foo_length $expect_out(1,string)
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+	# Bail out here, because we can't do the following tests if
+	# $foo_length is unknown.
+	return -1
+    }
+}
+
+# Calculate the size of the last instruction.  Single instruction
+# shouldn't be longer than 10 bytes.
+
+set test "disassemble foo+$foo_length,+10"
+gdb_test_multiple $test $test {
+    -re ".*($hex) <foo\\+$foo_length>:\[^\r\n\]+\r\n\[ \]+($hex) .*\.\r\n$gdb_prompt $" {
+	set start $expect_out(1,string)
+	set end $expect_out(2,string)
+
+	set foo_length [expr $foo_length + $end - $start]
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+	# Bail out here, because we can't do the following tests if
+	# $foo_length is unknown.
+	return -1
+    }
+}
+
+set bar_length ""
+set bar_call_foo ""
+
+# Calculate the offset of the last instruction from the beginning.
+set test "disassemble bar"
+gdb_test_multiple $test $test {
+    -re ".*$hex <\\+$decimal>:\[ \t\]+call\[^\r\n\]+\r\n\[ \]+$hex <\\+($decimal)>:" {
+	set bar_call_foo $expect_out(1,string)
+	exp_continue
+    }
+    -re ".*$hex <\\+($decimal)>:\[^\r\n\]+\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+	set bar_length $expect_out(1,string)
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+    }
+}
+
+if { [string equal $bar_call_foo ""] || [string equal $bar_length ""] } {
+    fail "Find the call or branch instruction offset in bar"
+    # The following test makes no sense if the offset is unknown.  We need
+    # to update the pattern above to match call or branch instruction for
+    # the target architecture.
+    return -1
+}
+
+# Calculate the size of the last instruction.
+
+set test "disassemble bar+$bar_length,+10"
+gdb_test_multiple $test $test {
+    -re ".*($hex) <bar\\+$bar_length>:\[^\r\n\]+\r\n\[ \]+($hex) .*\.\r\n$gdb_prompt $" {
+	set start $expect_out(1,string)
+	set end $expect_out(2,string)
+
+	set bar_length [expr $bar_length + $end - $start]
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	fail $test
+	# Bail out here, because we can't do the following tests if
+	# $bar_length is unknown.
+	return -1
+    }
+}
+
+gdb_exit
+
+# Make some DWARF for the test.
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    declare_labels int_label foo_label
+    global foo_length bar_length bar_call_foo
+
+    cu {} {
+	compile_unit {{language @DW_LANG_C}} {
+	    int_label: base_type {
+		{name int}
+		{encoding @DW_ATE_signed}
+		{byte_size 4 DW_FORM_sdata}
+	    }
+
+	    foo_label: subprogram {
+		{name foo}
+		{decl_file 1}
+		{low_pc foo addr}
+		{high_pc "foo + $foo_length" addr}
+	    } {
+		formal_parameter {
+		    {type :$int_label}
+		    {name i}
+		    {location {DW_OP_reg0} SPECIAL_expr}
+		}
+		formal_parameter {
+		    {type :$int_label}
+		    {name j}
+		    {location {DW_OP_reg1} SPECIAL_expr}
+		}
+	    }
+
+	    subprogram {
+		{name bar}
+		{decl_file 1}
+		{low_pc bar addr}
+		{high_pc "bar + $bar_length" addr}
+		{GNU_all_call_sites 1}
+	    } {
+		formal_parameter {
+		    {type :$int_label}
+		    {name i}
+		}
+
+		GNU_call_site {
+		    {low_pc "bar + $bar_call_foo" addr}
+		    {abstract_origin :$foo_label}
+		} {
+		    # Faked entry values are reference to variables 'global1'
+		    # and 'global2' and faked locations are register 0 and
+		    # register 1.
+		    GNU_call_site_parameter {
+			{location {DW_OP_reg0} SPECIAL_expr}
+			{GNU_call_site_value {
+			    addr global1
+			    deref_size 4
+			} SPECIAL_expr}
+		    }
+		    GNU_call_site_parameter {
+			{location {DW_OP_reg1} SPECIAL_expr}
+			{GNU_call_site_value {
+			    addr global2
+			    deref_size 4
+			} SPECIAL_expr}
+		    }
+		}
+	    }
+	}
+    }
+}
+
+if  {[gdb_compile $asm_file ${binfile}2.o object {nodebug}] != ""} {
+    return -1
+}
+
+if  {[gdb_compile [list ${binfile}1.o ${binfile}2.o] \
+	  "${binfile}" executable {}] != ""} {
+    return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+gdb_breakpoint "foo"
+
+gdb_continue_to_breakpoint "foo"
+
+gdb_test_no_output "set print entry-values both"
+
+gdb_test_sequence "bt" "bt (1)" {
+    "\[\r\n\]#0 .* foo \\(i=[-]?[0-9]+, i@entry=2, j=[-]?[0-9]+, j@entry=3\\)"
+    "\[\r\n\]#1 .* bar \\(i=<optimized out>, i@entry=<optimized out>\\)"
+    "\[\r\n\]#2 .* main \\(\\)"
+}
+
+# Update global variables 'global1' and 'global2' and test that the
+# entry values are updated too.
+
+gdb_test_no_output "set var global1=10"
+gdb_test_no_output "set var global2=11"
+
+gdb_test_sequence "bt" "bt (2)" {
+    "\[\r\n\]#0 .* foo \\(i=[-]?[0-9]+, i@entry=10, j=[-]?[0-9]+, j@entry=11\\)"
+    "\[\r\n\]#1 .* bar \\(i=<optimized out>, i@entry=<optimized out>\\)"
+    "\[\r\n\]#2 .* main \\(\\)"
+}
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 1d3eb03..3977384 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -667,6 +667,14 @@ namespace eval Dwarf {
 		    _op .sleb128 [lindex $line 2]
 		}
 
+		DW_OP_deref_size {
+		    if {[llength $line] != 2} {
+			error "usage: DW_OP_deref_size SIZE"
+		    }
+
+		    _op .byte [lindex $line 1]
+		}
+
 		default {
 		    if {[llength $line] > 1} {
 			error "Unimplemented: operands in location for $opcode"
-- 
1.7.7.6

Re: [PATCH 1/2] Test case for entry values.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> Here is the updated patch, without setting "addr_size" in cu.
Yao> Tested on x86 and x86_64 respectively.

Yao> 2013-08-23  Yao Qi  <yao@codesourcery.com>

Yao> 	* lib/dwarf.exp (_location): Handle DW_OP_deref_size.
Yao> 	* gdb.trace/entry-values.c: New.
Yao> 	* gdb.trace/entry-values.exp: New.

Ok.  Thanks!

Tom

Re: [PATCH 1/2] Test case for entry values.

[Yao Qi]

On 08/24/2013 03:23 AM, Tom Tromey wrote:
> Yao> 2013-08-23  Yao Qi<yao@codesourcery.com>
>
> Yao> 	* lib/dwarf.exp (_location): Handle DW_OP_deref_size.
> Yao> 	* gdb.trace/entry-values.c: New.
> Yao> 	* gdb.trace/entry-values.exp: New.
>
> Ok.  Thanks!

Patch is committed.

-- 
Yao (齐尧)

Re: [PATCH 1/2] Test case for entry values.

[Vidya Praveen]

Hi Yao Qi,

On 08/13/13 08:39, Yao Qi wrote:
[...]
> +set bar_length ""
> +set bar_call_foo ""
> +
> +# Calculate the offset of the last instruction from the beginning.
> +set test "disassemble bar"
> +gdb_test_multiple $test $test {
> +    -re ".*$hex <\\+$decimal>:\[ \t\]+call\[^\r\n\]+\r\n\[ \]+$hex <\\+($decimal)>:" {

If I understand it right, this expects a 'call' instruction. Isn't this target
specific?

Regards
VP

Re: [PATCH 1/2] Test case for entry values.

[Vidya Praveen]

On Fri, Aug 30, 2013 at 03:52:38PM +0100, Vidya Praveen wrote:
> Hi Yao Qi,
> 
> On 08/13/13 08:39, Yao Qi wrote:
> [...]
> > +set bar_length ""
> > +set bar_call_foo ""
> > +
> > +# Calculate the offset of the last instruction from the beginning.
> > +set test "disassemble bar"
> > +gdb_test_multiple $test $test {
> > +    -re ".*$hex <\\+$decimal>:\[ \t\]+call\[^\r\n\]+\r\n\[ \]+$hex <\\+($decimal)>:" {
> 
> If I understand it right, this expects a 'call' instruction. Isn't this target
> specific?

Sorry I missed this comment:

+if [string equal $bar_call_foo ""] {
+    fail "Find the call or branch instruction offset in bar"
+    # The following test makes no sense if the offset is unknown.  We need
+    # to update the pattern above to match call or branch instruction for
+    # the target architecture.
+    return -1
+}

This test fails for ARM targets as they generate 'bl'.

Regards                                                                                                                     
VP

        

Re: [PATCH 1/2] Test case for entry values.

[Yao Qi]

On 08/30/2013 11:29 PM, Vidya Praveen wrote:
> +if [string equal $bar_call_foo ""] {
> +    fail "Find the call or branch instruction offset in bar"
> +    # The following test makes no sense if the offset is unknown.  We need
> +    # to update the pattern above to match call or branch instruction for
> +    # the target architecture.
> +    return -1
> +}
>
> This test fails for ARM targets as they generate 'bl'.

As the comment said, the pattern can be updated to match instruction
'bl'.  I don't know branch instructions of all architectures, but
people familiar with one arch probably can add its branch instruction
into the pattern without much effort.

-- 
Yao (齐尧)

Re: [PATCH 1/2] Test case for entry values.

[Vidya Praveen]

On Sat, Aug 31, 2013 at 01:21:38AM +0100, Yao Qi wrote:
> On 08/30/2013 11:29 PM, Vidya Praveen wrote:
> > +if [string equal $bar_call_foo ""] {
> > +    fail "Find the call or branch instruction offset in bar"
> > +    # The following test makes no sense if the offset is unknown.  We need
> > +    # to update the pattern above to match call or branch instruction for
> > +    # the target architecture.
> > +    return -1
> > +}
> >
> > This test fails for ARM targets as they generate 'bl'.
> 
> As the comment said, the pattern can be updated to match instruction
> 'bl'.  I don't know branch instructions of all architectures, but
> people familiar with one arch probably can add its branch instruction
> into the pattern without much effort.

OK. But isn't it better to have the condition (!gdb_target_supports_trace)
that checks if the target supports tracing, in the beginning of the test
rather than much later?

I can modify to use an appropriate regular expression based on the 
architecture. But I am trying to see the point when the test eventually
ends as UNSUPPORTED.

Regards
VP


> 
> -- 
> Yao (??????)
> 

Re: [PATCH 1/2] Test case for entry values.

[Yao Qi]

On 09/10/2013 11:30 PM, Vidya Praveen wrote:
> OK. But isn't it better to have the condition (!gdb_target_supports_trace)
> that checks if the target supports tracing, in the beginning of the test
> rather than much later?

This part of test is about testing entry values, and the bottom part 
(added by patch 2/2) is about testing unavailable entry values when 
examining trace frames.  This part is not related to tracing, so we 
can't use gdb_target_supports_trace to check.

See my description in "PATCH 0/2"

> Patch 1/2 is to generate dwarf using Dwarf Assembler to test "entry values"
> are shown correctly.  At this point, gdb.trace/entry-values.exp is still
> a dwarf test, nothing to do with trace.  Patch 2/2 is to use tracepoint,
> to collect data, to test what happen when argument is available and entry
> value is not.

https://sourceware.org/ml/gdb-patches/2013-08/msg00327.html

-- 
Yao (齐尧)

Re: [PATCH 1/2] Test case for entry values.

[Vidya Praveen]

On Wed, Sep 11, 2013 at 12:43:24AM +0100, Yao Qi wrote:
> On 09/10/2013 11:30 PM, Vidya Praveen wrote:
> > OK. But isn't it better to have the condition (!gdb_target_supports_trace)
> > that checks if the target supports tracing, in the beginning of the test
> > rather than much later?
> 
> This part of test is about testing entry values, and the bottom part 
> (added by patch 2/2) is about testing unavailable entry values when 
> examining trace frames.  This part is not related to tracing, so we 
> can't use gdb_target_supports_trace to check.
> 
> See my description in "PATCH 0/2"
> 
> > Patch 1/2 is to generate dwarf using Dwarf Assembler to test "entry values"
> > are shown correctly.  At this point, gdb.trace/entry-values.exp is still
> > a dwarf test, nothing to do with trace.  Patch 2/2 is to use tracepoint,
> > to collect data, to test what happen when argument is available and entry
> > value is not.
> 
> https://sourceware.org/ml/gdb-patches/2013-08/msg00327.html

However, it scans for the 'call' instruction regardless of the target. Though
your comment explains it, the test is still target specific. It should check
for the target before scanning for 'call' and not assume the $sp for all 
targets except for those you already check.

Regards
VP

[RFC 2/2] Test entry values in trace frame

[Yao Qi]

Hi,
This is the test for "entry-values" when GDB is examining trace
frames.  In test, tracepoint action collects arguments i and j and
global variable global1.  Variable global2 is not collected.

The GNU_call_site_parameter is faked that i's entry value is from
global1, j's entry value is from global2.  When GDB is examining trace
frame, value of i, j and i@entry should be available, but value of
j@entry is not.

However, when I run this test, I find j@entry is something like,
j@entry=<error reading variable: Cannot access memory at address 0x8049788>
instead of unavailable.

I don't emit a fail for it because I am not very sure it is expected
to be "unavailable".  I am fine to kfail it.

I looked into a little, and looks reading entry value doesn't use
value availability-aware API.  It is not an easy fix to me.

gdb/testsuite:

2013-08-13  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/entry-values.c (end): New
	(main): Call end.
	* gdb.trace/entry-values.exp: Load trace-support.exp.  Set
	tracepoint and collect data.  Test entry value is unavailable.
---
 gdb/testsuite/gdb.trace/entry-values.c   |    5 +++
 gdb/testsuite/gdb.trace/entry-values.exp |   49 ++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/entry-values.c b/gdb/testsuite/gdb.trace/entry-values.c
index 3f98615..e287203 100644
--- a/gdb/testsuite/gdb.trace/entry-values.c
+++ b/gdb/testsuite/gdb.trace/entry-values.c
@@ -32,6 +32,10 @@ bar (int i)
 int global1 = 1;
 int global2 = 2;
 
+static void
+end (void)
+{}
+
 int
 main (void)
 {
@@ -41,5 +45,6 @@ main (void)
   global2++;
   ret = bar (0);
 
+  end ();
   return ret;
 }
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
index ba95e4f..bf946c3 100644
--- a/gdb/testsuite/gdb.trace/entry-values.exp
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -221,3 +221,52 @@ gdb_test_sequence "bt" "bt" {
     "\[\r\n\]#1 .* bar \\(i=<optimized out>, i@entry=<optimized out>\\)"
     "\[\r\n\]#2 .* main \\(\\)"
 }
+
+# Restart GDB and trace.
+
+clean_restart $binfile
+
+load_lib "trace-support.exp"
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+gdb_test "trace foo" "Tracepoint $decimal at .*"
+
+if [is_amd64_regs_target] {
+    set spreg "\$rsp"
+} elseif [is_x86_like_target] {
+    set spreg "\$esp"
+} else {
+    set spreg "\$sp"
+}
+
+# Collect arguments i and j.  Collect 'global1' which is entry value
+# of argument i.  Don't collect 'global2' to test the entry value of
+# argument j.
+
+gdb_trace_setactions "set action for tracepoint 1" "" \
+    "collect i, j, global1, \(\*\(void \*\*\) \($spreg\)\) @ 64" "^$"
+
+gdb_test_no_output "tstart"
+
+gdb_breakpoint "end"
+gdb_continue_to_breakpoint "end"
+
+gdb_test_no_output "tstop"
+
+gdb_test "tfind" "Found trace frame 0, .*" "tfind start"
+
+# Since 'global2' is not collected, j@entry is expected to be 'unavailable',
+# however, the current output is like:
+# j@entry=<error reading variable: Cannot access memory at address 0x8049788>
+gdb_test "bt 1" "#0 .* foo \\(i=\[-\]?$decimal, i@entry=2, j=\[-\]?$decimal, j@entry=.*\\).*"
+
+gdb_test "tfind" "Target failed to find requested trace frame\..*"
-- 
1.7.7.6

Re: [RFC 2/2] Test entry values in trace frame

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> I don't emit a fail for it because I am not very sure it is expected
Yao> to be "unavailable".  I am fine to kfail it.

Yao> I looked into a little, and looks reading entry value doesn't use
Yao> value availability-aware API.  It is not an easy fix to me.

I think on the whole I'd rather we not check in a test that fails.

I know we already have tests like that, but what I've noticed is that
these tests only ever seem to be fixed as a side effect of fixing
something else.  Otherwise the failures are just universally ignored.

I suppose the best thing to do is to file a bug about this problem.
Then you can attach this patch to the bug.

What do you think?

Tom

Re: [RFC 2/2] Test entry values in trace frame

[Yao Qi]

On 08/21/2013 01:48 AM, Tom Tromey wrote:
> I think on the whole I'd rather we not check in a test that fails.
> 
> I know we already have tests like that, but what I've noticed is that
> these tests only ever seem to be fixed as a side effect of fixing
> something else.  Otherwise the failures are just universally ignored.
> 
> I suppose the best thing to do is to file a bug about this problem.
> Then you can attach this patch to the bug.
> 
> What do you think?

I agree.  I opened PR15871 http://sourceware.org/bugzilla/show_bug.cgi?id=15871

On the other hand, I kfail the test.

-- 
Yao (齐尧)

gdb/testsuite:

2013-08-21  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/entry-values.c (end): New
	(main): Call end.
	* gdb.trace/entry-values.exp: Load trace-support.exp.  Set
	tracepoint and collect data.  Test entry value is unavailable.
---
 gdb/testsuite/gdb.trace/entry-values.c   |    5 +++
 gdb/testsuite/gdb.trace/entry-values.exp |   48 ++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/entry-values.c b/gdb/testsuite/gdb.trace/entry-values.c
index 3f98615..e287203 100644
--- a/gdb/testsuite/gdb.trace/entry-values.c
+++ b/gdb/testsuite/gdb.trace/entry-values.c
@@ -32,6 +32,10 @@ bar (int i)
 int global1 = 1;
 int global2 = 2;
 
+static void
+end (void)
+{}
+
 int
 main (void)
 {
@@ -41,5 +45,6 @@ main (void)
   global2++;
   ret = bar (0);
 
+  end ();
   return ret;
 }
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
index 30db4a6..cf2cb6f 100644
--- a/gdb/testsuite/gdb.trace/entry-values.exp
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -230,3 +230,51 @@ gdb_test_sequence "bt" "bt (2)" {
     "\[\r\n\]#1 .* bar \\(i=<optimized out>, i@entry=<optimized out>\\)"
     "\[\r\n\]#2 .* main \\(\\)"
 }
+
+# Restart GDB and trace.
+
+clean_restart $binfile
+
+load_lib "trace-support.exp"
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+gdb_test "trace foo" "Tracepoint $decimal at .*"
+
+if [is_amd64_regs_target] {
+    set spreg "\$rsp"
+} elseif [is_x86_like_target] {
+    set spreg "\$esp"
+} else {
+    set spreg "\$sp"
+}
+
+# Collect arguments i and j.  Collect 'global1' which is entry value
+# of argument i.  Don't collect 'global2' to test the entry value of
+# argument j.
+
+gdb_trace_setactions "set action for tracepoint 1" "" \
+    "collect i, j, global1, \(\*\(void \*\*\) \($spreg\)\) @ 64" "^$"
+
+gdb_test_no_output "tstart"
+
+gdb_breakpoint "end"
+gdb_continue_to_breakpoint "end"
+
+gdb_test_no_output "tstop"
+
+gdb_test "tfind" "Found trace frame 0, .*" "tfind start"
+
+# Since 'global2' is not collected, j@entry is expected to be 'unavailable'.
+setup_kfail "gdb/15871" *-*-*
+gdb_test "bt 1" "#0 .* foo \\(i=\[-\]?$decimal, i@entry=2, j=\[-\]?$decimal, j@entry=<unavailable>\\).*"
+
+gdb_test "tfind" "Target failed to find requested trace frame\..*"
-- 
1.7.7.6

[PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

[Pedro Alves]

On 08/13/2013 08:39 AM, Yao Qi wrote:
> However, when I run this test, I find j@entry is something like,
> j@entry=<error reading variable: Cannot access memory at address 0x8049788>
> instead of unavailable.
> 
> I don't emit a fail for it because I am not very sure it is expected
> to be "unavailable".  I am fine to kfail it.
> 
> I looked into a little, and looks reading entry value doesn't use
> value availability-aware API.  It is not an easy fix to me.

I looked into this.  Here's a patch.  Let me know what you think.

--------
Subject: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

In entry-values.exp, we have a test where the entry value of 'j' is
unavailable, so it is expected that printing j@entry yields
"<unavailable>".  However, the actual output is:

 (gdb) frame
 #0  0x0000000000400540 in foo (i=0, i@entry=2, j=2, j@entry=<error reading variable: Cannot access memory at address 0x6009e8>)

The error is thrown here:

#0  throw_it (reason=RETURN_ERROR, error=MEMORY_ERROR, fmt=0x8cd550 "Cannot access memory at address %s", ap=0x7fffffffc8e8) at ../../src/gdb/exceptions.c:373
#1  0x00000000005e2f9c in throw_error (error=MEMORY_ERROR, fmt=0x8cd550 "Cannot access memory at address %s") at ../../src/gdb/exceptions.c:422
#2  0x0000000000673a5f in memory_error (status=5, memaddr=6293992) at ../../src/gdb/corefile.c:204
#3  0x0000000000673aea in read_memory (memaddr=6293992, myaddr=0x7fffffffca60 "\200\316\377\377\377\177", len=4) at ../../src/gdb/corefile.c:223
#4  0x00000000006784d1 in dwarf_expr_read_mem (baton=0x7fffffffcd50, buf=0x7fffffffca60 "\200\316\377\377\377\177", addr=6293992, len=4) at ../../src/gdb/dwarf2loc.c:334
#5  0x000000000067645e in execute_stack_op (ctx=0x1409480, op_ptr=0x7fffffffce87 "\237<\005@", op_end=0x7fffffffce88 "<\005@") at ../../src/gdb/dwarf2expr.c:1045
#6  0x0000000000674e29 in dwarf_expr_eval (ctx=0x1409480, addr=0x7fffffffce80 "\003\350\t`", len=8) at ../../src/gdb/dwarf2expr.c:364
#7  0x000000000067c5b2 in dwarf2_evaluate_loc_desc_full (type=0x10876d0, frame=0xd8ecc0, data=0x7fffffffce80 "\003\350\t`", size=8, per_cu=0xf24c40, byte_offset=0)
    at ../../src/gdb/dwarf2loc.c:2236
#8  0x000000000067cc65 in dwarf2_evaluate_loc_desc (type=0x10876d0, frame=0xd8ecc0, data=0x7fffffffce80 "\003\350\t`", size=8, per_cu=0xf24c40)
    at ../../src/gdb/dwarf2loc.c:2407
#9  0x000000000067a5d4 in dwarf_entry_parameter_to_value (parameter=0x13a7960, deref_size=18446744073709551615, type=0x10876d0, caller_frame=0xd8ecc0, per_cu=0xf24c40)
    at ../../src/gdb/dwarf2loc.c:1160
#10 0x000000000067a962 in value_of_dwarf_reg_entry (type=0x10876d0, frame=0xd8de70, kind=CALL_SITE_PARAMETER_DWARF_REG, kind_u=...) at ../../src/gdb/dwarf2loc.c:1310
#11 0x000000000067aaca in value_of_dwarf_block_entry (type=0x10876d0, frame=0xd8de70, block=0xf1c2d4 "Q", block_len=1) at ../../src/gdb/dwarf2loc.c:1363
#12 0x000000000067e7c9 in locexpr_read_variable_at_entry (symbol=0x13a7540, frame=0xd8de70) at ../../src/gdb/dwarf2loc.c:3326
#13 0x00000000005daab6 in read_frame_arg (sym=0x13a7540, frame=0xd8de70, argp=0x7fffffffd0e0, entryargp=0x7fffffffd100) at ../../src/gdb/stack.c:362
#14 0x00000000005db384 in print_frame_args (func=0x13a7470, frame=0xd8de70, num=-1, stream=0xea3890) at ../../src/gdb/stack.c:669
#15 0x00000000005dc338 in print_frame (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC, print_args=1, sal=...) at ../../src/gdb/stack.c:1199
#16 0x00000000005db8ee in print_frame_info (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC, print_args=1) at ../../src/gdb/stack.c:851
#17 0x00000000005da2bb in print_stack_frame (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC) at ../../src/gdb/stack.c:169
#18 0x00000000005de236 in frame_command (level_exp=0x0, from_tty=1) at ../../src/gdb/stack.c:2265

dwarf2_evaluate_loc_desc_full (frame #7) knows to handle
NOT_AVAILABLE_ERROR errors, but read_memory always throws
a generic error.

Presently, only the value machinery knows to handle unavailable
memory.  We need to push the awareness down to the target_xfer layer,
making it return a finer grained error indication.  We can only return
a generic -1 nowadays, which leaves the upper layers with no clue on
why the xfer failed.  Use target_xfer_partial directly, rather than
propagating the error through target_read_memory so as to get a better
address to display in the error message.

(target_read_memory & friends build on top of target_read (thus the
target_xfer machinery), but turn all errors to EIO, an errno value.  I
think this is a mistake, and we'd better convert all these to return a
target_xfer_error too, but that can be done separately.  I looked
around a bit over memory_error calls, and the need to handle random
errno values, other than the EIOs gdb itself hardcodes, probably comes
(only) from deprecated_xfer_memory, which uses errno for error
indication, but I didn't look exhaustively.  We should really get rid
of that...)

Tested on x86_64 Fedora 17, native and gdbserver.

gdb/
2013-08-21  Pedro Alves  <palves@redhat.com>

	PR gdb/15871
	* corefile.c (target_xfer_memory_error): New function.
	(memory_error): Defer EIO to target_memory_error.
	(read_memory): Use target_xfer_partial, and handle finer-grained
	target xfer errors.
	* target.c (memory_xfer_partial_1): If memory is known to be
	unavailable, return -TARGET_XFER_E_UNAVAILABLE instead of -1.
	(target_xfer_partial): Make extern.
	* target.h (enum target_xfer_error): New.
	(target_xfer_partial): Declare.
	(struct target_ops) <xfer_partial>: Adjust describing comment.

gdb/testsuite/
2013-08-21  Pedro Alves  <palves@redhat.com>

	PR gdb/15871
	* gdb.trace/entry-values.exp: Remove kfail.
---
 gdb/corefile.c                           | 50 ++++++++++++++++++++++++++------
 gdb/target.c                             | 10 ++-----
 gdb/target.h                             | 33 +++++++++++++++++----
 gdb/testsuite/gdb.trace/entry-values.exp |  1 -
 4 files changed, 71 insertions(+), 23 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a86f4b3..23dfcb8 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -193,17 +193,38 @@ Use the \"file\" or \"exec-file\" command."));
 }
 \f
 
+/* Report a target xfer memory error by throwing a suitable
+   exception.  */
+
+static void
+target_xfer_memory_error (enum target_xfer_error err, CORE_ADDR memaddr)
+{
+  switch (err)
+    {
+    case TARGET_XFER_E_IO:
+      /* Actually, address between memaddr and memaddr + len was out of
+	 bounds.  */
+      throw_error (MEMORY_ERROR,
+		   _("Cannot access memory at address %s"),
+		   paddress (target_gdbarch (), memaddr));
+    case TARGET_XFER_E_UNAVAILABLE:
+      throw_error (NOT_AVAILABLE_ERROR,
+		   _("Memory at address %s unavailable."),
+		   paddress (target_gdbarch (), memaddr));
+    default:
+      internal_error (__FILE__, __LINE__,
+		      "unhandled target memory error: %s",
+		      plongest (err));
+    }
+}
+
 /* Report a memory error by throwing a MEMORY_ERROR error.  */
 
 void
 memory_error (int status, CORE_ADDR memaddr)
 {
   if (status == EIO)
-    /* Actually, address between memaddr and memaddr + len was out of
-       bounds.  */
-    throw_error (MEMORY_ERROR,
-		 _("Cannot access memory at address %s"),
-		 paddress (target_gdbarch (), memaddr));
+    target_xfer_memory_error (TARGET_XFER_E_IO, memaddr);
   else
     throw_error (MEMORY_ERROR,
 		 _("Error accessing memory address %s: %s."),
@@ -216,11 +237,22 @@ memory_error (int status, CORE_ADDR memaddr)
 void
 read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 {
-  int status;
+  LONGEST xfered = 0;
 
-  status = target_read_memory (memaddr, myaddr, len);
-  if (status != 0)
-    memory_error (status, memaddr);
+  while (xfered < len)
+    {
+      LONGEST xfer = target_xfer_partial (current_target.beneath,
+					  TARGET_OBJECT_MEMORY, NULL,
+					  myaddr + xfered, NULL,
+					  memaddr + xfered, len - xfered);
+
+      if (xfer == 0)
+	target_xfer_memory_error (TARGET_XFER_E_IO, memaddr + xfered);
+      if (xfer < 0)
+	target_xfer_memory_error (-xfer, memaddr + xfered);
+      xfered += xfer;
+      QUIT;
+    }
 }
 
 /* Same as target_read_stack, but report an error if can't read.  */
diff --git a/gdb/target.c b/gdb/target.c
index 377724d..97c8ce3 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -81,12 +81,6 @@ static LONGEST current_xfer_partial (struct target_ops *ops,
 				     const gdb_byte *writebuf,
 				     ULONGEST offset, LONGEST len);
 
-static LONGEST target_xfer_partial (struct target_ops *ops,
-				    enum target_object object,
-				    const char *annex,
-				    void *readbuf, const void *writebuf,
-				    ULONGEST offset, LONGEST len);
-
 static struct gdbarch *default_thread_architecture (struct target_ops *ops,
 						    ptid_t ptid);
 
@@ -1523,7 +1517,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 
 	      /* No use trying further, we know some memory starting
 		 at MEMADDR isn't available.  */
-	      return -1;
+	      return -TARGET_XFER_E_UNAVAILABLE;
 	    }
 
 	  /* Don't try to read more than how much is available, in
@@ -1700,7 +1694,7 @@ make_show_memory_breakpoints_cleanup (int show)
 
 /* For docs see target.h, to_xfer_partial.  */
 
-static LONGEST
+LONGEST
 target_xfer_partial (struct target_ops *ops,
 		     enum target_object object, const char *annex,
 		     void *readbuf, const void *writebuf,
diff --git a/gdb/target.h b/gdb/target.h
index d538e02..a3a4cfd 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -199,6 +199,20 @@ enum target_object
   /* Possible future objects: TARGET_OBJECT_FILE, ...  */
 };
 
+/* Possible error codes returned by target_xfer_partial, etc.  */
+
+enum target_xfer_error
+{
+  /* Generic I/O error.  Note that it's important this is '1', as we
+     still have target_xfer-related code returning hardcoded "-1" on
+     error.  */
+  TARGET_XFER_E_IO = 1,
+
+  /* Transfer failed because the piece of the object requested is
+     unavailable.  */
+  TARGET_XFER_E_UNAVAILABLE
+};
+
 /* Enumeration of the kinds of traceframe searches that a target may
    be able to perform.  */
 
@@ -293,6 +307,14 @@ extern char *target_read_stralloc (struct target_ops *ops,
 				   enum target_object object,
 				   const char *annex);
 
+/* See target_ops->to_xfer_partial.  */
+
+extern LONGEST target_xfer_partial (struct target_ops *ops,
+				    enum target_object object,
+				    const char *annex,
+				    void *readbuf, const void *writebuf,
+				    ULONGEST offset, LONGEST len);
+
 /* Wrappers to target read/write that perform memory transfers.  They
    throw an error if the memory transfer fails.
 
@@ -475,11 +497,12 @@ struct target_ops
        data-specific information to the target.
 
        Return the number of bytes actually transfered, zero when no
-       further transfer is possible, and -1 when the transfer is not
-       supported.  Return of a positive value smaller than LEN does
-       not indicate the end of the object, only the end of the
-       transfer; higher level code should continue transferring if
-       desired.  This is handled in target.c.
+       further transfer is possible, and a negative error code
+       (-TARGET_XFER_ERROR) when the transfer is not supported.
+       Return of a positive value smaller than LEN does not indicate
+       the end of the object, only the end of the transfer; higher
+       level code should continue transferring if desired.  This is
+       handled in target.c.
 
        The interface does not support a "retry" mechanism.  Instead it
        assumes that at least one byte will be transfered on each
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
index bfa4f5c..20d06ce 100644
--- a/gdb/testsuite/gdb.trace/entry-values.exp
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -265,7 +265,6 @@ gdb_test_no_output "tstop"
 gdb_test "tfind" "Found trace frame 0, .*" "tfind start"
 
 # Since 'global2' is not collected, j@entry is expected to be 'unavailable'.
-setup_kfail "gdb/15871" *-*-*
 gdb_test "bt 1" "#0 .* foo \\(i=\[-\]?$decimal, i@entry=2, j=\[-\]?$decimal, j@entry=<unavailable>\\).*"
 
 gdb_test "tfind" "Target failed to find requested trace frame\..*"
-- 
1.7.11.7

Re: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

[Mark Kettenis]

> Date: Wed, 21 Aug 2013 15:35:40 +0100
> From: Pedro Alves <palves@redhat.com>
> 
> On 08/13/2013 08:39 AM, Yao Qi wrote:
> > However, when I run this test, I find j@entry is something like,
> > j@entry=<error reading variable: Cannot access memory at address 0x8049788>
> > instead of unavailable.
> > 
> > I don't emit a fail for it because I am not very sure it is expected
> > to be "unavailable".  I am fine to kfail it.
> > 
> > I looked into a little, and looks reading entry value doesn't use
> > value availability-aware API.  It is not an easy fix to me.
> 
> I looked into this.  Here's a patch.  Let me know what you think.

I think you should simply define the return codes as negative numbers.

Re: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

[Pedro Alves]

On 08/21/2013 03:47 PM, Mark Kettenis wrote:
>> Date: Wed, 21 Aug 2013 15:35:40 +0100
>> From: Pedro Alves <palves@redhat.com>
>>
>> On 08/13/2013 08:39 AM, Yao Qi wrote:
>>> However, when I run this test, I find j@entry is something like,
>>> j@entry=<error reading variable: Cannot access memory at address 0x8049788>
>>> instead of unavailable.
>>>
>>> I don't emit a fail for it because I am not very sure it is expected
>>> to be "unavailable".  I am fine to kfail it.
>>>
>>> I looked into a little, and looks reading entry value doesn't use
>>> value availability-aware API.  It is not an easy fix to me.
>>
>> I looked into this.  Here's a patch.  Let me know what you think.
> 
> I think you should simply define the return codes as negative numbers.

I think that's really a matter of taste.  But maybe my taste is
broken...  It's quite common to return "-errno" as error indication.
The Linux kernel uses that convention for example.

Here's the updated patch.

----------
Subject: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

In entry-values.exp, we have a test where the entry value of 'j' is
unavailable, so it is expected that printing j@entry yields
"<unavailable>".  However, the actual output is:

 (gdb) frame
 #0  0x0000000000400540 in foo (i=0, i@entry=2, j=2, j@entry=<error reading variable: Cannot access memory at address 0x6009e8>)

The error is thrown here:

#0  throw_it (reason=RETURN_ERROR, error=MEMORY_ERROR, fmt=0x8cd550 "Cannot access memory at address %s", ap=0x7fffffffc8e8) at ../../src/gdb/exceptions.c:373
#1  0x00000000005e2f9c in throw_error (error=MEMORY_ERROR, fmt=0x8cd550 "Cannot access memory at address %s") at ../../src/gdb/exceptions.c:422
#2  0x0000000000673a5f in memory_error (status=5, memaddr=6293992) at ../../src/gdb/corefile.c:204
#3  0x0000000000673aea in read_memory (memaddr=6293992, myaddr=0x7fffffffca60 "\200\316\377\377\377\177", len=4) at ../../src/gdb/corefile.c:223
#4  0x00000000006784d1 in dwarf_expr_read_mem (baton=0x7fffffffcd50, buf=0x7fffffffca60 "\200\316\377\377\377\177", addr=6293992, len=4) at ../../src/gdb/dwarf2loc.c:334
#5  0x000000000067645e in execute_stack_op (ctx=0x1409480, op_ptr=0x7fffffffce87 "\237<\005@", op_end=0x7fffffffce88 "<\005@") at ../../src/gdb/dwarf2expr.c:1045
#6  0x0000000000674e29 in dwarf_expr_eval (ctx=0x1409480, addr=0x7fffffffce80 "\003\350\t`", len=8) at ../../src/gdb/dwarf2expr.c:364
#7  0x000000000067c5b2 in dwarf2_evaluate_loc_desc_full (type=0x10876d0, frame=0xd8ecc0, data=0x7fffffffce80 "\003\350\t`", size=8, per_cu=0xf24c40, byte_offset=0)
    at ../../src/gdb/dwarf2loc.c:2236
#8  0x000000000067cc65 in dwarf2_evaluate_loc_desc (type=0x10876d0, frame=0xd8ecc0, data=0x7fffffffce80 "\003\350\t`", size=8, per_cu=0xf24c40)
    at ../../src/gdb/dwarf2loc.c:2407
#9  0x000000000067a5d4 in dwarf_entry_parameter_to_value (parameter=0x13a7960, deref_size=18446744073709551615, type=0x10876d0, caller_frame=0xd8ecc0, per_cu=0xf24c40)
    at ../../src/gdb/dwarf2loc.c:1160
#10 0x000000000067a962 in value_of_dwarf_reg_entry (type=0x10876d0, frame=0xd8de70, kind=CALL_SITE_PARAMETER_DWARF_REG, kind_u=...) at ../../src/gdb/dwarf2loc.c:1310
#11 0x000000000067aaca in value_of_dwarf_block_entry (type=0x10876d0, frame=0xd8de70, block=0xf1c2d4 "Q", block_len=1) at ../../src/gdb/dwarf2loc.c:1363
#12 0x000000000067e7c9 in locexpr_read_variable_at_entry (symbol=0x13a7540, frame=0xd8de70) at ../../src/gdb/dwarf2loc.c:3326
#13 0x00000000005daab6 in read_frame_arg (sym=0x13a7540, frame=0xd8de70, argp=0x7fffffffd0e0, entryargp=0x7fffffffd100) at ../../src/gdb/stack.c:362
#14 0x00000000005db384 in print_frame_args (func=0x13a7470, frame=0xd8de70, num=-1, stream=0xea3890) at ../../src/gdb/stack.c:669
#15 0x00000000005dc338 in print_frame (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC, print_args=1, sal=...) at ../../src/gdb/stack.c:1199
#16 0x00000000005db8ee in print_frame_info (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC, print_args=1) at ../../src/gdb/stack.c:851
#17 0x00000000005da2bb in print_stack_frame (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC) at ../../src/gdb/stack.c:169
#18 0x00000000005de236 in frame_command (level_exp=0x0, from_tty=1) at ../../src/gdb/stack.c:2265

dwarf2_evaluate_loc_desc_full (frame #7) knows to handle
NOT_AVAILABLE_ERROR errors, but read_memory always throws
a generic error.

Presently, only the value machinery knows to handle unavailable
memory.  We need to push the awareness down to the target_xfer layer,
making it return a finer grained error indication.  We can only return
a generic -1 nowadays, which leaves the upper layers with no clue on
why the xfer failed.  Use target_xfer_partial directly, rather than
propagating the error through target_read_memory so as to get a better
address to display in the error message.

(target_read_memory & friends build on top of target_read (thus the
target_xfer machinery), but turn all errors to EIO, an errno value.  I
think this is a mistake, and we'd better convert all these to return a
target_xfer_error too, but that can be done separately.  I looked
around a bit over memory_error calls, and the need to handle random
errno values, other than the EIOs gdb itself hardcodes, probably comes
(only) from deprecated_xfer_memory, which uses errno for error
indication, but I didn't look exhaustively.  We should really get rid
of that...)

Tested on x86_64 Fedora 17, native and gdbserver.

gdb/
2013-08-21  Pedro Alves  <palves@redhat.com>

	PR gdb/15871
	* corefile.c (target_xfer_memory_error): New function.
	(memory_error): Defer EIO to target_memory_error.
	(read_memory): Use target_xfer_partial, and handle finer-grained
	target xfer errors.
	* target.c (memory_xfer_partial_1): If memory is known to be
	unavailable, return TARGET_XFER_E_UNAVAILABLE instead of -1.
	(target_xfer_partial): Make extern.
	* target.h (enum target_xfer_error): New.
	(target_xfer_partial): Declare.
	(struct target_ops) <xfer_partial>: Adjust describing comment.

gdb/testsuite/
2013-08-21  Pedro Alves  <palves@redhat.com>

	PR gdb/15871
	* gdb.trace/entry-values.exp: Remove kfail.
---
 gdb/corefile.c                           | 50 ++++++++++++++++++++++++++------
 gdb/target.c                             | 10 ++-----
 gdb/target.h                             | 25 +++++++++++++++-
 gdb/testsuite/gdb.trace/entry-values.exp |  1 -
 4 files changed, 67 insertions(+), 19 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a86f4b3..c86c172 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -193,17 +193,38 @@ Use the \"file\" or \"exec-file\" command."));
 }
 \f
 
+/* Report a target xfer memory error by throwing a suitable
+   exception.  */
+
+static void
+target_xfer_memory_error (enum target_xfer_error err, CORE_ADDR memaddr)
+{
+  switch (err)
+    {
+    case TARGET_XFER_E_IO:
+      /* Actually, address between memaddr and memaddr + len was out of
+	 bounds.  */
+      throw_error (MEMORY_ERROR,
+		   _("Cannot access memory at address %s"),
+		   paddress (target_gdbarch (), memaddr));
+    case TARGET_XFER_E_UNAVAILABLE:
+      throw_error (NOT_AVAILABLE_ERROR,
+		   _("Memory at address %s unavailable."),
+		   paddress (target_gdbarch (), memaddr));
+    default:
+      internal_error (__FILE__, __LINE__,
+		      "unhandled target xfer memory error: %s",
+		      plongest (err));
+    }
+}
+
 /* Report a memory error by throwing a MEMORY_ERROR error.  */
 
 void
 memory_error (int status, CORE_ADDR memaddr)
 {
   if (status == EIO)
-    /* Actually, address between memaddr and memaddr + len was out of
-       bounds.  */
-    throw_error (MEMORY_ERROR,
-		 _("Cannot access memory at address %s"),
-		 paddress (target_gdbarch (), memaddr));
+    target_xfer_memory_error (TARGET_XFER_E_IO, memaddr);
   else
     throw_error (MEMORY_ERROR,
 		 _("Error accessing memory address %s: %s."),
@@ -216,11 +237,22 @@ memory_error (int status, CORE_ADDR memaddr)
 void
 read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 {
-  int status;
+  LONGEST xfered = 0;
 
-  status = target_read_memory (memaddr, myaddr, len);
-  if (status != 0)
-    memory_error (status, memaddr);
+  while (xfered < len)
+    {
+      LONGEST xfer = target_xfer_partial (current_target.beneath,
+					  TARGET_OBJECT_MEMORY, NULL,
+					  myaddr + xfered, NULL,
+					  memaddr + xfered, len - xfered);
+
+      if (xfer == 0)
+	target_xfer_memory_error (TARGET_XFER_E_IO, memaddr + xfered);
+      if (xfer < 0)
+	target_xfer_memory_error (xfer, memaddr + xfered);
+      xfered += xfer;
+      QUIT;
+    }
 }
 
 /* Same as target_read_stack, but report an error if can't read.  */
diff --git a/gdb/target.c b/gdb/target.c
index 377724d..f17e53e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -81,12 +81,6 @@ static LONGEST current_xfer_partial (struct target_ops *ops,
 				     const gdb_byte *writebuf,
 				     ULONGEST offset, LONGEST len);
 
-static LONGEST target_xfer_partial (struct target_ops *ops,
-				    enum target_object object,
-				    const char *annex,
-				    void *readbuf, const void *writebuf,
-				    ULONGEST offset, LONGEST len);
-
 static struct gdbarch *default_thread_architecture (struct target_ops *ops,
 						    ptid_t ptid);
 
@@ -1523,7 +1517,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 
 	      /* No use trying further, we know some memory starting
 		 at MEMADDR isn't available.  */
-	      return -1;
+	      return TARGET_XFER_E_UNAVAILABLE;
 	    }
 
 	  /* Don't try to read more than how much is available, in
@@ -1700,7 +1694,7 @@ make_show_memory_breakpoints_cleanup (int show)
 
 /* For docs see target.h, to_xfer_partial.  */
 
-static LONGEST
+LONGEST
 target_xfer_partial (struct target_ops *ops,
 		     enum target_object object, const char *annex,
 		     void *readbuf, const void *writebuf,
diff --git a/gdb/target.h b/gdb/target.h
index d538e02..64a8b0f 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -199,6 +199,20 @@ enum target_object
   /* Possible future objects: TARGET_OBJECT_FILE, ...  */
 };
 
+/* Possible error codes returned by target_xfer_partial, etc.  */
+
+enum target_xfer_error
+{
+  /* Generic I/O error.  Note that it's important that this is '-1',
+     as we still have target_xfer-related code returning hardcoded
+     '-1' on error.  */
+  TARGET_XFER_E_IO = -1,
+
+  /* Transfer failed because the piece of the object requested is
+     unavailable.  */
+  TARGET_XFER_E_UNAVAILABLE = -2
+};
+
 /* Enumeration of the kinds of traceframe searches that a target may
    be able to perform.  */
 
@@ -293,6 +307,14 @@ extern char *target_read_stralloc (struct target_ops *ops,
 				   enum target_object object,
 				   const char *annex);
 
+/* See target_ops->to_xfer_partial.  */
+
+extern LONGEST target_xfer_partial (struct target_ops *ops,
+				    enum target_object object,
+				    const char *annex,
+				    void *readbuf, const void *writebuf,
+				    ULONGEST offset, LONGEST len);
+
 /* Wrappers to target read/write that perform memory transfers.  They
    throw an error if the memory transfer fails.
 
@@ -475,7 +497,8 @@ struct target_ops
        data-specific information to the target.
 
        Return the number of bytes actually transfered, zero when no
-       further transfer is possible, and -1 when the transfer is not
+       further transfer is possible, and a negative error code (really
+       an 'enum target_xfer_error' value) when the transfer is not
        supported.  Return of a positive value smaller than LEN does
        not indicate the end of the object, only the end of the
        transfer; higher level code should continue transferring if
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
index bfa4f5c..20d06ce 100644
--- a/gdb/testsuite/gdb.trace/entry-values.exp
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -265,7 +265,6 @@ gdb_test_no_output "tstop"
 gdb_test "tfind" "Found trace frame 0, .*" "tfind start"
 
 # Since 'global2' is not collected, j@entry is expected to be 'unavailable'.
-setup_kfail "gdb/15871" *-*-*
 gdb_test "bt 1" "#0 .* foo \\(i=\[-\]?$decimal, i@entry=2, j=\[-\]?$decimal, j@entry=<unavailable>\\).*"
 
 gdb_test "tfind" "Target failed to find requested trace frame\..*"
-- 
1.7.11.7

Re: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

[Mark Kettenis]

> Date: Wed, 21 Aug 2013 16:32:21 +0100
> From: Pedro Alves <palves@redhat.com>
> 
> On 08/21/2013 03:47 PM, Mark Kettenis wrote:
> >> Date: Wed, 21 Aug 2013 15:35:40 +0100
> >> From: Pedro Alves <palves@redhat.com>
> >>
> >> On 08/13/2013 08:39 AM, Yao Qi wrote:
> >>> However, when I run this test, I find j@entry is something like,
> >>> j@entry=<error reading variable: Cannot access memory at address 0x8049788>
> >>> instead of unavailable.
> >>>
> >>> I don't emit a fail for it because I am not very sure it is expected
> >>> to be "unavailable".  I am fine to kfail it.
> >>>
> >>> I looked into a little, and looks reading entry value doesn't use
> >>> value availability-aware API.  It is not an easy fix to me.
> >>
> >> I looked into this.  Here's a patch.  Let me know what you think.
> > 
> > I think you should simply define the return codes as negative numbers.
> 
> I think that's really a matter of taste.  But maybe my taste is
> broken...  It's quite common to return "-errno" as error indication.
> The Linux kernel uses that convention for example.

Yeah.  I was tempted to add that you've been exposed to too much Linux
kernel programming ;).

Anyway, I'd argue it's more than a matter of taste.  Even in the
little bit of code you used the enum values both with and without a
minus sign.  That's confusing.  Thanks for fixing this.  Diff looks
reasonable to me.

Re: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

[Yao Qi]

On 08/21/2013 11:32 PM, Pedro Alves wrote:
> I looked
> around a bit over memory_error calls, and the need to handle random
> errno values, other than the EIOs gdb itself hardcodes, probably comes
> (only) from deprecated_xfer_memory, which uses errno for error

Is it in function default_xfer_partial?

> indication, but I didn't look exhaustively.  We should really get rid
> of that...)
>

What do you mean by "that"? using errno for error indication?

> +/* Report a target xfer memory error by throwing a suitable
> +   exception.  */
> +
> +static void
> +target_xfer_memory_error (enum target_xfer_error err, CORE_ADDR memaddr)
> +{
> +  switch (err)
> +    {
> +    case TARGET_XFER_E_IO:
> +      /* Actually, address between memaddr and memaddr + len was out of
> +	 bounds.  */

This line of comment doesn't make much sense in the context of this
function.

This patch looks good to me.  Please commit it without the change to
gdb.trace/entry-values.exp, and I'll update my patch to remove kfail.

A suggestion here, IWBN to print the string of 'enum target_xfer_error'
in the debugging message at the end of target_xfer_partial.  We are
print numbers currently.

-- 
Yao (齐尧)

[COMMIT] Re: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

[Pedro Alves]

On 08/22/2013 02:24 AM, Yao Qi wrote:
> On 08/21/2013 11:32 PM, Pedro Alves wrote:
>> I looked
>> around a bit over memory_error calls, and the need to handle random
>> errno values, other than the EIOs gdb itself hardcodes, probably comes
>> (only) from deprecated_xfer_memory, which uses errno for error
> 
> Is it in function default_xfer_partial?

Yeah, it's used there, and it seems the actual errno value
is just ignored nowadays.  Only zero vs non-zero is checked.

> 
>> indication, but I didn't look exhaustively.  We should really get rid
>> of that...)
>>
> 
> What do you mean by "that"? using errno for error indication?

I meant deprecated_xfer_memory, which does:

       0 means that we can't handle this.  If errno has been set, it is the
       error which prevented us from doing it (FIXME: What about bfd_error?).

and using errno values as error indication in target.h methods.  E.g.,
target_read_memory:

/* Read LEN bytes of target memory at address MEMADDR, placing the results in
   GDB's memory at MYADDR.  Returns either 0 for success or an errno value
   if any error occurs.

using errno values limits what we can return here.  There's no
errno for <unavailable>, for example.  So, it'd be better if
we returned target_xfer_error instead.  If we needed to indicate
an errno or a bfd error in addition, new TARGET_XFER_E_ERRNO or
TARGET_XFER_E_BFD values could be added to target_xfer_error
(and the caller would then have to consult errno or bfd_error
in addition then).

> 
>> +/* Report a target xfer memory error by throwing a suitable
>> +   exception.  */
>> +
>> +static void
>> +target_xfer_memory_error (enum target_xfer_error err, CORE_ADDR memaddr)
>> +{
>> +  switch (err)
>> +    {
>> +    case TARGET_XFER_E_IO:
>> +      /* Actually, address between memaddr and memaddr + len was out of
>> +	 bounds.  */
> 
> This line of comment doesn't make much sense in the context of this
> function.

I think it makes some sense if you consider the context that the caller
always has a length handy.  But yeah.  This is a preexisting issue
though (see memory_error, this new function can be looked at as
a refactor).  Maybe what we should do is actually pass in the
length, and show the range to the user instead of just the first
address.  Sometimes that info is useful, as it may not be the first
address in the requested range that actually is inaccessible.

> 
> This patch looks good to me.  Please commit it without the change to
> gdb.trace/entry-values.exp, and I'll update my patch to remove kfail.

OK, done, as below.

> A suggestion here, IWBN to print the string of 'enum target_xfer_error'
> in the debugging message at the end of target_xfer_partial.  We are
> print numbers currently.

Done.  We now get:

internal-error: unhandled target_xfer_error: <unknown> (-3)
internal-error: unhandled target_xfer_error: TARGET_XFER_E_UNAVAILABLE (-2)
internal-error: unhandled target_xfer_error: TARGET_XFER_E_IO (-1)
internal-error: unhandled target_xfer_error: <unknown> (0)
internal-error: unhandled target_xfer_error: <unknown> (1)

-------
Subject: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

In entry-values.exp, we have a test where the entry value of 'j' is
unavailable, so it is expected that printing j@entry yields
"<unavailable>".  However, the actual output is:

 (gdb) frame
 #0  0x0000000000400540 in foo (i=0, i@entry=2, j=2, j@entry=<error reading variable: Cannot access memory at address 0x6009e8>)

The error is thrown here:

#0  throw_it (reason=RETURN_ERROR, error=MEMORY_ERROR, fmt=0x8cd550 "Cannot access memory at address %s", ap=0x7fffffffc8e8) at ../../src/gdb/exceptions.c:373
#1  0x00000000005e2f9c in throw_error (error=MEMORY_ERROR, fmt=0x8cd550 "Cannot access memory at address %s") at ../../src/gdb/exceptions.c:422
#2  0x0000000000673a5f in memory_error (status=5, memaddr=6293992) at ../../src/gdb/corefile.c:204
#3  0x0000000000673aea in read_memory (memaddr=6293992, myaddr=0x7fffffffca60 "\200\316\377\377\377\177", len=4) at ../../src/gdb/corefile.c:223
#4  0x00000000006784d1 in dwarf_expr_read_mem (baton=0x7fffffffcd50, buf=0x7fffffffca60 "\200\316\377\377\377\177", addr=6293992, len=4) at ../../src/gdb/dwarf2loc.c:334
#5  0x000000000067645e in execute_stack_op (ctx=0x1409480, op_ptr=0x7fffffffce87 "\237<\005@", op_end=0x7fffffffce88 "<\005@") at ../../src/gdb/dwarf2expr.c:1045
#6  0x0000000000674e29 in dwarf_expr_eval (ctx=0x1409480, addr=0x7fffffffce80 "\003\350\t`", len=8) at ../../src/gdb/dwarf2expr.c:364
#7  0x000000000067c5b2 in dwarf2_evaluate_loc_desc_full (type=0x10876d0, frame=0xd8ecc0, data=0x7fffffffce80 "\003\350\t`", size=8, per_cu=0xf24c40, byte_offset=0)
    at ../../src/gdb/dwarf2loc.c:2236
#8  0x000000000067cc65 in dwarf2_evaluate_loc_desc (type=0x10876d0, frame=0xd8ecc0, data=0x7fffffffce80 "\003\350\t`", size=8, per_cu=0xf24c40)
    at ../../src/gdb/dwarf2loc.c:2407
#9  0x000000000067a5d4 in dwarf_entry_parameter_to_value (parameter=0x13a7960, deref_size=18446744073709551615, type=0x10876d0, caller_frame=0xd8ecc0, per_cu=0xf24c40)
    at ../../src/gdb/dwarf2loc.c:1160
#10 0x000000000067a962 in value_of_dwarf_reg_entry (type=0x10876d0, frame=0xd8de70, kind=CALL_SITE_PARAMETER_DWARF_REG, kind_u=...) at ../../src/gdb/dwarf2loc.c:1310
#11 0x000000000067aaca in value_of_dwarf_block_entry (type=0x10876d0, frame=0xd8de70, block=0xf1c2d4 "Q", block_len=1) at ../../src/gdb/dwarf2loc.c:1363
#12 0x000000000067e7c9 in locexpr_read_variable_at_entry (symbol=0x13a7540, frame=0xd8de70) at ../../src/gdb/dwarf2loc.c:3326
#13 0x00000000005daab6 in read_frame_arg (sym=0x13a7540, frame=0xd8de70, argp=0x7fffffffd0e0, entryargp=0x7fffffffd100) at ../../src/gdb/stack.c:362
#14 0x00000000005db384 in print_frame_args (func=0x13a7470, frame=0xd8de70, num=-1, stream=0xea3890) at ../../src/gdb/stack.c:669
#15 0x00000000005dc338 in print_frame (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC, print_args=1, sal=...) at ../../src/gdb/stack.c:1199
#16 0x00000000005db8ee in print_frame_info (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC, print_args=1) at ../../src/gdb/stack.c:851
#17 0x00000000005da2bb in print_stack_frame (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC) at ../../src/gdb/stack.c:169
#18 0x00000000005de236 in frame_command (level_exp=0x0, from_tty=1) at ../../src/gdb/stack.c:2265

dwarf2_evaluate_loc_desc_full (frame #7) knows to handle
NOT_AVAILABLE_ERROR errors, but read_memory always throws
a generic error.

Presently, only the value machinery knows to handle unavailable
memory.  We need to push the awareness down to the target_xfer layer,
making it return a finer grained error indication.  We can only return
a generic -1 nowadays, which leaves the upper layers with no clue on
why the xfer failed.  Use target_xfer_partial directly, rather than
propagating the error through target_read_memory so as to get a better
address to display in the error message.

(target_read_memory & friends build on top of target_read (thus the
target_xfer machinery), but turn all errors to EIO, an errno value.  I
think this is a mistake, and we'd better convert all these to return a
target_xfer_error too, but that can be done separately.  I looked
around a bit over memory_error calls, and the need to handle random
errno values, other than the EIOs gdb itself hardcodes, probably comes
(only) from deprecated_xfer_memory, which uses errno for error
indication, but I didn't look exhaustively.  We should really get rid
of deprecated_xfer_memory and of passing down errno values as error
indication in target_read & friends methods).

Tested on x86_64 Fedora 17, native and gdbserver.  Fixes the test in
the PR, which will be added to the testsuite later.

gdb/
2013-08-22  Pedro Alves  <palves@redhat.com>

	PR gdb/15871
	* corefile.c (target_xfer_memory_error): New function.
	(memory_error): Defer EIO to target_memory_error.
	(read_memory): Use target_xfer_partial, and handle finer-grained
	target xfer errors.
	* target.c (target_xfer_error_to_string): New function.
	(memory_xfer_partial_1): If memory is known to be
	unavailable, return TARGET_XFER_E_UNAVAILABLE instead of -1.
	(target_xfer_partial): Make extern.
	* target.h (enum target_xfer_error): New enum.
	(target_xfer_error_to_string): Declare function.
	(target_xfer_partial): Declare function.
	(struct target_ops) <xfer_partial>: Adjust describing comment.
---
 gdb/corefile.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
 gdb/target.c   | 25 +++++++++++++++++--------
 gdb/target.h   | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 89 insertions(+), 18 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a86f4b3..cb7f14e 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -193,17 +193,39 @@ Use the \"file\" or \"exec-file\" command."));
 }
 \f
 
+/* Report a target xfer memory error by throwing a suitable
+   exception.  */
+
+static void
+target_xfer_memory_error (enum target_xfer_error err, CORE_ADDR memaddr)
+{
+  switch (err)
+    {
+    case TARGET_XFER_E_IO:
+      /* Actually, address between memaddr and memaddr + len was out of
+	 bounds.  */
+      throw_error (MEMORY_ERROR,
+		   _("Cannot access memory at address %s"),
+		   paddress (target_gdbarch (), memaddr));
+    case TARGET_XFER_E_UNAVAILABLE:
+      throw_error (NOT_AVAILABLE_ERROR,
+		   _("Memory at address %s unavailable."),
+		   paddress (target_gdbarch (), memaddr));
+    default:
+      internal_error (__FILE__, __LINE__,
+		      "unhandled target_xfer_error: %s (%s)",
+		      target_xfer_error_to_string (err),
+		      plongest (err));
+    }
+}
+
 /* Report a memory error by throwing a MEMORY_ERROR error.  */
 
 void
 memory_error (int status, CORE_ADDR memaddr)
 {
   if (status == EIO)
-    /* Actually, address between memaddr and memaddr + len was out of
-       bounds.  */
-    throw_error (MEMORY_ERROR,
-		 _("Cannot access memory at address %s"),
-		 paddress (target_gdbarch (), memaddr));
+    target_xfer_memory_error (TARGET_XFER_E_IO, memaddr);
   else
     throw_error (MEMORY_ERROR,
 		 _("Error accessing memory address %s: %s."),
@@ -216,11 +238,22 @@ memory_error (int status, CORE_ADDR memaddr)
 void
 read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 {
-  int status;
+  LONGEST xfered = 0;
 
-  status = target_read_memory (memaddr, myaddr, len);
-  if (status != 0)
-    memory_error (status, memaddr);
+  while (xfered < len)
+    {
+      LONGEST xfer = target_xfer_partial (current_target.beneath,
+					  TARGET_OBJECT_MEMORY, NULL,
+					  myaddr + xfered, NULL,
+					  memaddr + xfered, len - xfered);
+
+      if (xfer == 0)
+	target_xfer_memory_error (TARGET_XFER_E_IO, memaddr + xfered);
+      if (xfer < 0)
+	target_xfer_memory_error (xfer, memaddr + xfered);
+      xfered += xfer;
+      QUIT;
+    }
 }
 
 /* Same as target_read_stack, but report an error if can't read.  */
diff --git a/gdb/target.c b/gdb/target.c
index 377724d..f18661b 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -81,12 +81,6 @@ static LONGEST current_xfer_partial (struct target_ops *ops,
 				     const gdb_byte *writebuf,
 				     ULONGEST offset, LONGEST len);
 
-static LONGEST target_xfer_partial (struct target_ops *ops,
-				    enum target_object object,
-				    const char *annex,
-				    void *readbuf, const void *writebuf,
-				    ULONGEST offset, LONGEST len);
-
 static struct gdbarch *default_thread_architecture (struct target_ops *ops,
 						    ptid_t ptid);
 
@@ -1238,6 +1232,21 @@ target_translate_tls_address (struct objfile *objfile, CORE_ADDR offset)
   return addr;
 }
 
+const char *
+target_xfer_error_to_string (enum target_xfer_error err)
+{
+#define CASE(X) case X: return #X
+  switch (err)
+    {
+      CASE(TARGET_XFER_E_IO);
+      CASE(TARGET_XFER_E_UNAVAILABLE);
+    default:
+      return "<unknown>";
+    }
+#undef CASE
+};
+
+
 #undef	MIN
 #define MIN(A, B) (((A) <= (B)) ? (A) : (B))
 
@@ -1523,7 +1532,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 
 	      /* No use trying further, we know some memory starting
 		 at MEMADDR isn't available.  */
-	      return -1;
+	      return TARGET_XFER_E_UNAVAILABLE;
 	    }
 
 	  /* Don't try to read more than how much is available, in
@@ -1700,7 +1709,7 @@ make_show_memory_breakpoints_cleanup (int show)
 
 /* For docs see target.h, to_xfer_partial.  */
 
-static LONGEST
+LONGEST
 target_xfer_partial (struct target_ops *ops,
 		     enum target_object object, const char *annex,
 		     void *readbuf, const void *writebuf,
diff --git a/gdb/target.h b/gdb/target.h
index d538e02..6959503 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -199,6 +199,26 @@ enum target_object
   /* Possible future objects: TARGET_OBJECT_FILE, ...  */
 };
 
+/* Possible error codes returned by target_xfer_partial, etc.  */
+
+enum target_xfer_error
+{
+  /* Generic I/O error.  Note that it's important that this is '-1',
+     as we still have target_xfer-related code returning hardcoded
+     '-1' on error.  */
+  TARGET_XFER_E_IO = -1,
+
+  /* Transfer failed because the piece of the object requested is
+     unavailable.  */
+  TARGET_XFER_E_UNAVAILABLE = -2,
+
+  /* Keep list in sync with target_xfer_error_to_string.  */
+};
+
+/* Return the string form of ERR.  */
+
+extern const char *target_xfer_error_to_string (enum target_xfer_error err);
+
 /* Enumeration of the kinds of traceframe searches that a target may
    be able to perform.  */
 
@@ -293,6 +313,14 @@ extern char *target_read_stralloc (struct target_ops *ops,
 				   enum target_object object,
 				   const char *annex);
 
+/* See target_ops->to_xfer_partial.  */
+
+extern LONGEST target_xfer_partial (struct target_ops *ops,
+				    enum target_object object,
+				    const char *annex,
+				    void *readbuf, const void *writebuf,
+				    ULONGEST offset, LONGEST len);
+
 /* Wrappers to target read/write that perform memory transfers.  They
    throw an error if the memory transfer fails.
 
@@ -475,7 +503,8 @@ struct target_ops
        data-specific information to the target.
 
        Return the number of bytes actually transfered, zero when no
-       further transfer is possible, and -1 when the transfer is not
+       further transfer is possible, and a negative error code (really
+       an 'enum target_xfer_error' value) when the transfer is not
        supported.  Return of a positive value smaller than LEN does
        not indicate the end of the object, only the end of the
        transfer; higher level code should continue transferring if
-- 
1.7.11.7

Re: [COMMIT] Re: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

[Yao Qi]

On 08/22/2013 06:04 PM, Pedro Alves wrote:
> I meant deprecated_xfer_memory, which does:
>
>         0 means that we can't handle this.  If errno has been set, it is the
>         error which prevented us from doing it (FIXME: What about bfd_error?).
>
> and using errno values as error indication in target.h methods.  E.g.,
> target_read_memory:
>
> /* Read LEN bytes of target memory at address MEMADDR, placing the results in
>     GDB's memory at MYADDR.  Returns either 0 for success or an errno value
>     if any error occurs.
>
> using errno values limits what we can return here.  There's no
> errno for <unavailable>, for example.  So, it'd be better if
> we returned target_xfer_error instead.  If we needed to indicate
> an errno or a bfd error in addition, new TARGET_XFER_E_ERRNO or
> TARGET_XFER_E_BFD values could be added to target_xfer_error
> (and the caller would then have to consult errno or bfd_error
> in addition then).
>

OK, it is clear to me.

I'd like to write this down in section "Internals" in 
http://sourceware.org/gdb/wiki/ProjectIdeas , let me know what do you think.

Use target_xfer_error instead of errno.  Some xfer_memory functions 
(such as target_read_memory and deprecated_xfer_memory) are
using errno to indicate any error occurs.  This limites what we can 
return here, because we can't return some GDB-specific errors, such as 
<unavailable>.  If we need to indicate an errno, bfd error for example, 
a new TARGET_XFER_E_BFD can be added to target_xfer_error.  See
http://sourceware.org/ml/gdb-patches/2013-08/msg00589.html for more info.

>> >
>>> >>+/* Report a target xfer memory error by throwing a suitable
>>> >>+   exception.  */
>>> >>+
>>> >>+static void
>>> >>+target_xfer_memory_error (enum target_xfer_error err, CORE_ADDR memaddr)
>>> >>+{
>>> >>+  switch (err)
>>> >>+    {
>>> >>+    case TARGET_XFER_E_IO:
>>> >>+      /* Actually, address between memaddr and memaddr + len was out of
>>> >>+	 bounds.  */
>> >
>> >This line of comment doesn't make much sense in the context of this
>> >function.
> I think it makes some sense if you consider the context that the caller
> always has a length handy.  But yeah.  This is a preexisting issue
> though (see memory_error, this new function can be looked at as
> a refactor).  Maybe what we should do is actually pass in the
> length, and show the range to the user instead of just the first
> address.  Sometimes that info is useful, as it may not be the first
> address in the requested range that actually is inaccessible.
>

Yeah, printing the range is useful some times.

-- 
Yao (齐尧)

Re: [COMMIT] Re: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

[Pedro Alves]

On 08/23/2013 02:07 AM, Yao Qi wrote:

> I'd like to write this down in section "Internals" in 
> http://sourceware.org/gdb/wiki/ProjectIdeas , let me know what do you think.
> 
> Use target_xfer_error instead of errno.  Some xfer_memory functions 
> (such as target_read_memory and deprecated_xfer_memory) are
> using errno to indicate any error occurs.  This limites what we can 
> return here, because we can't return some GDB-specific errors, such as 
> <unavailable>.  If we need to indicate an errno, bfd error for example, 
> a new TARGET_XFER_E_BFD can be added to target_xfer_error.  See
> http://sourceware.org/ml/gdb-patches/2013-08/msg00589.html for more info.

Thanks, but rather than end up with an incomplete transition,
I went ahead and did the legwork:

 http://sourceware.org/ml/gdb-patches/2013-08/msg00687.html

deprecated_xfer_memory should just be eliminated.  I've now
cleaned up remote.c:

  http://sourceware.org/ml/gdb-patches/2013-08/msg00668.html

A few more to go still ...

$ grep "deprecated_xfer_memory.*=" *.c | grep -v target.c
darwin-nat.c:  darwin_ops->deprecated_xfer_memory = darwin_xfer_memory;
gnu-nat.c:  t->deprecated_xfer_memory = gnu_xfer_memory;
go32-nat.c:  go32_ops.deprecated_xfer_memory = go32_xfer_memory;
monitor.c:  monitor_ops.deprecated_xfer_memory = monitor_xfer_memory;
nto-procfs.c:  procfs_ops.deprecated_xfer_memory = procfs_xfer_memory;
procfs.c:  t->deprecated_xfer_memory = procfs_xfer_memory;
remote-m32r-sdi.c:  m32r_ops.deprecated_xfer_memory = m32r_xfer_memory;
remote-mips.c:  mips_ops.deprecated_xfer_memory = mips_xfer_memory;
remote-sim.c:  gdbsim_ops.deprecated_xfer_memory = gdbsim_xfer_inferior_memory;
windows-nat.c:  windows_ops.deprecated_xfer_memory = windows_xfer_memory;

-- 
Pedro Alves

Re: [RFC 2/2] Test entry values in trace frame

[Yao Qi]

On 08/21/2013 02:05 PM, Yao Qi wrote:
> I agree.  I opened PR15871http://sourceware.org/bugzilla/show_bug.cgi?id=15871
> 
> On the other hand, I kfail the test.

Pedro fixed this bug.  I update the patch to get rid of the kfail.

-- 
Yao (齐尧)

gdb/testsuite:

2013-08-23  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/entry-values.c (end): New
	(main): Call end.
	* gdb.trace/entry-values.exp: Load trace-support.exp.  Set
	tracepoint and collect data.  Test entry value is unavailable.
---
 gdb/testsuite/gdb.trace/entry-values.c   |    5 +++
 gdb/testsuite/gdb.trace/entry-values.exp |   47 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/entry-values.c b/gdb/testsuite/gdb.trace/entry-values.c
index 3f98615..e287203 100644
--- a/gdb/testsuite/gdb.trace/entry-values.c
+++ b/gdb/testsuite/gdb.trace/entry-values.c
@@ -32,6 +32,10 @@ bar (int i)
 int global1 = 1;
 int global2 = 2;
 
+static void
+end (void)
+{}
+
 int
 main (void)
 {
@@ -41,5 +45,6 @@ main (void)
   global2++;
   ret = bar (0);
 
+  end ();
   return ret;
 }
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
index 0fb060a..bb62e5d 100644
--- a/gdb/testsuite/gdb.trace/entry-values.exp
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -230,3 +230,50 @@ gdb_test_sequence "bt" "bt (2)" {
     "\[\r\n\]#1 .* bar \\(i=<optimized out>, i@entry=<optimized out>\\)"
     "\[\r\n\]#2 .* main \\(\\)"
 }
+
+# Restart GDB and trace.
+
+clean_restart $binfile
+
+load_lib "trace-support.exp"
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+gdb_test "trace foo" "Tracepoint $decimal at .*"
+
+if [is_amd64_regs_target] {
+    set spreg "\$rsp"
+} elseif [is_x86_like_target] {
+    set spreg "\$esp"
+} else {
+    set spreg "\$sp"
+}
+
+# Collect arguments i and j.  Collect 'global1' which is entry value
+# of argument i.  Don't collect 'global2' to test the entry value of
+# argument j.
+
+gdb_trace_setactions "set action for tracepoint 1" "" \
+    "collect i, j, global1, \(\*\(void \*\*\) \($spreg\)\) @ 64" "^$"
+
+gdb_test_no_output "tstart"
+
+gdb_breakpoint "end"
+gdb_continue_to_breakpoint "end"
+
+gdb_test_no_output "tstop"
+
+gdb_test "tfind" "Found trace frame 0, .*" "tfind start"
+
+# Since 'global2' is not collected, j@entry is expected to be 'unavailable'.
+gdb_test "bt 1" "#0 .* foo \\(i=\[-\]?$decimal, i@entry=2, j=\[-\]?$decimal, j@entry=<unavailable>\\).*"
+
+gdb_test "tfind" "Target failed to find requested trace frame\..*"
-- 
1.7.7.6

Re: [RFC 2/2] Test entry values in trace frame

[Pedro Alves]

On 08/23/2013 01:31 AM, Yao Qi wrote:
> On 08/21/2013 02:05 PM, Yao Qi wrote:
>> I agree.  I opened PR15871http://sourceware.org/bugzilla/show_bug.cgi?id=15871
>>
>> On the other hand, I kfail the test.
> 
> Pedro fixed this bug.  I update the patch to get rid of the kfail.

FAOD, this patch is fine with me, if fine with Tom.

-- 
Pedro Alves

Re: [RFC 2/2] Test entry values in trace frame

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 08/23/2013 01:31 AM, Yao Qi wrote:
>> On 08/21/2013 02:05 PM, Yao Qi wrote:
>>> I agree.  I opened
>>> PR15871http://sourceware.org/bugzilla/show_bug.cgi?id=15871
>>> 
>>> On the other hand, I kfail the test.
>> 
>> Pedro fixed this bug.  I update the patch to get rid of the kfail.

Pedro> FAOD, this patch is fine with me, if fine with Tom.

It's ok with me, thanks.

Tom

Re: [RFC 2/2] Test entry values in trace frame

[Yao Qi]

On 08/24/2013 03:16 AM, Tom Tromey wrote:
> Pedro> FAOD, this patch is fine with me, if fine with Tom.
>
> It's ok with me, thanks.

Thanks for the review.  Patch is committed.

-- 
Yao (齐尧)