Patchwork Fix gdb.ada/vla.exp

login
register
mail settings
Submitter Tom Tromey
Date May 28, 2019, 6:25 p.m.
Message ID <20190528182556.20011-1-tromey@adacore.com>
Download mbox | patch
Permalink /patch/32874/
State New
Headers show

Comments

Tom Tromey - May 28, 2019, 6:25 p.m.
PR ada/24539 concerns a test failure in gdb.ada/vla.exp.

The problem here is that different versions of Gnat emit the
structure's fields in different orders -- with the order currently
failing actually being the correct one.

This patch attempts to fix the problem by changing the test to accept
both orders.  I can't test this as all the compilers I have available
here use the incorrect order.

I've reported a Gnat compiler bug internally in hopes of getting this
fixed.

gdb/testsuite/ChangeLog
2019-05-28  Tom Tromey  <tromey@adacore.com>

	PR ada/24539:
	* gdb.ada/vla.exp (mk): New proc.
	Use it in test results.
---
 gdb/testsuite/ChangeLog       |  6 ++++++
 gdb/testsuite/gdb.ada/vla.exp | 26 ++++++++++++++++++++++----
 2 files changed, 28 insertions(+), 4 deletions(-)
Joel Brobecker - May 29, 2019, 3:27 p.m.
Hi Tom,

Thanks for the patch :)

> PR ada/24539 concerns a test failure in gdb.ada/vla.exp.
> 
> The problem here is that different versions of Gnat emit the
> structure's fields in different orders -- with the order currently
> failing actually being the correct one.
> 
> This patch attempts to fix the problem by changing the test to accept
> both orders.  I can't test this as all the compilers I have available
> here use the incorrect order.
> 
> I've reported a Gnat compiler bug internally in hopes of getting this
> fixed.
> 
> gdb/testsuite/ChangeLog
> 2019-05-28  Tom Tromey  <tromey@adacore.com>
> 
> 	PR ada/24539:
> 	* gdb.ada/vla.exp (mk): New proc.
> 	Use it in test results.

That's pretty much what we did in AdaCore's internal gdb-testsuite
(not based on the official dejagnu-based testsuite), and the change
looks good to me, so could be pushed as is. But I thought I'd mention
another option for this testcase, which is to change the source code
to use...

    pragma No_Component_Reordering (Record_Type);

... so as to avoid the reordering altogether. Normally, I tend to avoid
those, because I try to be as close to what users would do. In this case,
I'm 50/50, because while I don't like to deviate from standard scenarios,
it is also true that the purpose of the test is really to verify that
we handle arrays whose bounds are dynamic and originating from
a discriminant. So component ordering is really not a part of what
we are trying to test.

So, to summarize, I'd be fine with both approaches. I don't see
the re-ordering heuristics in the compiler change the order of
the components of this record again sometime soon.

> ---
>  gdb/testsuite/ChangeLog       |  6 ++++++
>  gdb/testsuite/gdb.ada/vla.exp | 26 ++++++++++++++++++++++----
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.ada/vla.exp b/gdb/testsuite/gdb.ada/vla.exp
> index 7e10e9b8223..3b325c74899 100644
> --- a/gdb/testsuite/gdb.ada/vla.exp
> +++ b/gdb/testsuite/gdb.ada/vla.exp
> @@ -28,11 +28,29 @@ clean_restart ${testfile}
>  set bp_location [gdb_get_line_number "Set breakpoint here" ${testdir}/vla.adb]
>  runto "vla.adb:$bp_location"
>  
> +# Helper proc to compute the regexp needed to match both results that
> +# have been seen.
> +proc mk {a1 a2} {
> +    set n1 "a1 => $a1, i2 => 2, a2 => $a2, i3 => 3"
> +    set n2 "i2 => 2, i3 => 3, a1 => $a1, a2 => $a2"
> +    return "($n1|$n2)"
> +}
> +
> +# Some versions of gnat emit the variable-length elements after the
> +# other elements, so these test cases accept both.
> +
> +set r00 [mk "\\(\\)" "\\(\\)"]
>  gdb_test "print r00" \
> -         "= \\(l1 => 0, l2 => 0, i1 => 1, i2 => 2, i3 => 3, a1 => \\(\\), a2 => \\(\\)\\)"
> +    "= \\(l1 => 0, l2 => 0, i1 => 1, $r00\\)"
> +
> +set r01 [mk "\\(\\)" "\\(20\\)"]
>  gdb_test "print r01" \
> -         "= \\(l1 => 0, l2 => 1, i1 => 1, i2 => 2, i3 => 3, a1 => \\(\\), a2 => \\(20\\)\\)"
> +    "= \\(l1 => 0, l2 => 1, i1 => 1, $r01\\)"
> +
> +set r10 [mk "\\(10\\)" "\\(\\)"]
>  gdb_test "print r10" \
> -         "= \\(l1 => 1, l2 => 0, i1 => 1, i2 => 2, i3 => 3, a1 => \\(10\\), a2 => \\(\\)\\)"
> +    "= \\(l1 => 1, l2 => 0, i1 => 1, $r10\\)"
> +
> +set r22 [mk "\\(10, 10\\)" "\\(20, 20\\)"]
>  gdb_test "print r22" \
> -         "= \\(l1 => 2, l2 => 2, i1 => 1, i2 => 2, i3 => 3, a1 => \\(10, 10\\), a2 => \\(20, 20\\)\\)"
> +    "= \\(l1 => 2, l2 => 2, i1 => 1, $r22\\)"
> -- 
> 2.20.1

Patch

diff --git a/gdb/testsuite/gdb.ada/vla.exp b/gdb/testsuite/gdb.ada/vla.exp
index 7e10e9b8223..3b325c74899 100644
--- a/gdb/testsuite/gdb.ada/vla.exp
+++ b/gdb/testsuite/gdb.ada/vla.exp
@@ -28,11 +28,29 @@  clean_restart ${testfile}
 set bp_location [gdb_get_line_number "Set breakpoint here" ${testdir}/vla.adb]
 runto "vla.adb:$bp_location"
 
+# Helper proc to compute the regexp needed to match both results that
+# have been seen.
+proc mk {a1 a2} {
+    set n1 "a1 => $a1, i2 => 2, a2 => $a2, i3 => 3"
+    set n2 "i2 => 2, i3 => 3, a1 => $a1, a2 => $a2"
+    return "($n1|$n2)"
+}
+
+# Some versions of gnat emit the variable-length elements after the
+# other elements, so these test cases accept both.
+
+set r00 [mk "\\(\\)" "\\(\\)"]
 gdb_test "print r00" \
-         "= \\(l1 => 0, l2 => 0, i1 => 1, i2 => 2, i3 => 3, a1 => \\(\\), a2 => \\(\\)\\)"
+    "= \\(l1 => 0, l2 => 0, i1 => 1, $r00\\)"
+
+set r01 [mk "\\(\\)" "\\(20\\)"]
 gdb_test "print r01" \
-         "= \\(l1 => 0, l2 => 1, i1 => 1, i2 => 2, i3 => 3, a1 => \\(\\), a2 => \\(20\\)\\)"
+    "= \\(l1 => 0, l2 => 1, i1 => 1, $r01\\)"
+
+set r10 [mk "\\(10\\)" "\\(\\)"]
 gdb_test "print r10" \
-         "= \\(l1 => 1, l2 => 0, i1 => 1, i2 => 2, i3 => 3, a1 => \\(10\\), a2 => \\(\\)\\)"
+    "= \\(l1 => 1, l2 => 0, i1 => 1, $r10\\)"
+
+set r22 [mk "\\(10, 10\\)" "\\(20, 20\\)"]
 gdb_test "print r22" \
-         "= \\(l1 => 2, l2 => 2, i1 => 1, i2 => 2, i3 => 3, a1 => \\(10, 10\\), a2 => \\(20, 20\\)\\)"
+    "= \\(l1 => 2, l2 => 2, i1 => 1, $r22\\)"