[PATCH 0/3] dprintf tweaks

[PATCH 0/3] dprintf tweaks

[Yao Qi]

Hi,
I happen to see that a block of code about dprintf in
print_one_breakpoint_location is dead, and patch 2/3 is
to remove the dead block.  Patch 1/3 is to test the output
of 'info breakpoints' for dprintf in different dprint-style.

The whole series are tested on x86_linux with native and
gdbserver board file.

  Test of breakpoint output for dprintf
  Remove dead code for dprintf.
  Assert 'printf_line' is non-null.

 gdb/breakpoint.c                               |   49 ++++++++++--------------
 gdb/testsuite/gdb.base/dprintf.exp             |   13 ++++++
 gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp |    2 +-
 3 files changed, 34 insertions(+), 30 deletions(-)

-- 
1.7.7.6

[PATCH 1/3] Test of breakpoint output for dprintf

[Yao Qi]

Hi,
We don't have a test to check the output of 'info breakpoints' and fields
in "=breakpoint-created" notificiation, which are all related to
'print_one_breakpoint_location'.  This patch adds tests for dprintf.
Is it OK?

gdb/testsuite:

2012-12-12  Yao Qi  <yao@codesourcery.com>

	* gdb.base/dprintf.exp: Check the output of 'info breakpoints' for
	dprintf.
	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
	Check the fields in "=breakpoint-created" for dprintf.
---
 gdb/testsuite/gdb.base/dprintf.exp             |   13 +++++++++++++
 gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp |    2 +-
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/gdb/testsuite/gdb.base/dprintf.exp b/gdb/testsuite/gdb.base/dprintf.exp
index bd0615b..979ceaf 100644
--- a/gdb/testsuite/gdb.base/dprintf.exp
+++ b/gdb/testsuite/gdb.base/dprintf.exp
@@ -43,6 +43,13 @@ gdb_test "dprintf foo,\"At foo entry\\n\"" \
 gdb_test "dprintf $dp_location1,\"arg=%d, g=%d\\n\", arg, g" \
   "Dprintf .*"
 
+gdb_test "info breakpoints" "3\[\t \]+dprintf .*
+\[\t \]+printf \"At foo entry\\\\n\".
+\[\t \]+continue.
+4\[\t \]+dprintf .*
+\[\t \]+printf \"arg=%d, g=%d\\\\n\", arg, g.
+\[\t \]+continue." "dprintf info 1"
+
 gdb_test "break $bp_location1" \
   "Breakpoint .*"
 
@@ -107,6 +114,12 @@ if $target_can_dprintf {
 
     gdb_test "continue" "Breakpoint \[0-9\]+, foo .*" "2nd dprintf, agent"
 
+    gdb_test "info breakpoints" "3\[\t \]+dprintf .*
+\[\t \]+breakpoint already hit 2 times.
+\[\t \]+agent-printf \"At foo entry\\\\n\".
+4\[\t \]+dprintf .*
+\[\t \]+breakpoint already hit 2 times.
+\[\t \]+agent-printf \"arg=%d, g=%d\\\\n\", arg, g.*" "info dprintf 2"
 }
 
 gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index ec10032..df6101c 100644
--- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
@@ -96,7 +96,7 @@ proc test_insert_delete_modify { } {
 	$test
     set test "dprintf marker, \"arg\" \""
     mi_gdb_test $test \
-	{.*=breakpoint-created,bkpt=\{number="6",type="dprintf".*\}.*\n\^done} \
+	{.*=breakpoint-created,bkpt=\{number="6",type="dprintf".*,script=\{\"printf \\\\\"arg\\\\\" \\\\\"\",\"continue\"\}.*\}\r\n\^done} \
 	$test
 
     # 2. when modifying condition
-- 
1.7.7.6

Re: [PATCH 1/3] Test of breakpoint output for dprintf

[Tom Tromey]

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

Yao> We don't have a test to check the output of 'info breakpoints' and fields
Yao> in "=breakpoint-created" notificiation, which are all related to
Yao> 'print_one_breakpoint_location'.  This patch adds tests for dprintf.
Yao> Is it OK?

Yao> +gdb_test "info breakpoints" "3\[\t \]+dprintf .*
Yao> +\[\t \]+printf \"At foo entry\\\\n\".
Yao> +\[\t \]+continue.
Yao> +4\[\t \]+dprintf .*
Yao> +\[\t \]+printf \"arg=%d, g=%d\\\\n\", arg, g.
Yao> +\[\t \]+continue." "dprintf info 1"

I find this style of test pretty hard to read.
How about using "\n" instead of a newline?
Or writing the string some other way to make it more readable?

Or perhaps this is just a personal idiosyncracy of mine.
I don't know.  If others are ok with this, I don't mind.

Otherwise the patch looks fine.

Tom

Re: [PATCH 1/3] Test of breakpoint output for dprintf

[Joel Brobecker]

> Yao> +gdb_test "info breakpoints" "3\[\t \]+dprintf .*
> Yao> +\[\t \]+printf \"At foo entry\\\\n\".
> Yao> +\[\t \]+continue.
> Yao> +4\[\t \]+dprintf .*
> Yao> +\[\t \]+printf \"arg=%d, g=%d\\\\n\", arg, g.
> Yao> +\[\t \]+continue." "dprintf info 1"
> 
> I find this style of test pretty hard to read.
> How about using "\n" instead of a newline?
> Or writing the string some other way to make it more readable?

FWIW, I sometimes do the following when writing tests:

gdb_test "info tasks" \
         [join {" +ID +TID P-ID Pri State +Name" \
                " +1 .* main_task" \
                " +2 .* task_list\\(1\\)" \
                "\\* +3 .* task_list\\(2\\)" \
                " +4 .* task_list\\(3\\)"} \
               "\r\n"] \
         "info tasks after hitting breakpoint"

I find that having a multi-line expected output shown as multiple
lines is easier to understand than one ginormous string...

-- 
Joel

Re: [PATCH 1/3] Test of breakpoint output for dprintf

[Yao Qi]

On 01/10/2013 01:43 PM, Joel Brobecker wrote:
>> Yao> +gdb_test "info breakpoints" "3\[\t \]+dprintf .*
>> Yao> +\[\t \]+printf \"At foo entry\\\\n\".
>> Yao> +\[\t \]+continue.
>> Yao> +4\[\t \]+dprintf .*
>> Yao> +\[\t \]+printf \"arg=%d, g=%d\\\\n\", arg, g.
>> Yao> +\[\t \]+continue." "dprintf info 1"
>>
>> I find this style of test pretty hard to read.
>> How about using "\n" instead of a newline?
>> Or writing the string some other way to make it more readable?
> 
> FWIW, I sometimes do the following when writing tests:
> 
> gdb_test "info tasks" \
>           [join {" +ID +TID P-ID Pri State +Name" \
>                  " +1 .* main_task" \
>                  " +2 .* task_list\\(1\\)" \
>                  "\\* +3 .* task_list\\(2\\)" \
>                  " +4 .* task_list\\(3\\)"} \
>                 "\r\n"] \
>           "info tasks after hitting breakpoint"
> 
> I find that having a multi-line expected output shown as multiple
> lines is easier to understand than one ginormous string...
> 

I agree, and that is why I break pattern into several lines.  I use
gdb_test_sequence in the new version of patch, which looks better.

    gdb_test_sequence "info breakpoints" "dprintf info 2" {
	"\[\r\n\]Num     Type           Disp Enb Address    What"
	"\[\r\n\]2       breakpoint"
	"\[\r\n\]\tbreakpoint already hit 2 times"
	"\[\r\n\]3       dprintf"
	"\[\r\n\]\tbreakpoint already hit 2 times"
	"\[\r\n\]        agent-printf \"At foo entry\\\\n\""
	"\[\r\n\]4       dprintf"
	"\[\r\n\]\tbreakpoint already hit 2 times"
	"\[\r\n\]        agent-printf \"arg=%d, g=%d\\\\n\", arg, g"
    }

How about the new version?

-- 
Yao (齐尧)

gdb/testsuite:

2013-01-10  Yao Qi  <yao@codesourcery.com>

	* gdb.base/dprintf.exp: Check the output of 'info breakpoints'
	for dprintf.
	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
	Check the fields in "=breakpoint-created" for dprintf.
---
 gdb/testsuite/gdb.base/dprintf.exp             |   22 ++++++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp |    2 +-
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/gdb/testsuite/gdb.base/dprintf.exp b/gdb/testsuite/gdb.base/dprintf.exp
index f99d75e..fb626fa 100644
--- a/gdb/testsuite/gdb.base/dprintf.exp
+++ b/gdb/testsuite/gdb.base/dprintf.exp
@@ -43,6 +43,17 @@ gdb_test "dprintf foo,\"At foo entry\\n\"" \
 gdb_test "dprintf $dp_location1,\"arg=%d, g=%d\\n\", arg, g" \
   "Dprintf .*"
 
+gdb_test_sequence "info breakpoints" "dprintf info 1" {
+    "\[\r\n\]Num     Type           Disp Enb Address    What"
+    "\[\r\n\]2       breakpoint"
+    "\[\r\n\]3       dprintf"
+    "\[\r\n\]        printf \"At foo entry\\\\n\""
+    "\[\r\n\]        continue"
+    "\[\r\n\]4       dprintf"
+    "\[\r\n\]        printf \"arg=%d, g=%d\\\\n\", arg, g"
+    "\[\r\n\]        continue"
+}
+
 gdb_test "break $bp_location1" \
   "Breakpoint .*"
 
@@ -107,6 +118,17 @@ if $target_can_dprintf {
 
     gdb_test "continue" "Breakpoint \[0-9\]+, foo .*" "2nd dprintf, agent"
 
+    gdb_test_sequence "info breakpoints" "dprintf info 2" {
+	"\[\r\n\]Num     Type           Disp Enb Address    What"
+	"\[\r\n\]2       breakpoint"
+	"\[\r\n\]\tbreakpoint already hit 2 times"
+	"\[\r\n\]3       dprintf"
+	"\[\r\n\]\tbreakpoint already hit 2 times"
+	"\[\r\n\]        agent-printf \"At foo entry\\\\n\""
+	"\[\r\n\]4       dprintf"
+	"\[\r\n\]\tbreakpoint already hit 2 times"
+	"\[\r\n\]        agent-printf \"arg=%d, g=%d\\\\n\", arg, g"
+    }
 }
 
 gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index 7ce314d..fd32698 100644
--- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
@@ -96,7 +96,7 @@ proc test_insert_delete_modify { } {
 	$test
     set test "dprintf marker, \"arg\" \""
     mi_gdb_test $test \
-	{.*=breakpoint-created,bkpt=\{number="6",type="dprintf".*\}.*\n\^done} \
+	{.*=breakpoint-created,bkpt=\{number="6",type="dprintf".*,script=\{\"printf \\\\\"arg\\\\\" \\\\\"\",\"continue\"\}.*\}\r\n\^done} \
 	$test
 
     # 2. when modifying condition
-- 
1.7.7.6

Re: [PATCH 1/3] Test of breakpoint output for dprintf

[Tom Tromey]

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

Yao> I agree, and that is why I break pattern into several lines.  I use
Yao> gdb_test_sequence in the new version of patch, which looks better.

Nice, I didn't know about this proc.
I like the result.

Yao> How about the new version?

Thank you for doing this.
The patch is ok.

Tom

[obv] Fix new FAIL on 64-bit targets [Re: [PATCH 1/3] Test of breakpoint output for dprintf]

[Jan Kratochvil]

On Thu, 10 Jan 2013 15:53:01 +0100, Yao Qi wrote:
> 	* gdb.base/dprintf.exp: Check the output of 'info breakpoints'
> 	for dprintf.
> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
> 	Check the fields in "=breakpoint-created" for dprintf.

It has FAIL on 64-bit targets:

gdb_expect_list pattern: /[^M
]Num     Type           Disp Enb Address    What/
info breakpoints^M
Num     Type           Disp Enb Address            What^M
2       breakpoint     keep y   0x0000000000400743 in main at ./gdb.base/dprintf.c:33^M
[...]
(gdb) FAIL: gdb.base/dprintf.exp: dprintf info 1 (pattern 1)


Checked in as obvious.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2013-01/msg00059.html

--- src/gdb/testsuite/ChangeLog	2013/01/11 00:31:56	1.3500
+++ src/gdb/testsuite/ChangeLog	2013/01/11 15:21:13	1.3501
@@ -1,3 +1,8 @@
+2013-01-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.base/dprintf.exp (dprintf info 1): Fix expectation on 64-bit
+	targets.
+
 2013-01-11  Yao Qi  <yao@codesourcery.com>
 
 	* gdb.base/dprintf.exp: Check the output of 'info breakpoints'
--- src/gdb/testsuite/gdb.base/dprintf.exp	2013/01/11 00:31:58	1.7
+++ src/gdb/testsuite/gdb.base/dprintf.exp	2013/01/11 15:21:14	1.8
@@ -44,7 +44,7 @@
   "Dprintf .*"
 
 gdb_test_sequence "info breakpoints" "dprintf info 1" {
-    "\[\r\n\]Num     Type           Disp Enb Address    What"
+    "\[\r\n\]Num     Type           Disp Enb Address +What"
     "\[\r\n\]2       breakpoint"
     "\[\r\n\]3       dprintf"
     "\[\r\n\]        printf \"At foo entry\\\\n\""

[PATCH 3/3] Assert 'printf_line' is non-null.

[Yao Qi]

As we can see the code above, printf_line is always assigned to
a non-null string, so this patch removes the check on
'printf_line' and add an assert.

On the other hand, 'breakpoint_set_commands' will be called
unconditionally, and B->commands is non-null (B is a dprintf).

gdb:

2012-12-12  Yao Qi  <yao@codesourcery.com>

	* breakpoint.c (update_dprintf_command_list): Assert that
	'printf_line' is non-null.  Remove condition check.
---
 gdb/breakpoint.c |   40 ++++++++++++++++++++--------------------
 1 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 65bdc2c..ab056d4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8865,30 +8865,30 @@ update_dprintf_command_list (struct breakpoint *b)
     internal_error (__FILE__, __LINE__,
 		    _("Invalid dprintf style."));
 
+  gdb_assert (printf_line != NULL);
   /* Manufacture a printf/continue sequence.  */
-  if (printf_line)
-    {
-      struct command_line *printf_cmd_line, *cont_cmd_line = NULL;
+  {
+    struct command_line *printf_cmd_line, *cont_cmd_line = NULL;
 
-      if (strcmp (dprintf_style, dprintf_style_agent) != 0)
-	{
-	  cont_cmd_line = xmalloc (sizeof (struct command_line));
-	  cont_cmd_line->control_type = simple_control;
-	  cont_cmd_line->body_count = 0;
-	  cont_cmd_line->body_list = NULL;
-	  cont_cmd_line->next = NULL;
-	  cont_cmd_line->line = xstrdup ("continue");
-	}
+    if (strcmp (dprintf_style, dprintf_style_agent) != 0)
+      {
+	cont_cmd_line = xmalloc (sizeof (struct command_line));
+	cont_cmd_line->control_type = simple_control;
+	cont_cmd_line->body_count = 0;
+	cont_cmd_line->body_list = NULL;
+	cont_cmd_line->next = NULL;
+	cont_cmd_line->line = xstrdup ("continue");
+      }
 
-      printf_cmd_line = xmalloc (sizeof (struct command_line));
-      printf_cmd_line->control_type = simple_control;
-      printf_cmd_line->body_count = 0;
-      printf_cmd_line->body_list = NULL;
-      printf_cmd_line->next = cont_cmd_line;
-      printf_cmd_line->line = printf_line;
+    printf_cmd_line = xmalloc (sizeof (struct command_line));
+    printf_cmd_line->control_type = simple_control;
+    printf_cmd_line->body_count = 0;
+    printf_cmd_line->body_list = NULL;
+    printf_cmd_line->next = cont_cmd_line;
+    printf_cmd_line->line = printf_line;
 
-      breakpoint_set_commands (b, printf_cmd_line);
-    }
+    breakpoint_set_commands (b, printf_cmd_line);
+  }
 }
 
 /* Update all dprintf commands, making their command lists reflect
-- 
1.7.7.6

Re: [PATCH 3/3] Assert 'printf_line' is non-null.

[Tom Tromey]

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

Yao> 2012-12-12  Yao Qi  <yao@codesourcery.com>
Yao> 	* breakpoint.c (update_dprintf_command_list): Assert that
Yao> 	'printf_line' is non-null.  Remove condition check.

Ok.

Tom

[PATCH 2/3] Remove dead code for dprintf.

[Yao Qi]

Hi,
For a given dprintf B, B->commands can never be null, so the condition
below is always false.  This patch is to remove it as sort of dead
code.

When creating a dprintf B, the following call chain can be found
unconditionally,

init_breakpoint_sal
  --> update_dprintf_command_list (because B->extra_string can't be null)
         --> breakpoint_set_commands (because PRINT_LINE can't be null)
                --> set B->commands.
gdb:

2012-12-12  Yao Qi  <yao@codesourcery.com>

	* breakpoint.c (print_one_breakpoint_location): Remove dead code.
---
 gdb/breakpoint.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e1da50b..65bdc2c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6091,15 +6091,6 @@ print_one_breakpoint_location (struct breakpoint *b,
 	  ui_out_text (uiout, " bytes\n");
 	}
     }
-  
-  if (!part_of_multiple && b->extra_string
-      && b->type == bp_dprintf && !b->commands)
-    {
-      annotate_field (7);
-      ui_out_text (uiout, "\t(agent printf) ");
-      ui_out_field_string (uiout, "printf", b->extra_string);
-      ui_out_text (uiout, "\n");
-    }
 
   l = b->commands ? b->commands->commands : NULL;
   if (!part_of_multiple && l)
-- 
1.7.7.6

Re: [PATCH 2/3] Remove dead code for dprintf.

[Tom Tromey]

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

Yao> 2012-12-12  Yao Qi  <yao@codesourcery.com>
Yao> 	* breakpoint.c (print_one_breakpoint_location): Remove dead code.

Looks good to me.

Tom

[ping]: [PATCH 0/3] dprintf tweaks

[Yao Qi]

On 12/12/2012 12:12 PM, Yao Qi wrote:
> I happen to see that a block of code about dprintf in
> print_one_breakpoint_location is dead, and patch 2/3 is
> to remove the dead block.  Patch 1/3 is to test the output
> of 'info breakpoints' for dprintf in different dprint-style.
>
> The whole series are tested on x86_linux with native and
> gdbserver board file.

Ping.  http://sourceware.org/ml/gdb-patches/2012-12/msg00373.html

-- 
Yao (齐尧)