[PATCH] Fix Ada val_print removal regression

[PATCH] Fix Ada val_print removal regression

[Tom Tromey]

The removal of val_print caused a regression in the Ada code.  In one
scenario, a variant type would not be properly printed, because the
address of a component was lost.  This patch fixes the bug in the most
straightforward way, by arranging to preserve the address.

gdb/ChangeLog
2020-03-17  Tom Tromey  <tromey@adacore.com>

	* ada-valprint.c (print_variant_part): Add "address" parameter.
	(print_field_values): Likewise.
	(ada_val_print_struct_union): Update.

gdb/testsuite/ChangeLog
2020-03-17  Tom Tromey  <tromey@adacore.com>

	* gdb.ada/sub_variant/subv.adb: New file.
	* gdb.ada/sub_variant.exp: New file.
---
 gdb/ChangeLog                              |  6 +++
 gdb/ada-valprint.c                         | 19 +++++----
 gdb/testsuite/ChangeLog                    |  5 +++
 gdb/testsuite/gdb.ada/sub_variant.exp      | 34 ++++++++++++++++
 gdb/testsuite/gdb.ada/sub_variant/subv.adb | 45 ++++++++++++++++++++++
 5 files changed, 101 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/sub_variant.exp
 create mode 100644 gdb/testsuite/gdb.ada/sub_variant/subv.adb

diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
index abf7ba4b959..46209a5be3d 100644
--- a/gdb/ada-valprint.c
+++ b/gdb/ada-valprint.c
@@ -31,7 +31,7 @@
 #include "gdbarch.h"
 
 static int print_field_values (struct type *, const gdb_byte *,
-			       int,
+			       CORE_ADDR, int,
 			       struct ui_file *, int,
 			       struct value *,
 			       const struct value_print_options *,
@@ -554,7 +554,7 @@ ada_printstr (struct ui_file *stream, struct type *type,
 
 static int
 print_variant_part (struct type *type, int field_num,
-		    const gdb_byte *valaddr, int offset,
+		    const gdb_byte *valaddr, CORE_ADDR address, int offset,
 		    struct ui_file *stream, int recurse,
 		    struct value *val,
 		    const struct value_print_options *options,
@@ -571,7 +571,7 @@ print_variant_part (struct type *type, int field_num,
   else
     return print_field_values
       (TYPE_FIELD_TYPE (var_type, which),
-       valaddr,
+       valaddr, address,
        offset + TYPE_FIELD_BITPOS (type, field_num) / HOST_CHAR_BIT
        + TYPE_FIELD_BITPOS (var_type, which) / HOST_CHAR_BIT,
        stream, recurse, val, options,
@@ -595,6 +595,7 @@ print_variant_part (struct type *type, int field_num,
 
 static int
 print_field_values (struct type *type, const gdb_byte *valaddr,
+		    CORE_ADDR address,
 		    int offset, struct ui_file *stream, int recurse,
 		    struct value *val,
 		    const struct value_print_options *options,
@@ -615,7 +616,7 @@ print_field_values (struct type *type, const gdb_byte *valaddr,
 	{
 	  comma_needed =
 	    print_field_values (TYPE_FIELD_TYPE (type, i),
-				valaddr,
+				valaddr, address,
 				(offset
 				 + TYPE_FIELD_BITPOS (type, i) / HOST_CHAR_BIT),
 				stream, recurse, val, options,
@@ -625,7 +626,7 @@ print_field_values (struct type *type, const gdb_byte *valaddr,
       else if (ada_is_variant_part (type, i))
 	{
 	  comma_needed =
-	    print_variant_part (type, i, valaddr,
+	    print_variant_part (type, i, valaddr, address,
 				offset, stream, recurse, val,
 				options, comma_needed,
 				outer_type, outer_offset, language);
@@ -689,8 +690,10 @@ print_field_values (struct type *type, const gdb_byte *valaddr,
 
 	  LONGEST local_off = (offset + TYPE_FIELD_BITPOS (type, i)
 			       / HOST_CHAR_BIT);
-	  struct value *v = value_from_contents (TYPE_FIELD_TYPE (type, i),
-						 valaddr + local_off);
+	  struct value *v
+	    = value_from_contents_and_address (TYPE_FIELD_TYPE (type, i),
+					       valaddr + local_off,
+					       address + local_off);
 	  common_val_print (v, stream, recurse + 1, &opts, language);
 	}
       annotate_field_end ();
@@ -941,7 +944,7 @@ ada_val_print_struct_union
 
   fprintf_filtered (stream, "(");
 
-  if (print_field_values (type, valaddr, offset_aligned,
+  if (print_field_values (type, valaddr, address, offset_aligned,
 			  stream, recurse, original_value, options,
 			  0, type, offset_aligned,
 			  language_def (language_ada)) != 0
diff --git a/gdb/testsuite/gdb.ada/sub_variant.exp b/gdb/testsuite/gdb.ada/sub_variant.exp
new file mode 100644
index 00000000000..381d138234d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_variant.exp
@@ -0,0 +1,34 @@
+# Copyright 2020 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 "ada.exp"
+
+standard_ada_testfile subv
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/subv.adb]
+runto "subv.adb:$bp_location"
+
+gdb_test "print q" \
+    "\\(indicator => first, associated => \\(indicator => first, value => 42\\), value => 51\\)"
+gdb_test "print r" \
+    "\\(indicator => first, associated => \\(indicator => last\\), value => 51\\)"
+gdb_test "print s" \
+    "\\(indicator => last, associated => \\(indicator => first, value => 42\\)\\)"
diff --git a/gdb/testsuite/gdb.ada/sub_variant/subv.adb b/gdb/testsuite/gdb.ada/sub_variant/subv.adb
new file mode 100644
index 00000000000..632ec32087d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_variant/subv.adb
@@ -0,0 +1,45 @@
+--  Copyright 2020 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/>.
+
+procedure Subv is
+  type Indicator_T is (First, Last);
+
+  type T1 (Indicator : Indicator_T := First) is
+    record
+      case Indicator is
+        when First =>
+          Value : Natural;
+        when Last =>
+          null;
+      end case;
+    end record;
+
+  type T2 (Indicator : Indicator_T := First) is
+    record
+      Associated : T1;
+      case Indicator is
+        when First =>
+          Value : Natural;
+        when Last =>
+          null;
+      end case;
+    end record;
+
+  Q : T2  := ( First, (First, 42), 51 );
+  R : T2  := ( First, (Indicator => Last), 51 );
+  S : T2  := ( Last, (First, 42));
+begin
+  null;  -- STOP
+end;
-- 
2.21.1

Re: [PATCH] Fix Ada val_print removal regression

[Simon Marchi]

On 2020-03-17 2:00 p.m., Tom Tromey wrote:
> @@ -595,6 +595,7 @@ print_variant_part (struct type *type, int field_num,
>  
>  static int
>  print_field_values (struct type *type, const gdb_byte *valaddr,

The comment of this function would need to be updated.

While checking this, I noticed that the `struct value *` parameters of print_field_values
and print_variant_part appear to be unnecessary.  They are only passed recursively, but
never actually used.

But most importantly, I was thinking that this ada_val_print_struct_union function looked
like it was accepting a decomposed struct value, much like the API that you have removed.
Would it work and be good to change it to work more with `struct value`s?  It seems to me
like this bug would have been avoided.

Simon

Re: [PATCH] Fix Ada val_print removal regression

[Tom Tromey]

Simon> While checking this, I noticed that the `struct value *`
Simon> parameters of print_field_values and print_variant_part appear to
Simon> be unnecessary.  They are only passed recursively, but never
Simon> actually used.

Thanks

Simon> But most importantly, I was thinking that this
Simon> ada_val_print_struct_union function looked like it was accepting
Simon> a decomposed struct value, much like the API that you have
Simon> removed.  Would it work and be good to change it to work more
Simon> with `struct value`s?  It seems to me like this bug would have
Simon> been avoided.

Yeah.  The val_print removal series neglected a few spots.  I will try
to see if this one can be easily changed.

Tom

Re: [PATCH] Fix Ada val_print removal regression

[Tom Tromey]

Tom> Yeah.  The val_print removal series neglected a few spots.  I will try
Tom> to see if this one can be easily changed.

Here's v2.

Tom

commit 328b3401bb5ea0d6135e8f084f35d29e77debec9
Author: Tom Tromey <tromey@adacore.com>
Date:   Thu Mar 19 13:36:44 2020 -0600

    Fix Ada val_print removal regression
    
    The removal of val_print caused a regression in the Ada code.  In one
    scenario, a variant type would not be properly printed, because the
    address of a component was lost.  This patch fixes the bug by changing
    this API to be value-based.  This is cleaner and fixes the bug as a
    side effect.
    
    gdb/ChangeLog
    2020-03-19  Tom Tromey  <tromey@adacore.com>
    
            * ada-valprint.c (print_variant_part): Remove parameters; switch
            to value-based API.
            (print_field_values): Likewise.
            (ada_val_print_struct_union): Likewise.
            (ada_value_print_1): Update.
    
    gdb/testsuite/ChangeLog
    2020-03-19  Tom Tromey  <tromey@adacore.com>
    
            * gdb.ada/sub_variant/subv.adb: New file.
            * gdb.ada/sub_variant.exp: New file.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0955d648e79..d150b46fce3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2020-03-19  Tom Tromey  <tromey@adacore.com>
+
+	* ada-valprint.c (print_variant_part): Remove parameters; switch
+	to value-based API.
+	(print_field_values): Likewise.
+	(ada_val_print_struct_union): Likewise.
+	(ada_value_print_1): Update.
+
 2020-03-19  Kamil Rytarowski  <n54@gmx.com>
 
 	* x86-bsd-nat.c (gdb_ptrace): New.
diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
index abf7ba4b959..4f2cc63a556 100644
--- a/gdb/ada-valprint.c
+++ b/gdb/ada-valprint.c
@@ -30,13 +30,11 @@
 #include "cli/cli-style.h"
 #include "gdbarch.h"
 
-static int print_field_values (struct type *, const gdb_byte *,
-			       int,
+static int print_field_values (struct value *, struct value *,
 			       struct ui_file *, int,
-			       struct value *,
 			       const struct value_print_options *,
-			       int, struct type *, int,
-			       const struct language_defn *);
+			       int, const struct language_defn *);
+
 \f
 
 /* Make TYPE unsigned if its range of values includes no negatives.  */
@@ -553,39 +551,34 @@ ada_printstr (struct ui_file *stream, struct type *type,
 }
 
 static int
-print_variant_part (struct type *type, int field_num,
-		    const gdb_byte *valaddr, int offset,
+print_variant_part (struct value *value, int field_num,
+		    struct value *outer_value,
 		    struct ui_file *stream, int recurse,
-		    struct value *val,
 		    const struct value_print_options *options,
 		    int comma_needed,
-		    struct type *outer_type, int outer_offset,
 		    const struct language_defn *language)
 {
+  struct type *type = value_type (value);
   struct type *var_type = TYPE_FIELD_TYPE (type, field_num);
-  int which = ada_which_variant_applies (var_type, outer_type,
-					 valaddr + outer_offset);
+  int which = ada_which_variant_applies (var_type,
+					 value_type (outer_value),
+					 value_contents (outer_value));
 
   if (which < 0)
     return 0;
-  else
-    return print_field_values
-      (TYPE_FIELD_TYPE (var_type, which),
-       valaddr,
-       offset + TYPE_FIELD_BITPOS (type, field_num) / HOST_CHAR_BIT
-       + TYPE_FIELD_BITPOS (var_type, which) / HOST_CHAR_BIT,
-       stream, recurse, val, options,
-       comma_needed, outer_type, outer_offset, language);
+
+  struct value *variant = value_field (value, which);
+  return print_field_values (variant, outer_value, stream, recurse,
+			     options, comma_needed, language);
 }
 
-/* Print out fields of value at VALADDR + OFFSET having structure type TYPE.
+/* Print out fields of VALUE.
 
-   TYPE, VALADDR, OFFSET, STREAM, RECURSE, and OPTIONS have the same
-   meanings as in ada_print_value and ada_val_print.
+   STREAM, RECURSE, and OPTIONS have the same meanings as in
+   ada_print_value and ada_value_print.
 
-   OUTER_TYPE and OUTER_OFFSET give type and address of enclosing
-   record (used to get discriminant values when printing variant
-   parts).
+   OUTER_VALUE gives the enclosing record (used to get discriminant
+   values when printing variant parts).
 
    COMMA_NEEDED is 1 if fields have been printed at the current recursion
    level, so that a comma is needed before any field printed by this
@@ -594,16 +587,15 @@ print_variant_part (struct type *type, int field_num,
    Returns 1 if COMMA_NEEDED or any fields were printed.  */
 
 static int
-print_field_values (struct type *type, const gdb_byte *valaddr,
-		    int offset, struct ui_file *stream, int recurse,
-		    struct value *val,
+print_field_values (struct value *value, struct value *outer_value,
+		    struct ui_file *stream, int recurse,
 		    const struct value_print_options *options,
 		    int comma_needed,
-		    struct type *outer_type, int outer_offset,
 		    const struct language_defn *language)
 {
   int i, len;
 
+  struct type *type = value_type (value);
   len = TYPE_NFIELDS (type);
 
   for (i = 0; i < len; i += 1)
@@ -614,21 +606,16 @@ print_field_values (struct type *type, const gdb_byte *valaddr,
       if (ada_is_wrapper_field (type, i))
 	{
 	  comma_needed =
-	    print_field_values (TYPE_FIELD_TYPE (type, i),
-				valaddr,
-				(offset
-				 + TYPE_FIELD_BITPOS (type, i) / HOST_CHAR_BIT),
-				stream, recurse, val, options,
-				comma_needed, type, offset, language);
+	    print_field_values (value_field (value, i), outer_value,
+				stream, recurse, options,
+				comma_needed, language);
 	  continue;
 	}
       else if (ada_is_variant_part (type, i))
 	{
 	  comma_needed =
-	    print_variant_part (type, i, valaddr,
-				offset, stream, recurse, val,
-				options, comma_needed,
-				outer_type, outer_offset, language);
+	    print_variant_part (value, i, outer_value, stream, recurse,
+				options, comma_needed, language);
 	  continue;
 	}
 
@@ -672,8 +659,8 @@ print_field_values (struct type *type, const gdb_byte *valaddr,
 
 	      adjust_type_signedness (TYPE_FIELD_TYPE (type, i));
 	      v = ada_value_primitive_packed_val
-		    (NULL, valaddr,
-		     offset + bit_pos / HOST_CHAR_BIT,
+		    (value, nullptr,
+		     bit_pos / HOST_CHAR_BIT,
 		     bit_pos % HOST_CHAR_BIT,
 		     bit_size, TYPE_FIELD_TYPE (type, i));
 	      opts = *options;
@@ -687,10 +674,7 @@ print_field_values (struct type *type, const gdb_byte *valaddr,
 
 	  opts.deref_ref = 0;
 
-	  LONGEST local_off = (offset + TYPE_FIELD_BITPOS (type, i)
-			       / HOST_CHAR_BIT);
-	  struct value *v = value_from_contents (TYPE_FIELD_TYPE (type, i),
-						 valaddr + local_off);
+	  struct value *v = value_field (value, i);
 	  common_val_print (v, stream, recurse + 1, &opts, language);
 	}
       annotate_field_end ();
@@ -923,17 +907,16 @@ ada_val_print_enum (struct value *value, struct ui_file *stream, int recurse,
     print_longest (stream, 'd', 0, val);
 }
 
-/* Implement Ada val_print'ing for the case where TYPE is
-   a TYPE_CODE_STRUCT or TYPE_CODE_UNION.  */
+/* Implement Ada val_print'ing for the case where the type is
+   TYPE_CODE_STRUCT or TYPE_CODE_UNION.  */
 
 static void
 ada_val_print_struct_union
-  (struct type *type, const gdb_byte *valaddr, int offset,
-   int offset_aligned, CORE_ADDR address, struct ui_file *stream,
+  (struct value *value, struct ui_file *stream,
    int recurse, struct value *original_value,
    const struct value_print_options *options)
 {
-  if (ada_is_bogus_array_descriptor (type))
+  if (ada_is_bogus_array_descriptor (value_type (value)))
     {
       fprintf_filtered (stream, "(...?)");
       return;
@@ -941,10 +924,8 @@ ada_val_print_struct_union
 
   fprintf_filtered (stream, "(");
 
-  if (print_field_values (type, valaddr, offset_aligned,
-			  stream, recurse, original_value, options,
-			  0, type, offset_aligned,
-			  language_def (language_ada)) != 0
+  if (print_field_values (value, original_value, stream, recurse, options,
+			  0, language_def (language_ada)) != 0
       && options->prettyformat)
     {
       fprintf_filtered (stream, "\n");
@@ -1116,9 +1097,7 @@ ada_value_print_1 (struct value *val, struct ui_file *stream, int recurse,
 
     case TYPE_CODE_UNION:
     case TYPE_CODE_STRUCT:
-      ada_val_print_struct_union (type, valaddr, 0, 0,
-				  address, stream, recurse,
-				  val, options);
+      ada_val_print_struct_union (val, stream, recurse, val, options);
       break;
 
     case TYPE_CODE_ARRAY:
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index cb65ffa784c..a3b115d6dc2 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2020-03-19  Tom Tromey  <tromey@adacore.com>
+
+	* gdb.ada/sub_variant/subv.adb: New file.
+	* gdb.ada/sub_variant.exp: New file.
+
 2020-03-19  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdb.server/exit-multiple-threads.c: New file.
diff --git a/gdb/testsuite/gdb.ada/sub_variant.exp b/gdb/testsuite/gdb.ada/sub_variant.exp
new file mode 100644
index 00000000000..381d138234d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_variant.exp
@@ -0,0 +1,34 @@
+# Copyright 2020 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 "ada.exp"
+
+standard_ada_testfile subv
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/subv.adb]
+runto "subv.adb:$bp_location"
+
+gdb_test "print q" \
+    "\\(indicator => first, associated => \\(indicator => first, value => 42\\), value => 51\\)"
+gdb_test "print r" \
+    "\\(indicator => first, associated => \\(indicator => last\\), value => 51\\)"
+gdb_test "print s" \
+    "\\(indicator => last, associated => \\(indicator => first, value => 42\\)\\)"
diff --git a/gdb/testsuite/gdb.ada/sub_variant/subv.adb b/gdb/testsuite/gdb.ada/sub_variant/subv.adb
new file mode 100644
index 00000000000..632ec32087d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_variant/subv.adb
@@ -0,0 +1,45 @@
+--  Copyright 2020 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/>.
+
+procedure Subv is
+  type Indicator_T is (First, Last);
+
+  type T1 (Indicator : Indicator_T := First) is
+    record
+      case Indicator is
+        when First =>
+          Value : Natural;
+        when Last =>
+          null;
+      end case;
+    end record;
+
+  type T2 (Indicator : Indicator_T := First) is
+    record
+      Associated : T1;
+      case Indicator is
+        when First =>
+          Value : Natural;
+        when Last =>
+          null;
+      end case;
+    end record;
+
+  Q : T2  := ( First, (First, 42), 51 );
+  R : T2  := ( First, (Indicator => Last), 51 );
+  S : T2  := ( Last, (First, 42));
+begin
+  null;  -- STOP
+end;

Re: [PATCH] Fix Ada val_print removal regression

[Simon Marchi]

On 2020-03-19 3:42 p.m., Tom Tromey wrote:
> @@ -553,39 +551,34 @@ ada_printstr (struct ui_file *stream, struct type *type,
>  }
>  
>  static int
> -print_variant_part (struct type *type, int field_num,
> -		    const gdb_byte *valaddr, int offset,
> +print_variant_part (struct value *value, int field_num,
> +		    struct value *outer_value,
>  		    struct ui_file *stream, int recurse,
> -		    struct value *val,
>  		    const struct value_print_options *options,
>  		    int comma_needed,
> -		    struct type *outer_type, int outer_offset,
>  		    const struct language_defn *language)
>  {
> +  struct type *type = value_type (value);
>    struct type *var_type = TYPE_FIELD_TYPE (type, field_num);
> -  int which = ada_which_variant_applies (var_type, outer_type,
> -					 valaddr + outer_offset);
> +  int which = ada_which_variant_applies (var_type,
> +					 value_type (outer_value),
> +					 value_contents (outer_value));z
>  
>    if (which < 0)
>      return 0;
> -  else
> -    return print_field_values
> -      (TYPE_FIELD_TYPE (var_type, which),
> -       valaddr,
> -       offset + TYPE_FIELD_BITPOS (type, field_num) / HOST_CHAR_BIT
> -       + TYPE_FIELD_BITPOS (var_type, which) / HOST_CHAR_BIT,
> -       stream, recurse, val, options,
> -       comma_needed, outer_type, outer_offset, language);
> +
> +  struct value *variant = value_field (value, which);

A bit of a nit, but it's to make sure I understand what's happening.  From what
I understand, the variant is the enclosing type, from which only one component
is active at a given time.  This value variable represents the active component,
right?  If so, I'd suggest naming it active_component or something like that.

> @@ -923,17 +907,16 @@ ada_val_print_enum (struct value *value, struct ui_file *stream, int recurse,
>      print_longest (stream, 'd', 0, val);
>  }
>  
> -/* Implement Ada val_print'ing for the case where TYPE is
> -   a TYPE_CODE_STRUCT or TYPE_CODE_UNION.  */
> +/* Implement Ada val_print'ing for the case where the type is
> +   TYPE_CODE_STRUCT or TYPE_CODE_UNION.  */
>  
>  static void
>  ada_val_print_struct_union
> -  (struct type *type, const gdb_byte *valaddr, int offset,
> -   int offset_aligned, CORE_ADDR address, struct ui_file *stream,
> +  (struct value *value, struct ui_file *stream,
>     int recurse, struct value *original_value,
>     const struct value_print_options *options)
>  {

The sole caller of this function passes the same `val` twice, so I suppose you could remove `original_value`.

Simon

Re: [PATCH] Fix Ada val_print removal regression

[Tom Tromey]

>> +  struct value *variant = value_field (value, which);

Simon> A bit of a nit, but it's to make sure I understand what's
Simon> happening.  From what I understand, the variant is the enclosing
Simon> type, from which only one component is active at a given time.
Simon> This value variable represents the active component, right?

Yep.

Simon> If
Simon> so, I'd suggest naming it active_component or something like
Simon> that.

Done.

Simon> The sole caller of this function passes the same `val` twice, so
Simon> I suppose you could remove `original_value`.

I did this too.

Tom

commit bbd4ecc8eabafd4f4996810d8677c266539c3aa1
Author: Tom Tromey <tromey@adacore.com>
Date:   Thu Mar 19 13:36:44 2020 -0600

    Fix Ada val_print removal regression
    
    The removal of val_print caused a regression in the Ada code.  In one
    scenario, a variant type would not be properly printed, because the
    address of a component was lost.  This patch fixes the bug by changing
    this API to be value-based.  This is cleaner and fixes the bug as a
    side effect.
    
    gdb/ChangeLog
    2020-03-19  Tom Tromey  <tromey@adacore.com>
    
            * ada-valprint.c (print_variant_part): Remove parameters; switch
            to value-based API.
            (print_field_values): Likewise.
            (ada_val_print_struct_union): Likewise.
            (ada_value_print_1): Update.
    
    gdb/testsuite/ChangeLog
    2020-03-19  Tom Tromey  <tromey@adacore.com>
    
            * gdb.ada/sub_variant/subv.adb: New file.
            * gdb.ada/sub_variant.exp: New file.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0955d648e79..d150b46fce3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2020-03-19  Tom Tromey  <tromey@adacore.com>
+
+	* ada-valprint.c (print_variant_part): Remove parameters; switch
+	to value-based API.
+	(print_field_values): Likewise.
+	(ada_val_print_struct_union): Likewise.
+	(ada_value_print_1): Update.
+
 2020-03-19  Kamil Rytarowski  <n54@gmx.com>
 
 	* x86-bsd-nat.c (gdb_ptrace): New.
diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
index abf7ba4b959..59ada24b947 100644
--- a/gdb/ada-valprint.c
+++ b/gdb/ada-valprint.c
@@ -30,13 +30,11 @@
 #include "cli/cli-style.h"
 #include "gdbarch.h"
 
-static int print_field_values (struct type *, const gdb_byte *,
-			       int,
+static int print_field_values (struct value *, struct value *,
 			       struct ui_file *, int,
-			       struct value *,
 			       const struct value_print_options *,
-			       int, struct type *, int,
-			       const struct language_defn *);
+			       int, const struct language_defn *);
+
 \f
 
 /* Make TYPE unsigned if its range of values includes no negatives.  */
@@ -553,39 +551,34 @@ ada_printstr (struct ui_file *stream, struct type *type,
 }
 
 static int
-print_variant_part (struct type *type, int field_num,
-		    const gdb_byte *valaddr, int offset,
+print_variant_part (struct value *value, int field_num,
+		    struct value *outer_value,
 		    struct ui_file *stream, int recurse,
-		    struct value *val,
 		    const struct value_print_options *options,
 		    int comma_needed,
-		    struct type *outer_type, int outer_offset,
 		    const struct language_defn *language)
 {
+  struct type *type = value_type (value);
   struct type *var_type = TYPE_FIELD_TYPE (type, field_num);
-  int which = ada_which_variant_applies (var_type, outer_type,
-					 valaddr + outer_offset);
+  int which = ada_which_variant_applies (var_type,
+					 value_type (outer_value),
+					 value_contents (outer_value));
 
   if (which < 0)
     return 0;
-  else
-    return print_field_values
-      (TYPE_FIELD_TYPE (var_type, which),
-       valaddr,
-       offset + TYPE_FIELD_BITPOS (type, field_num) / HOST_CHAR_BIT
-       + TYPE_FIELD_BITPOS (var_type, which) / HOST_CHAR_BIT,
-       stream, recurse, val, options,
-       comma_needed, outer_type, outer_offset, language);
+
+  struct value *active_component = value_field (value, which);
+  return print_field_values (active_component, outer_value, stream, recurse,
+			     options, comma_needed, language);
 }
 
-/* Print out fields of value at VALADDR + OFFSET having structure type TYPE.
+/* Print out fields of VALUE.
 
-   TYPE, VALADDR, OFFSET, STREAM, RECURSE, and OPTIONS have the same
-   meanings as in ada_print_value and ada_val_print.
+   STREAM, RECURSE, and OPTIONS have the same meanings as in
+   ada_print_value and ada_value_print.
 
-   OUTER_TYPE and OUTER_OFFSET give type and address of enclosing
-   record (used to get discriminant values when printing variant
-   parts).
+   OUTER_VALUE gives the enclosing record (used to get discriminant
+   values when printing variant parts).
 
    COMMA_NEEDED is 1 if fields have been printed at the current recursion
    level, so that a comma is needed before any field printed by this
@@ -594,16 +587,15 @@ print_variant_part (struct type *type, int field_num,
    Returns 1 if COMMA_NEEDED or any fields were printed.  */
 
 static int
-print_field_values (struct type *type, const gdb_byte *valaddr,
-		    int offset, struct ui_file *stream, int recurse,
-		    struct value *val,
+print_field_values (struct value *value, struct value *outer_value,
+		    struct ui_file *stream, int recurse,
 		    const struct value_print_options *options,
 		    int comma_needed,
-		    struct type *outer_type, int outer_offset,
 		    const struct language_defn *language)
 {
   int i, len;
 
+  struct type *type = value_type (value);
   len = TYPE_NFIELDS (type);
 
   for (i = 0; i < len; i += 1)
@@ -614,21 +606,16 @@ print_field_values (struct type *type, const gdb_byte *valaddr,
       if (ada_is_wrapper_field (type, i))
 	{
 	  comma_needed =
-	    print_field_values (TYPE_FIELD_TYPE (type, i),
-				valaddr,
-				(offset
-				 + TYPE_FIELD_BITPOS (type, i) / HOST_CHAR_BIT),
-				stream, recurse, val, options,
-				comma_needed, type, offset, language);
+	    print_field_values (value_field (value, i), outer_value,
+				stream, recurse, options,
+				comma_needed, language);
 	  continue;
 	}
       else if (ada_is_variant_part (type, i))
 	{
 	  comma_needed =
-	    print_variant_part (type, i, valaddr,
-				offset, stream, recurse, val,
-				options, comma_needed,
-				outer_type, outer_offset, language);
+	    print_variant_part (value, i, outer_value, stream, recurse,
+				options, comma_needed, language);
 	  continue;
 	}
 
@@ -672,8 +659,8 @@ print_field_values (struct type *type, const gdb_byte *valaddr,
 
 	      adjust_type_signedness (TYPE_FIELD_TYPE (type, i));
 	      v = ada_value_primitive_packed_val
-		    (NULL, valaddr,
-		     offset + bit_pos / HOST_CHAR_BIT,
+		    (value, nullptr,
+		     bit_pos / HOST_CHAR_BIT,
 		     bit_pos % HOST_CHAR_BIT,
 		     bit_size, TYPE_FIELD_TYPE (type, i));
 	      opts = *options;
@@ -687,10 +674,7 @@ print_field_values (struct type *type, const gdb_byte *valaddr,
 
 	  opts.deref_ref = 0;
 
-	  LONGEST local_off = (offset + TYPE_FIELD_BITPOS (type, i)
-			       / HOST_CHAR_BIT);
-	  struct value *v = value_from_contents (TYPE_FIELD_TYPE (type, i),
-						 valaddr + local_off);
+	  struct value *v = value_field (value, i);
 	  common_val_print (v, stream, recurse + 1, &opts, language);
 	}
       annotate_field_end ();
@@ -923,17 +907,16 @@ ada_val_print_enum (struct value *value, struct ui_file *stream, int recurse,
     print_longest (stream, 'd', 0, val);
 }
 
-/* Implement Ada val_print'ing for the case where TYPE is
-   a TYPE_CODE_STRUCT or TYPE_CODE_UNION.  */
+/* Implement Ada val_print'ing for the case where the type is
+   TYPE_CODE_STRUCT or TYPE_CODE_UNION.  */
 
 static void
-ada_val_print_struct_union
-  (struct type *type, const gdb_byte *valaddr, int offset,
-   int offset_aligned, CORE_ADDR address, struct ui_file *stream,
-   int recurse, struct value *original_value,
-   const struct value_print_options *options)
+ada_val_print_struct_union (struct value *value,
+			    struct ui_file *stream,
+			    int recurse,
+			    const struct value_print_options *options)
 {
-  if (ada_is_bogus_array_descriptor (type))
+  if (ada_is_bogus_array_descriptor (value_type (value)))
     {
       fprintf_filtered (stream, "(...?)");
       return;
@@ -941,10 +924,8 @@ ada_val_print_struct_union
 
   fprintf_filtered (stream, "(");
 
-  if (print_field_values (type, valaddr, offset_aligned,
-			  stream, recurse, original_value, options,
-			  0, type, offset_aligned,
-			  language_def (language_ada)) != 0
+  if (print_field_values (value, value, stream, recurse, options,
+			  0, language_def (language_ada)) != 0
       && options->prettyformat)
     {
       fprintf_filtered (stream, "\n");
@@ -1116,9 +1097,7 @@ ada_value_print_1 (struct value *val, struct ui_file *stream, int recurse,
 
     case TYPE_CODE_UNION:
     case TYPE_CODE_STRUCT:
-      ada_val_print_struct_union (type, valaddr, 0, 0,
-				  address, stream, recurse,
-				  val, options);
+      ada_val_print_struct_union (val, stream, recurse, options);
       break;
 
     case TYPE_CODE_ARRAY:
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index cb65ffa784c..a3b115d6dc2 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2020-03-19  Tom Tromey  <tromey@adacore.com>
+
+	* gdb.ada/sub_variant/subv.adb: New file.
+	* gdb.ada/sub_variant.exp: New file.
+
 2020-03-19  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdb.server/exit-multiple-threads.c: New file.
diff --git a/gdb/testsuite/gdb.ada/sub_variant.exp b/gdb/testsuite/gdb.ada/sub_variant.exp
new file mode 100644
index 00000000000..381d138234d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_variant.exp
@@ -0,0 +1,34 @@
+# Copyright 2020 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 "ada.exp"
+
+standard_ada_testfile subv
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/subv.adb]
+runto "subv.adb:$bp_location"
+
+gdb_test "print q" \
+    "\\(indicator => first, associated => \\(indicator => first, value => 42\\), value => 51\\)"
+gdb_test "print r" \
+    "\\(indicator => first, associated => \\(indicator => last\\), value => 51\\)"
+gdb_test "print s" \
+    "\\(indicator => last, associated => \\(indicator => first, value => 42\\)\\)"
diff --git a/gdb/testsuite/gdb.ada/sub_variant/subv.adb b/gdb/testsuite/gdb.ada/sub_variant/subv.adb
new file mode 100644
index 00000000000..632ec32087d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_variant/subv.adb
@@ -0,0 +1,45 @@
+--  Copyright 2020 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/>.
+
+procedure Subv is
+  type Indicator_T is (First, Last);
+
+  type T1 (Indicator : Indicator_T := First) is
+    record
+      case Indicator is
+        when First =>
+          Value : Natural;
+        when Last =>
+          null;
+      end case;
+    end record;
+
+  type T2 (Indicator : Indicator_T := First) is
+    record
+      Associated : T1;
+      case Indicator is
+        when First =>
+          Value : Natural;
+        when Last =>
+          null;
+      end case;
+    end record;
+
+  Q : T2  := ( First, (First, 42), 51 );
+  R : T2  := ( First, (Indicator => Last), 51 );
+  S : T2  := ( Last, (First, 42));
+begin
+  null;  -- STOP
+end;

Re: [PATCH] Fix Ada val_print removal regression

[Simon Marchi]

On 2020-03-19 5:13 p.m., Tom Tromey wrote:
>>> +  struct value *variant = value_field (value, which);
> 
> Simon> A bit of a nit, but it's to make sure I understand what's
> Simon> happening.  From what I understand, the variant is the enclosing
> Simon> type, from which only one component is active at a given time.
> Simon> This value variable represents the active component, right?
> 
> Yep.
> 
> Simon> If
> Simon> so, I'd suggest naming it active_component or something like
> Simon> that.
> 
> Done.
> 
> Simon> The sole caller of this function passes the same `val` twice, so
> Simon> I suppose you could remove `original_value`.
> 
> I did this too.
> 
> Tom

Thanks, I think this is fine.

Simon