[PATCH 0/3] Add tesecases for examine/display/list

[PATCH 0/3] Add tesecases for examine/display/list

[Fei Jie]

These patches add testcases to test examine/display/list under different conditions:
	*examine address in different data types
	*display data in different types, enable/disable display
	*list with more options

Fei Jie (3):
  Add testcases for examine function
  Add testcases for display function
  Add testcases for list function

 gdb/testsuite/gdb.base/display.c       |  5 ++++
 gdb/testsuite/gdb.base/list.exp        | 48 ++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/testdisplay.exp | 35 +++++++++++++++++++++++++
 gdb/testsuite/gdb.base/testexamine.exp | 42 +++++++++++++++++++++++++++++
 4 files changed, 130 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/testdisplay.exp
 create mode 100644 gdb/testsuite/gdb.base/testexamine.exp

-- 
1.8.3.1

[PATCH 2/3] Add testcases for display function

[Fei Jie]

    add testcases to test display under different conditions
---
 gdb/testsuite/gdb.base/display.c       |  5 +++++
 gdb/testsuite/gdb.base/testdisplay.exp | 35 ++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/testdisplay.exp

diff --git a/gdb/testsuite/gdb.base/display.c b/gdb/testsuite/gdb.base/display.c
index cd833e2..6db289c 100644
--- a/gdb/testsuite/gdb.base/display.c
+++ b/gdb/testsuite/gdb.base/display.c
@@ -4,6 +4,11 @@
 #define LOOP 10
 
 int sum = 0;
+int int_array[2]={0,1};
+struct {
+	char name;
+	int age;
+} human;
 
 /* Call to force a variable onto the stack so we can see its address.  */
 void force_mem (int *arg) { }
diff --git a/gdb/testsuite/gdb.base/testdisplay.exp b/gdb/testsuite/gdb.base/testdisplay.exp
new file mode 100644
index 0000000..984465e2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/testdisplay.exp
@@ -0,0 +1,35 @@
+set srcfile display.c
+
+if { [prepare_for_testing testdisplay.exp "testdisplay" display.c {debug nowarnings}] } {
+	untest testdisplay.exp
+	return -1
+}
+
+set bp_location [gdb_get_line_number "set breakpoint 1 here"]
+send_gdb "break $bp_location\n"
+send_gdb "run\n"
+
+#Test disp(display)
+gdb_test "display f" "1: f = 3.1415"
+gdb_test "display/x f" "2: /x f = 0x3"
+gdb_test "display/d f" "3: /d f = 3"
+gdb_test "display/u f" "4: /u f = 3"
+gdb_test "display/o f" "5: /o f = 03"
+gdb_test "display/t f" "6: /t f = 11"
+gdb_test "display/a f" "7: /a f = 0x3"
+gdb_test "display/c f" "8: /c f = 3\ \'\\\\003\'"
+gdb_test "display/f f" "9: /f f = 3.1415"
+
+gdb_test "display int_array" \
+	"10: int_array = \\{0, 1\\}"
+gdb_test "display human" \
+	"11: human = {name = 0 '\\\\000', age = 0}"
+
+#Test disable/enable display
+gdb_test "disable display 999" \
+	"No display number 999\."
+gdb_test_no_output "disable display 9"
+gdb_test_no_output "enable display 9"
+
+gdb_exit
+return 0
-- 
1.8.3.1

Re: [PATCH 2/3] Add testcases for display function

[Joel Brobecker]

>     add testcases to test display under different conditions

Same kind of procedural issues as before, so I'm not going to comment
on those again.

> --- a/gdb/testsuite/gdb.base/display.c
> +++ b/gdb/testsuite/gdb.base/display.c
> @@ -4,6 +4,11 @@
>  #define LOOP 10
>  
>  int sum = 0;
> +int int_array[2]={0,1};

The formatting does not follow the GCS:
  - spaces around binary operators;
  - space after comma.
Thus:

int int_array[2] = {0, 1};

> +struct {
> +	char name;
> +	int age;
> +} human;

GCS issues:
  - the opening curly braces should be on the next line. 
  - the indentation level should be 2 characters.
Thus:

struct
  {
    char name;
    int age;
  } human;

> +set bp_location [gdb_get_line_number "set breakpoint 1 here"]
> +send_gdb "break $bp_location\n"
> +send_gdb "run\n"

Do not use send_gdb unless you absolutely have no other choice.
Take a look at the gdb testsuite cookbook for how to insert breakpoints
and run to them.

Also, this might no longer become relevant, but it's better to never
test the "run" command directly, since there are targets where this
is not the appropriate thing to do (Eg: when using gdbserver, we do
"set target remote ..." followed by "continue" instead).

But why not enhance display.exp instead of creating a new one?

-- 
Joel

[PATCH 3/3] Add testcases for list function

[Fei Jie]

    add testcases to list.exp, test list under more conditions
---
 gdb/testsuite/gdb.base/list.exp | 48 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/gdb/testsuite/gdb.base/list.exp b/gdb/testsuite/gdb.base/list.exp
index 2aea9a3..cf06cd5 100644
--- a/gdb/testsuite/gdb.base/list.exp
+++ b/gdb/testsuite/gdb.base/list.exp
@@ -624,4 +624,52 @@ test_list "list -" 10 2 "7-8" "5-6"
 # the current line.
 test_list "list -" 10 1 "7" "6"
 
+#Get main address
+set main_addr ""
+gdb_test_multiple "print/x &main" "getting main's address" {
+    -re "$decimal = \($hex\)\r\n$gdb_prompt $" {
+        set main_addr $expect_out(1,string)
+    }
+}
+send_gdb "set listsize 10\n"
+#Test list with line number
+gdb_test "list 12" \
+    "7\\s+x = 0.*16\\s+foo \\(x\\+\\+\\);"
+
+#Test list with +
+gdb_test "list +" \
+    "17\\s+foo \\(x\\+\\+\\).*26\\s+foo \\(x\\+\\+\\);"
+
+#Test list with '+' line number
+gdb_test "list +1" \
+    "23\\s+foo \\(x\\+\\+\\).*32\\s+foo \\(x\\+\\+\\);"
+
+#Test list with starting line number and ','
+gdb_test "list 20," \
+    "20\\s+foo \\(x\\+\\+\\).*29\\s+foo \\(x\\+\\+\\);"
+
+#Test list with ',' and ending line number
+gdb_test "list ,25" \
+    "16\\s+foo \\(x\\+\\+\\).*25\\s+foo \\(x\\+\\+\\);"
+
+#Test list with address
+gdb_test "list *$main_addr" \
+    "$main_addr is in main.*list0\.c.*"
+
+#Test list with '-' max line number
+gdb_test "list -43" \
+    "1\\s+#include \"list0.h\".*10\\s+foo \\(x\\+\\+\\);"
+
+#Test list with starting line num and out-of-ranged ending line
+gdb_test " list 1,48" \
+    "1\\s+#include \"list0.h\".*43\\s+\\} \\/\\* last line \\*\\/"
+
+#Test list with ',' and out-of-ranged ending line
+gdb_test "list ,44" \
+    ".*43\\s+\\} \\/\\* last line \\*\\/"
+
+#Test list with ',' and out-of-ranged ending line(which is 10 more than max line number)
+gdb_test "list ,53" \
+    "Line number 44 out of range.*"
+
 remote_exec build "rm -f list0.h"
-- 
1.8.3.1

Re: [PATCH 3/3] Add testcases for list function

[Joel Brobecker]

>     add testcases to list.exp, test list under more conditions

Same general comments regarding our Coding Style and submission
guidelines, so I will let you update this patch accordingly.

Thank you,
-- 
Joel

[PATCH 1/3] Add testcases for examine function

[Fei Jie]

    use examine to display the content of main function's address
    in different formats and examine address that can not be accessed
---
 gdb/testsuite/gdb.base/testexamine.exp | 42 ++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/testexamine.exp

diff --git a/gdb/testsuite/gdb.base/testexamine.exp b/gdb/testsuite/gdb.base/testexamine.exp
new file mode 100644
index 0000000..16d916d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/testexamine.exp
@@ -0,0 +1,42 @@
+if {[prepare_for_testing testprint.exp testprint display.c \
+	{debug nowarnings}]} {
+	untested testprint.exp
+	return -1
+}
+
+if ![runto_main] then {
+	fail "Can not run to main."
+}
+
+#Get main address
+set main_addr ""
+gdb_test_multiple "print/x &main" "getting main's address" {
+    -re "$decimal = \($hex\)\r\n$gdb_prompt $" {
+        set main_addr $expect_out(1,string)
+    }
+}
+
+#Test x(examine)
+gdb_test "x" \
+	"Argument required.*"
+gdb_test "x/x $main_addr" \
+	".*<main>.*0xe5894855"
+gdb_test "x/d $main_addr" \
+	".*<main>.*-443987883"
+gdb_test "x/u $main_addr" \
+	".*<main>.*3850979413"
+gdb_test "x/o $main_addr" \
+	".*<main>.*034542244125"
+gdb_test "x/t $main_addr" \
+	".*<main>.*11100101100010010100100001010101"
+gdb_test "x/a $main_addr" \
+	".*<main>.*0xb8e5894855"
+gdb_test "x/c $main_addr" \
+	".*<main>.*85\ \'U\'"
+gdb_test "x/f $main_addr" \
+	".*<main>.*3.923498621684153e-312"
+gdb_test "x 0x000000000000" \
+	"Cannot access memory at address 0x0"
+
+gdb_exit
+return 0
-- 
1.8.3.1

Re: [PATCH 1/3] Add testcases for examine function

[Joel Brobecker]

On Wed, Nov 04, 2015 at 03:09:45PM +0800, Fei Jie wrote:
>     use examine to display the content of main function's address
>     in different formats and examine address that can not be accessed

As explained in our submission procedures, this change is missing
a ChangeLog entry.

> diff --git a/gdb/testsuite/gdb.base/testexamine.exp b/gdb/testsuite/gdb.base/testexamine.exp
> new file mode 100644
> index 0000000..16d916d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/testexamine.exp

*ALL* new files must have a copyright header. For .exp files, suggest
you just copy it from another file, and fix the copyright year as
appropriate (year range from when the file was first saved on
hard drive).

> @@ -0,0 +1,42 @@
> +if {[prepare_for_testing testprint.exp testprint display.c \
> +	{debug nowarnings}]} {
> +	untested testprint.exp
> +	return -1
> +
> +if ![runto_main] then {
> +	fail "Can not run to main."
> +}

As mentioned in a previous message, please take a look at how
we prefer tests to be built:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Building_the_Example_Program

In particular, use "standard_testfile", and lose the untested.
See also the block on we "runto_main" (we don't use "fail" in this
case).

> +#Get main address

We follow the GNU Coding Standards (GCS):
http://www.gnu.org/prep/standards/standards.html

So, could you please add a space after the "#", and also terminate
all sentences with a period?

# Get main's address.

> +set main_addr ""
> +gdb_test_multiple "print/x &main" "getting main's address" {
> +    -re "$decimal = \($hex\)\r\n$gdb_prompt $" {
> +        set main_addr $expect_out(1,string)
> +    }
> +}
> +
> +#Test x(examine)
> +gdb_test "x" \
> +	"Argument required.*"
> +gdb_test "x/x $main_addr" \
> +	".*<main>.*0xe5894855"

I am afraid that this isn't going to work for anyone by you,
and only when using your very specific compiler. You're expecting
the compiler to compile the code in such a way that the first
word of the function is 0xe5894855, which is not always true.

Better, IMO, to do this using a variable, either global or local,
whose content is well known.

> +gdb_test "x 0x000000000000" \
> +	"Cannot access memory at address 0x0"

This, unfortunately, is not guaranteed to work either. Some targets
allow reading at this address. I would just remove that test.

> +gdb_exit
> +return 0

This is also unnecessary.

-- 
Joel

Re: [PATCH 1/3] Add testcases for examine function

[Andrew Burgess]

* Fei Jie <feij.fnst@cn.fujitsu.com> [2015-11-04 15:09:45 +0800]:

>     use examine to display the content of main function's address
>     in different formats and examine address that can not be accessed
> ---
>  gdb/testsuite/gdb.base/testexamine.exp | 42 ++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/testexamine.exp
> 
> diff --git a/gdb/testsuite/gdb.base/testexamine.exp b/gdb/testsuite/gdb.base/testexamine.exp
> new file mode 100644
> index 0000000..16d916d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/testexamine.exp
> @@ -0,0 +1,42 @@
> +if {[prepare_for_testing testprint.exp testprint display.c \
> +	{debug nowarnings}]} {
> +	untested testprint.exp
> +	return -1
> +}
> +
> +if ![runto_main] then {
> +	fail "Can not run to main."
> +}
> +
> +#Get main address
> +set main_addr ""
> +gdb_test_multiple "print/x &main" "getting main's address" {
> +    -re "$decimal = \($hex\)\r\n$gdb_prompt $" {
> +        set main_addr $expect_out(1,string)
> +    }
> +}
> +
> +#Test x(examine)
> +gdb_test "x" \
> +	"Argument required.*"
> +gdb_test "x/x $main_addr" \
> +	".*<main>.*0xe5894855"

These addresses are not going to be correct for all the different
targets that gdb supports.  Or even a different version of GCC (or
whatever compiler) building the same target as you are.

Thanks,
Andrew


> +gdb_test "x/d $main_addr" \
> +	".*<main>.*-443987883"
> +gdb_test "x/u $main_addr" \
> +	".*<main>.*3850979413"
> +gdb_test "x/o $main_addr" \
> +	".*<main>.*034542244125"
> +gdb_test "x/t $main_addr" \
> +	".*<main>.*11100101100010010100100001010101"
> +gdb_test "x/a $main_addr" \
> +	".*<main>.*0xb8e5894855"
> +gdb_test "x/c $main_addr" \
> +	".*<main>.*85\ \'U\'"
> +gdb_test "x/f $main_addr" \
> +	".*<main>.*3.923498621684153e-312"
> +gdb_test "x 0x000000000000" \
> +	"Cannot access memory at address 0x0"
> +
> +gdb_exit
> +return 0
> -- 
> 1.8.3.1
> 

Re: [PATCH 0/3] Add tesecases for examine/display/list

[Joel Brobecker]

> These patches add testcases to test examine/display/list under different conditions:
> 	*examine address in different data types
> 	*display data in different types, enable/disable display
> 	*list with more options

Thanks for providing new testcases, we can always use additional
testing!

First, a few comments, before I delve into reviewing the patches?

  (1) The GDB project requires that the copyright to all changes
      integrated by assigned to the FSF. Do you have a copyright
      assignment on file with the FSF? I tried looking at the current
      records, and did not find you. If you don't, then let me know,
      and we'll start the paper work.

  (2) I noticed that your emails were not following some of the guidelines
      we have for patch submission. The guidelines should all be
      explained at this page:

      https://sourceware.org/gdb/wiki/ContributionChecklist

  (3) For testcases, there is a "cookbook" that provides best practices.

      https://sourceware.org/gdb/wiki/GDBTestcaseCookbook

-- 
Joel

Re: [PATCH 0/3] Add tesecases for examine/display/list

[fj]

On 11/05/2015 05:21 AM, Joel Brobecker wrote:
>> These patches add testcases to test examine/display/list under different conditions:
>> 	*examine address in different data types
>> 	*display data in different types, enable/disable display
>> 	*list with more options
>
> Thanks for providing new testcases, we can always use additional
> testing!
>
> First, a few comments, before I delve into reviewing the patches?
>
>    (1) The GDB project requires that the copyright to all changes
>        integrated by assigned to the FSF. Do you have a copyright
>        assignment on file with the FSF? I tried looking at the current
>        records, and did not find you. If you don't, then let me know,
>        and we'll start the paper work.
>
I haven't got a copyright assignment on file with the FSF yet.
>    (2) I noticed that your emails were not following some of the guidelines
>        we have for patch submission. The guidelines should all be
>        explained at this page:
>
>        https://sourceware.org/gdb/wiki/ContributionChecklist
>
>    (3) For testcases, there is a "cookbook" that provides best practices.
>
>        https://sourceware.org/gdb/wiki/GDBTestcaseCookbook
>
OK. I will remake my patches basing on these.

-- 
Thanks
Fei Jie

Re: [PATCH 0/3] Add tesecases for examine/display/list

[fj]

On 11/05/2015 05:21 AM, Joel Brobecker wrote:
>> These patches add testcases to test examine/display/list under different conditions:
>> 	*examine address in different data types
>> 	*display data in different types, enable/disable display
>> 	*list with more options
>
> Thanks for providing new testcases, we can always use additional
> testing!
>
> First, a few comments, before I delve into reviewing the patches?
>
>    (1) The GDB project requires that the copyright to all changes
>        integrated by assigned to the FSF. Do you have a copyright
>        assignment on file with the FSF? I tried looking at the current
>        records, and did not find you. If you don't, then let me know,
>        and we'll start the paper work.
>

Hello, I haven't got that and could you tell me how to start that?

>    (2) I noticed that your emails were not following some of the guidelines
>        we have for patch submission. The guidelines should all be
>        explained at this page:
>
>        https://sourceware.org/gdb/wiki/ContributionChecklist
>
>    (3) For testcases, there is a "cookbook" that provides best practices.
>
>        https://sourceware.org/gdb/wiki/GDBTestcaseCookbook
>

-- 
Thanks
Fei Jie