[PATCH 0/2] Couple of small fixes to gdb.mi/mi-vla-fortran.exp test

[PATCH 0/2] Couple of small fixes to gdb.mi/mi-vla-fortran.exp test

[Andrew Burgess]

Couple of minor fixes for gdb.mi/mi-vla-fortran.exp test.

Andrew Burgess (2):
  gdb: Fix parameter passing to mi_create_breakpoint
  gdb: Make some test names unique

 gdb/testsuite/ChangeLog                 |  9 ++++++
 gdb/testsuite/gdb.mi/mi-vla-fortran.exp | 50 ++++++++++++++++++---------------
 2 files changed, 37 insertions(+), 22 deletions(-)

-- 
2.5.1

[PATCH 1/2] gdb: Fix parameter passing to mi_create_breakpoint

[Andrew Burgess]

In the test gdb.mi/mi-vla-fortran.exp the parameters passed to
mi_create_breakpoint are passed in the wrong order.  By good luck the
tests still passes, however the wrong test name is used.  All fixed in
this commit.

gdb/testsuite/ChangeLog:

	* gdb.mi/mi-vla-fortran.exp: Correct parameter passing to
	mi_create_breakpoint.
---
 gdb/testsuite/ChangeLog                 |  5 +++++
 gdb/testsuite/gdb.mi/mi-vla-fortran.exp | 36 +++++++++++++++++++--------------
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-vla-fortran.exp b/gdb/testsuite/gdb.mi/mi-vla-fortran.exp
index b81165e..7b40aba 100644
--- a/gdb/testsuite/gdb.mi/mi-vla-fortran.exp
+++ b/gdb/testsuite/gdb.mi/mi-vla-fortran.exp
@@ -37,9 +37,9 @@ mi_gdb_reinitialize_dir $srcdir/$subdir
 mi_gdb_load ${binfile}
 
 set bp_lineno [gdb_get_line_number "vla1-not-allocated"]
-mi_create_breakpoint "-t vla.f90:$bp_lineno" 1 "del" "vla" \
-  ".*vla.f90" $bp_lineno $hex \
-  "insert breakpoint at line $bp_lineno (vla not allocated)"
+mi_create_breakpoint "-t vla.f90:$bp_lineno" \
+    "insert breakpoint at line $bp_lineno (vla not allocated)" \
+    -number 1 -disp del -func vla ".*vla.f90" $bp_lineno $hex
 mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
@@ -63,8 +63,9 @@ mi_list_array_varobj_children_with_index "vla1_not_allocated" "0" "1" \
 
 
 set bp_lineno [gdb_get_line_number "vla1-allocated"]
-mi_create_breakpoint "-t vla.f90:$bp_lineno" 2 "del" "vla" ".*vla.f90" \
-  $bp_lineno $hex "insert breakpoint at line $bp_lineno (vla allocated)"
+mi_create_breakpoint "-t vla.f90:$bp_lineno" \
+    "insert breakpoint at line $bp_lineno (vla allocated)" \
+    -number 2 -disp del -func vla ".*vla.f90" $bp_lineno $hex
 mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
@@ -87,8 +88,9 @@ mi_list_array_varobj_children_with_index "vla1_allocated" "5" "1" \
 
 
 set bp_lineno [gdb_get_line_number "vla1-filled"]
-mi_create_breakpoint "-t vla.f90:$bp_lineno" 3 "del" "vla" ".*vla.f90" \
-  $bp_lineno $hex "insert breakpoint at line $bp_lineno"
+mi_create_breakpoint "-t vla.f90:$bp_lineno" \
+    "insert breakpoint at line $bp_lineno" \
+    -number 3 -disp del -func vla ".*vla.f90" $bp_lineno $hex
 mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
@@ -97,8 +99,9 @@ mi_gdb_test "520-data-evaluate-expression vla1" \
 
 
 set bp_lineno [gdb_get_line_number "vla1-modified"]
-mi_create_breakpoint "-t vla.f90:$bp_lineno" 4 "del" "vla" ".*vla.f90" \
-  $bp_lineno $hex "insert breakpoint at line $bp_lineno"
+mi_create_breakpoint "-t vla.f90:$bp_lineno" \
+    "insert breakpoint at line $bp_lineno" \
+    -number 4 -disp del -func vla ".*vla.f90" $bp_lineno $hex
 mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
@@ -113,8 +116,9 @@ mi_gdb_test "560-data-evaluate-expression vla1(4)" \
 
 
 set bp_lineno [gdb_get_line_number "vla1-deallocated"]
-mi_create_breakpoint "-t vla.f90:$bp_lineno" 5 "del" "vla" ".*vla.f90" \
-  $bp_lineno $hex "insert breakpoint at line $bp_lineno"
+mi_create_breakpoint "-t vla.f90:$bp_lineno" \
+    "insert breakpoint at line $bp_lineno" \
+    -number 5 -disp del -func vla ".*vla.f90" $bp_lineno $hex
 mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
@@ -163,8 +167,9 @@ gdb_expect {
 }
 
 set bp_lineno [gdb_get_line_number "pvla2-associated"]
-mi_create_breakpoint "-t vla.f90:$bp_lineno" 7 "del" "vla" ".*vla.f90" \
-  $bp_lineno $hex "insert breakpoint at line $bp_lineno"
+mi_create_breakpoint "-t vla.f90:$bp_lineno" \
+    "insert breakpoint at line $bp_lineno" \
+    -number 7 -disp del -func vla ".*vla.f90" $bp_lineno $hex
 mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
@@ -186,8 +191,9 @@ mi_gdb_test "593-var-evaluate-expression pvla2_associated" \
 
 
 set bp_lineno [gdb_get_line_number "pvla2-set-to-null"]
-mi_create_breakpoint "-t vla.f90:$bp_lineno" 8 "del" "vla" ".*vla.f90" \
-  $bp_lineno $hex "insert breakpoint at line $bp_lineno"
+mi_create_breakpoint "-t vla.f90:$bp_lineno" \
+    "insert breakpoint at line $bp_lineno" \
+    -number 8 -disp del -func vla ".*vla.f90" $bp_lineno $hex
 mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
-- 
2.5.1

Re: [PATCH 1/2] gdb: Fix parameter passing to mi_create_breakpoint

[Yao Qi]

Andrew Burgess <andrew.burgess@embecosm.com> writes:

> gdb/testsuite/ChangeLog:
>
> 	* gdb.mi/mi-vla-fortran.exp: Correct parameter passing to
> 	mi_create_breakpoint.

Patch is good to me.

-- 
Yao (齐尧)

[PATCH 2/2] gdb: Make some test names unique

[Andrew Burgess]

Make sure all of the tests have unique names in
gdb.mi/mi-vla-fortran.exp.

gdb/testsuite/ChangeLog:

	* gdb.mi/mi-vla-fortran.exp: Make test names unique.
---
 gdb/testsuite/ChangeLog                 |  4 ++++
 gdb/testsuite/gdb.mi/mi-vla-fortran.exp | 14 +++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-vla-fortran.exp b/gdb/testsuite/gdb.mi/mi-vla-fortran.exp
index 7b40aba..22e2bce 100644
--- a/gdb/testsuite/gdb.mi/mi-vla-fortran.exp
+++ b/gdb/testsuite/gdb.mi/mi-vla-fortran.exp
@@ -44,7 +44,7 @@ mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
 mi_gdb_test "500-data-evaluate-expression vla1" \
-  "500\\^done,value=\"<not allocated>\"" "evaluate not allocated vla"
+  "500\\^done,value=\"<not allocated>\"" "evaluate not allocated vla at line $bp_lineno"
 
 mi_create_varobj_checked vla1_not_allocated vla1 "<not allocated>" \
   "create local variable vla1_not_allocated"
@@ -95,7 +95,7 @@ mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
 mi_gdb_test "520-data-evaluate-expression vla1" \
-  "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla"
+  "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla at line $bp_lineno"
 
 
 set bp_lineno [gdb_get_line_number "vla1-modified"]
@@ -106,13 +106,13 @@ mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
 mi_gdb_test "530-data-evaluate-expression vla1" \
-  "530\\^done,value=\"\\(1, 42, 1, 24, 1\\)\"" "evaluate filled vla"
+  "530\\^done,value=\"\\(1, 42, 1, 24, 1\\)\"" "evaluate filled vla at line $bp_lineno"
 mi_gdb_test "540-data-evaluate-expression vla1(1)" \
-  "540\\^done,value=\"1\"" "evaluate filled vla"
+  "540\\^done,value=\"1\"" "evaluate filled vla(1)"
 mi_gdb_test "550-data-evaluate-expression vla1(2)" \
-  "550\\^done,value=\"42\"" "evaluate filled vla"
+  "550\\^done,value=\"42\"" "evaluate filled vla(2)"
 mi_gdb_test "560-data-evaluate-expression vla1(4)" \
-  "560\\^done,value=\"24\"" "evaluate filled vla"
+  "560\\^done,value=\"24\"" "evaluate filled vla(4)"
 
 
 set bp_lineno [gdb_get_line_number "vla1-deallocated"]
@@ -123,7 +123,7 @@ mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
 mi_gdb_test "570-data-evaluate-expression vla1" \
-  "570\\^done,value=\"<not allocated>\"" "evaluate not allocated vla"
+  "570\\^done,value=\"<not allocated>\"" "evaluate not allocated vla at line $bp_lineno"
 
 
 set bp_lineno [gdb_get_line_number "pvla2-not-associated"]
-- 
2.5.1

Re: [PATCH 2/2] gdb: Make some test names unique

[Yao Qi]

Andrew Burgess <andrew.burgess@embecosm.com> writes:


>  mi_gdb_test "540-data-evaluate-expression vla1(1)" \
> -  "540\\^done,value=\"1\"" "evaluate filled vla"
> +  "540\\^done,value=\"1\"" "evaluate filled vla(1)"

Do not use "tail parentheses" on the test message,
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

Maybe ""evaluate filled vla1 1"?

>  mi_gdb_test "550-data-evaluate-expression vla1(2)" \
> -  "550\\^done,value=\"42\"" "evaluate filled vla"
> +  "550\\^done,value=\"42\"" "evaluate filled vla(2)"
>  mi_gdb_test "560-data-evaluate-expression vla1(4)" \
> -  "560\\^done,value=\"24\"" "evaluate filled vla"
> +  "560\\^done,value=\"24\"" "evaluate filled vla(4)"

Otherwise, patch is good to me.

-- 
Yao (齐尧)

Re: [PATCH 2/2] gdb: Make some test names unique

[Pedro Alves]

On 06/22/2017 11:37 AM, Yao Qi wrote:
> Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> 
>>  mi_gdb_test "540-data-evaluate-expression vla1(1)" \
>> -  "540\\^done,value=\"1\"" "evaluate filled vla"
>> +  "540\\^done,value=\"1\"" "evaluate filled vla(1)"
> 
> Do not use "tail parentheses" on the test message,
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

The wiki doesn't mention it, but I think the rule should only apply when
there's a space before the parens.  Otherwise, we have a problem with
all the tests that call functions, and don't explicitly specify a test
name, like:

  gdb_test "p function(1)" " = 1"

We have many such cases, C tests, Python tests, etc:

 $ grep "[a-z](.*)$" testsuite/gdb.sum  | wc -l
 1174

I don't think it's worth it, or even a good idea to try to
come up with different test names for all of these.  In cases like
these, I think it's generally possible to avoid the space before
the parens.  So IMO, we should clarify the rule instead
(and the buildbot testresult diffing accordingly, if necessary).

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb: Make some test names unique

[Yao Qi]

On Thu, Jun 22, 2017 at 12:08 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/22/2017 11:37 AM, Yao Qi wrote:
>> Andrew Burgess <andrew.burgess@embecosm.com> writes:
>>
>>
>>>  mi_gdb_test "540-data-evaluate-expression vla1(1)" \
>>> -  "540\\^done,value=\"1\"" "evaluate filled vla"
>>> +  "540\\^done,value=\"1\"" "evaluate filled vla(1)"
>>
>> Do not use "tail parentheses" on the test message,
>> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages
>
> The wiki doesn't mention it, but I think the rule should only apply when
> there's a space before the parens.

I wanted to get confirm from analyze-racy-logs.py,
but I was lost in it, so I decided to follow the wiki.

Andrew,
your patch is OK as-is.

>
> I don't think it's worth it, or even a good idea to try to
> come up with different test names for all of these.  In cases like
> these, I think it's generally possible to avoid the space before
> the parens.  So IMO, we should clarify the rule instead
> (and the buildbot testresult diffing accordingly, if necessary).
>

The current rule is "When you write a test, do not
put text between parentheses at the end of the text
message", we can change it "when you write a test,
do not put text between parentheses at the end of
the text message and space before parentheses".

They are OK,
PASS: gdb.base/foo.exp: whatever test
FAIL: gdb.base/foo.exp: whatever test (timeout)
PASS: gdb.base/foo.exp: whatever test(1st)
PASS: gdb.base/foo.exp: whatever test(2nd)

They are not OK,
PASS: gdb.trace/trace-break.exp: 2 trace trace on: trace set_point (1)
PASS: gdb.trace/trace-break.exp: 2 trace trace on: trace set_point (2)

Is it right?

-- 
Yao (齐尧)

Re: [PATCH 2/2] gdb: Make some test names unique

[Pedro Alves]

On 06/22/2017 12:59 PM, Yao Qi wrote:
> On Thu, Jun 22, 2017 at 12:08 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/22/2017 11:37 AM, Yao Qi wrote:
>>> Andrew Burgess <andrew.burgess@embecosm.com> writes:
>>>
>>>
>>>>  mi_gdb_test "540-data-evaluate-expression vla1(1)" \
>>>> -  "540\\^done,value=\"1\"" "evaluate filled vla"
>>>> +  "540\\^done,value=\"1\"" "evaluate filled vla(1)"
>>>
>>> Do not use "tail parentheses" on the test message,
>>> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages
>>
>> The wiki doesn't mention it, but I think the rule should only apply when
>> there's a space before the parens.
> 
> I wanted to get confirm from analyze-racy-logs.py,
> but I was lost in it, so I decided to follow the wiki.

I only realized the need for the tweak to the rule after
the original discussion that led to the rule being added
to the wiki, and this is the first time I'm suggesting it,
I think.

>>
>> I don't think it's worth it, or even a good idea to try to
>> come up with different test names for all of these.  In cases like
>> these, I think it's generally possible to avoid the space before
>> the parens.  So IMO, we should clarify the rule instead
>> (and the buildbot testresult diffing accordingly, if necessary).
>>
> 
> The current rule is "When you write a test, do not
> put text between parentheses at the end of the text
> message", we can change it "when you write a test,
> do not put text between parentheses at the end of
> the text message and space before parentheses".
> 
> They are OK,
> PASS: gdb.base/foo.exp: whatever test
> FAIL: gdb.base/foo.exp: whatever test (timeout)
> PASS: gdb.base/foo.exp: whatever test(1st)
> PASS: gdb.base/foo.exp: whatever test(2nd)
> 
> They are not OK,
> PASS: gdb.trace/trace-break.exp: 2 trace trace on: trace set_point (1)
> PASS: gdb.trace/trace-break.exp: 2 trace trace on: trace set_point (2)
> 
> Is it right?

Yes.

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb: Make some test names unique

[Pedro Alves]

On 06/14/2017 07:57 PM, Andrew Burgess wrote:
> @@ -95,7 +95,7 @@ mi_run_cmd
>  mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
>    { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
>  mi_gdb_test "520-data-evaluate-expression vla1" \
> -  "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla"
> +  "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla at line $bp_lineno"
>  

As principle, please avoid putting line numbers in test names.  This
causes test names to spuriously change if/when the source file changes.

Thanks,
Pedro Alves

[PATCHv2 2/2] gdb: Make some test names unique

[Andrew Burgess]

Thanks for the review and feedback.  I've taken on-board what you both
said, and I have a new patch which I think should be fine, in this I:

  - Add '(...text...)' at the end, but only _without_ a space before
    the '('.

  - I no longer use line numbers to make test names unique, instead I
    use more descriptive text.

  - I've not changed the names of any tests that were already unique,
    even if those names included a line number.

OK to apply?

Thanks,
Andrew

---

gdb: Make some test names unique

Make sure all of the tests have unique names in
gdb.mi/mi-vla-fortran.exp.

gdb/testsuite/ChangeLog:

	* gdb.mi/mi-vla-fortran.exp: Make test names unique.

diff --git a/gdb/testsuite/gdb.mi/mi-vla-fortran.exp b/gdb/testsuite/gdb.mi/mi-vla-fortran.exp
index c7f9bd49dab..313a00a165f 100644
--- a/gdb/testsuite/gdb.mi/mi-vla-fortran.exp
+++ b/gdb/testsuite/gdb.mi/mi-vla-fortran.exp
@@ -44,7 +44,7 @@ mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
 mi_gdb_test "500-data-evaluate-expression vla1" \
-  "500\\^done,value=\"<not allocated>\"" "evaluate not allocated vla"
+  "500\\^done,value=\"<not allocated>\"" "evaluate not allocated vla(before allocation)"
 
 mi_create_varobj_checked vla1_not_allocated vla1 "<not allocated>" \
   "create local variable vla1_not_allocated"
@@ -95,7 +95,7 @@ mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
 mi_gdb_test "520-data-evaluate-expression vla1" \
-  "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla"
+  "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla(filled all 1s)"
 
 
 set bp_lineno [gdb_get_line_number "vla1-modified"]
@@ -106,13 +106,13 @@ mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
 mi_gdb_test "530-data-evaluate-expression vla1" \
-  "530\\^done,value=\"\\(1, 42, 1, 24, 1\\)\"" "evaluate filled vla"
+  "530\\^done,value=\"\\(1, 42, 1, 24, 1\\)\"" "evaluate filled vla(contents modified)"
 mi_gdb_test "540-data-evaluate-expression vla1(1)" \
-  "540\\^done,value=\"1\"" "evaluate filled vla"
+  "540\\^done,value=\"1\"" "evaluate filled vla(1)"
 mi_gdb_test "550-data-evaluate-expression vla1(2)" \
-  "550\\^done,value=\"42\"" "evaluate filled vla"
+  "550\\^done,value=\"42\"" "evaluate filled vla(2)"
 mi_gdb_test "560-data-evaluate-expression vla1(4)" \
-  "560\\^done,value=\"24\"" "evaluate filled vla"
+  "560\\^done,value=\"24\"" "evaluate filled vla(4)"
 
 
 set bp_lineno [gdb_get_line_number "vla1-deallocated"]
@@ -123,7 +123,7 @@ mi_run_cmd
 mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \
   { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno"
 mi_gdb_test "570-data-evaluate-expression vla1" \
-  "570\\^done,value=\"<not allocated>\"" "evaluate not allocated vla"
+  "570\\^done,value=\"<not allocated>\"" "evaluate not allocated vla(after deallocation)"
 
 
 set bp_lineno [gdb_get_line_number "pvla2-not-associated"]

Re: [PATCHv2 2/2] gdb: Make some test names unique

[Pedro Alves]

On 07/13/2017 09:15 PM, Andrew Burgess wrote:
> Thanks for the review and feedback.  I've taken on-board what you both
> said, and I have a new patch which I think should be fine, in this I:
> 
>   - Add '(...text...)' at the end, but only _without_ a space before
>     the '('.
> 
>   - I no longer use line numbers to make test names unique, instead I
>     use more descriptive text.
> 
>   - I've not changed the names of any tests that were already unique,
>     even if those names included a line number.
> 
> OK to apply?

OK.

Though I'd think avoiding ()s entirely in the cases that the
()s are not part of the expression would be less surprising,
like e.g.:

 -"evaluate filled vla(contents modified)"
 +"evaluate filled vla, contents modified"

OK with that change too.

Thanks,
Pedro Alves