[PATCH 0/2] Fix two bugs with packed array printing

[PATCH 0/2] Fix two bugs with packed array printing

[Tom Tromey]

This series fixes a couple of bugs involving the printing of packed
arrays.

Regression tested on x86-64 Fedora 34.  I've also run these patches
through the internal test suite on many more targets (and in
particular for the second patch, some big-endian targets).

Tom

[PATCH 1/2] Fix bug in Ada packed array handling

[Tom Tromey]

A user found a bug where an array of packed arrays was printed
incorrectly.  The bug here is that the packed array has a bit stride,
but the outer array does not -- and should not.  However,
update_static_array_size does not distinguish between an array of
packed arrays and a multi-dimensional packed array, and for the
latter, only the innermost array will end up with a stride.

This patch fixes the problem by adding a flag to indicate whether a
given array type is a constituent of a multi-dimensional array.
---
 gdb/dwarf2/read.c                         |  4 ++++
 gdb/gdbtypes.c                            |  1 +
 gdb/gdbtypes.h                            | 21 +++++++++++++++++++++
 gdb/testsuite/gdb.ada/packed_array.exp    |  6 ++++++
 gdb/testsuite/gdb.ada/packed_array/pa.adb | 14 ++++++++++++++
 5 files changed, 46 insertions(+)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 78f4cc1f60d..116fc59821a 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -15695,6 +15695,7 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
 	{
 	  type = create_array_type_with_stride (NULL, type, range_types[i++],
 						byte_stride_prop, bit_stride);
+	  type->set_is_multi_dimensional (true);
 	  bit_stride = 0;
 	  byte_stride_prop = nullptr;
 	}
@@ -15706,11 +15707,14 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
 	{
 	  type = create_array_type_with_stride (NULL, type, range_types[ndim],
 						byte_stride_prop, bit_stride);
+	  type->set_is_multi_dimensional (true);
 	  bit_stride = 0;
 	  byte_stride_prop = nullptr;
 	}
     }
 
+  /* Clear the flag on the outermost array type.  */
+  type->set_is_multi_dimensional (false);
   gdb_assert (type != element_type);
 
   /* Understand Dwarf2 support for vector types (like they occur on
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 643bb0a14a3..9ddb1f81cc1 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1319,6 +1319,7 @@ update_static_array_size (struct type *type)
 	 wrong size when trying to find elements of the outer
 	 array.  */
       if (element_type->code () == TYPE_CODE_ARRAY
+	  && (stride != 0 || element_type->is_multi_dimensional ())
 	  && element_type->length () != 0
 	  && TYPE_FIELD_BITSIZE (element_type, 0) != 0
 	  && get_array_bounds (element_type, &low_bound, &high_bound)
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 94d4b6684fb..f92b3894a1b 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -926,6 +926,15 @@ struct main_type
 
   unsigned int m_flag_flag_enum : 1;
 
+  /* * For TYPE_CODE_ARRAY, this is true if this type is part of a
+     multi-dimensional array.  Multi-dimensional arrays are
+     represented internally as arrays of arrays, and this flag lets
+     gdb distinguish between multiple dimensions and an ordinary array
+     of arrays.  The flag is set on each inner dimension, but not the
+     outermost dimension.  */
+
+  unsigned int m_multi_dimensional : 1;
+
   /* * A discriminant telling us which field of the type_specific
      union is being used for this type, if any.  */
 
@@ -1349,6 +1358,18 @@ struct type
     this->main_type->m_flag_flag_enum = is_flag_enum;
   }
 
+  /* True if this array type is part of a multi-dimensional array.  */
+
+  bool is_multi_dimensional () const
+  {
+    return this->main_type->m_multi_dimensional;
+  }
+
+  void set_is_multi_dimensional (bool value)
+  {
+    this->main_type->m_multi_dimensional = value;
+  }
+
   /* * Assuming that THIS is a TYPE_CODE_FIXED_POINT, return a reference
      to this type's fixed_point_info.  */
 
diff --git a/gdb/testsuite/gdb.ada/packed_array.exp b/gdb/testsuite/gdb.ada/packed_array.exp
index eb857f7e564..df34f31348a 100644
--- a/gdb/testsuite/gdb.ada/packed_array.exp
+++ b/gdb/testsuite/gdb.ada/packed_array.exp
@@ -57,4 +57,10 @@ foreach_with_prefix scenario {all minimal} {
 	    xfail $test
 	}
     }
+
+    set line "(4 => true, false, true, false, true)"
+    gdb_test "print o_var" \
+	[string_to_regexp " = ($line, $line, $line, $line)"]
+    gdb_test "print o2_var" \
+	[string_to_regexp " = ($line, $line, $line, $line)"]
 }
diff --git a/gdb/testsuite/gdb.ada/packed_array/pa.adb b/gdb/testsuite/gdb.ada/packed_array/pa.adb
index 2955f986b00..bb6a2acd388 100644
--- a/gdb/testsuite/gdb.ada/packed_array/pa.adb
+++ b/gdb/testsuite/gdb.ada/packed_array/pa.adb
@@ -27,6 +27,20 @@ procedure PA is
 
    U_Var : Unconstrained_Packed_Array (1 .. Ident (6));
 
+   -- Note that this array is not packed.
+   type Outer_Array is array (1 .. 4) of Packed_Array;
+   O_Var : Outer_Array := ((true, false, true, false, true),
+                           (true, false, true, false, true),
+                           (true, false, true, false, true),
+                           (true, false, true, false, true));
+
+   type Outer_Array2 is array (1 .. 4) of Packed_Array;
+   pragma pack (Outer_Array2);
+   O2_Var : Outer_Array2 := ((true, false, true, false, true),
+                           (true, false, true, false, true),
+                           (true, false, true, false, true),
+                           (true, false, true, false, true));
+
 begin
 
    Var := (True, False, True, False, True);
-- 
2.34.3

[PATCH 2/2] Fix crash in value_print_array_elements

[Tom Tromey]

A user noticed that gdb would crash when printing a packed array after
doing "set lang c".  Packed arrays don't exist in C, but it's
occasionally useful to print things in C mode when working in a non-C
language -- this lets you see under the hood a little bit.

The bug here is that generic value printing does not handle packed
arrays at all.  This patch fixes the bug by introducing a new function
to extract a value from a bit offset and width.

The new function includes a hack to avoid problems with some existing
test cases when using -fgnat-encodings=all.  Cleaning up this code
looked difficult, and since "all" is effectively deprecated, I thought
it made sense to simply work around the problems.
---
 gdb/testsuite/gdb.ada/packed_array.exp | 13 +++++
 gdb/valprint.c                         | 20 ++++---
 gdb/value.c                            | 79 ++++++++++++++++++++++++++
 gdb/value.h                            | 21 +++++++
 4 files changed, 126 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/gdb.ada/packed_array.exp b/gdb/testsuite/gdb.ada/packed_array.exp
index df34f31348a..e73298ec84c 100644
--- a/gdb/testsuite/gdb.ada/packed_array.exp
+++ b/gdb/testsuite/gdb.ada/packed_array.exp
@@ -63,4 +63,17 @@ foreach_with_prefix scenario {all minimal} {
 	[string_to_regexp " = ($line, $line, $line, $line)"]
     gdb_test "print o2_var" \
 	[string_to_regexp " = ($line, $line, $line, $line)"]
+
+    # This is a regression test for a crash with
+    # -fgnat-encodings=minimal, and with 'all' the output is unusual,
+    # so restrict the test.
+    if {$scenario == "minimal"} {
+	set line "{true, false, true, false, true}"
+
+	gdb_test "set lang c" \
+	    "Warning: the current language does not match this frame."
+	gdb_test "print o2_var" \
+	    [string_to_regexp " = {$line, $line, $line, $line}"] \
+	    "print o2_var in c mode"
+    }
 }
diff --git a/gdb/valprint.c b/gdb/valprint.c
index 91a59419c4e..266a76dbdc4 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -1928,7 +1928,6 @@ value_print_array_elements (struct value *val, struct ui_file *stream,
   unsigned int things_printed = 0;
   unsigned len;
   struct type *elttype, *index_type;
-  unsigned eltlen;
   /* Position of the array element we are examining to see
      whether it is repeated.  */
   unsigned int rep1;
@@ -1939,7 +1938,9 @@ value_print_array_elements (struct value *val, struct ui_file *stream,
   struct type *type = check_typedef (value_type (val));
 
   elttype = type->target_type ();
-  eltlen = type_length_units (check_typedef (elttype));
+  unsigned bit_stride = type->bit_stride ();
+  if (bit_stride == 0)
+    bit_stride = 8 * check_typedef (elttype)->length ();
   index_type = type->index_type ();
   if (index_type->code () == TYPE_CODE_RANGE)
     index_type = index_type->target_type ();
@@ -1988,23 +1989,28 @@ value_print_array_elements (struct value *val, struct ui_file *stream,
       maybe_print_array_index (index_type, i + low_bound,
 			       stream, options);
 
+      struct value *element = value_from_component_bitsize (val, elttype,
+							    bit_stride * i,
+							    bit_stride);
       rep1 = i + 1;
       reps = 1;
       /* Only check for reps if repeat_count_threshold is not set to
 	 UINT_MAX (unlimited).  */
       if (options->repeat_count_threshold < UINT_MAX)
 	{
-	  while (rep1 < len
-		 && value_contents_eq (val, i * eltlen,
-				       val, rep1 * eltlen,
-				       eltlen))
+	  while (rep1 < len)
 	    {
+	      struct value *rep_elt
+		= value_from_component_bitsize (val, elttype,
+						rep1 * bit_stride,
+						bit_stride);
+	      if (!value_contents_eq (element, rep_elt))
+		break;
 	      ++reps;
 	      ++rep1;
 	    }
 	}
 
-      struct value *element = value_from_component (val, elttype, eltlen * i);
       common_val_print (element, stream, recurse + 1, options,
 			current_language);
 
diff --git a/gdb/value.c b/gdb/value.c
index 8ed941f3749..53b35af5620 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -916,6 +916,17 @@ value_contents_eq (const struct value *val1, LONGEST offset1,
 				 length * TARGET_CHAR_BIT);
 }
 
+/* See value.h.  */
+
+bool
+value_contents_eq (const struct value *val1, const struct value *val2)
+{
+  ULONGEST len1 = check_typedef (value_enclosing_type (val1))->length ();
+  ULONGEST len2 = check_typedef (value_enclosing_type (val2))->length ();
+  if (len1 != len2)
+    return false;
+  return value_contents_eq (val1, 0, val2, 0, len1);
+}
 
 /* The value-history records all the values printed by print commands
    during this session.  */
@@ -1368,6 +1379,43 @@ value_contents_copy_raw (struct value *dst, LONGEST dst_offset,
 			      bit_length);
 }
 
+/* A helper for value_from_component_bitsize that copies bits from SRC
+   to DEST.  */
+
+static void
+value_contents_copy_raw_bitwise (struct value *dst, LONGEST dst_bit_offset,
+				 struct value *src, LONGEST src_bit_offset,
+				 LONGEST bit_length)
+{
+  /* A lazy DST would make that this copy operation useless, since as
+     soon as DST's contents were un-lazied (by a later value_contents
+     call, say), the contents would be overwritten.  A lazy SRC would
+     mean we'd be copying garbage.  */
+  gdb_assert (!dst->lazy && !src->lazy);
+
+  /* The overwritten DST range gets unavailability ORed in, not
+     replaced.  Make sure to remember to implement replacing if it
+     turns out actually necessary.  */
+  LONGEST dst_offset = dst_bit_offset / TARGET_CHAR_BIT;
+  LONGEST length = bit_length / TARGET_CHAR_BIT;
+  gdb_assert (value_bytes_available (dst, dst_offset, length));
+  gdb_assert (!value_bits_any_optimized_out (dst, dst_bit_offset,
+					     bit_length));
+
+  /* Copy the data.  */
+  gdb::array_view<gdb_byte> dst_contents = value_contents_all_raw (dst);
+  gdb::array_view<const gdb_byte> src_contents = value_contents_all_raw (src);
+  copy_bitwise (dst_contents.data (), dst_bit_offset,
+		src_contents.data (), src_bit_offset,
+		bit_length,
+		type_byte_order (value_type (src)) == BFD_ENDIAN_BIG);
+
+  /* Copy the meta-data.  */
+  value_ranges_copy_adjusted (dst, dst_bit_offset,
+			      src, src_bit_offset,
+			      bit_length);
+}
+
 /* Copy LENGTH bytes of SRC value's (all) contents
    (value_contents_all) starting at SRC_OFFSET byte, into DST value's
    (all) contents, starting at DST_OFFSET.  If unavailable contents
@@ -3774,6 +3822,37 @@ value_from_component (struct value *whole, struct type *type, LONGEST offset)
   return v;
 }
 
+/* See value.h.  */
+
+struct value *
+value_from_component_bitsize (struct value *whole, struct type *type,
+			      LONGEST bit_offset, LONGEST bit_length)
+{
+  gdb_assert (!value_lazy (whole));
+
+  /* Preserve lvalue-ness if possible.  This is needed to avoid
+     array-printing failures (including crashes) when printing Ada
+     arrays in programs compiled with -fgnat-encodings=all.  */
+  if ((bit_offset % TARGET_CHAR_BIT) == 0
+      && (bit_length % TARGET_CHAR_BIT) == 0
+      && bit_length == TARGET_CHAR_BIT * type->length ())
+    return value_from_component (whole, type, bit_offset / TARGET_CHAR_BIT);
+
+  struct value *v = allocate_value (type);
+
+  LONGEST dst_offset = TARGET_CHAR_BIT * value_embedded_offset (v);
+  if (is_scalar_type (type) && type_byte_order (type) == BFD_ENDIAN_BIG)
+    dst_offset += TARGET_CHAR_BIT * type->length () - bit_length;
+
+  value_contents_copy_raw_bitwise (v, dst_offset,
+				   whole,
+				   TARGET_CHAR_BIT
+				   * value_embedded_offset (whole)
+				   + bit_offset,
+				   bit_length);
+  return v;
+}
+
 struct value *
 coerce_ref_if_computed (const struct value *arg)
 {
diff --git a/gdb/value.h b/gdb/value.h
index 52752df1f4c..57a7724388b 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -599,6 +599,12 @@ extern bool value_contents_eq (const struct value *val1, LONGEST offset1,
 			       const struct value *val2, LONGEST offset2,
 			       LONGEST length);
 
+/* An overload of value_contents_eq that compares the entirety of both
+   values.  */
+
+extern bool value_contents_eq (const struct value *val1,
+			       const struct value *val2);
+
 /* Read LENGTH addressable memory units starting at MEMADDR into BUFFER,
    which is (or will be copied to) VAL's contents buffer offset by
    BIT_OFFSET bits.  Marks value contents ranges as unavailable if
@@ -687,6 +693,21 @@ extern struct value *value_from_history_ref (const char *, const char **);
 extern struct value *value_from_component (struct value *, struct type *,
 					   LONGEST);
 
+
+/* Create a new value by extracting it from WHOLE.  TYPE is the type
+   of the new value.  BIT_OFFSET and BIT_LENGTH describe the offset
+   and field width of the value to extract from WHOLE -- BIT_LENGTH
+   may differ from TYPE's length in the case where WHOLE's type is
+   packed.
+
+   When the value does come from a non-byte-aligned offset or field
+   width, it will be marked non_lval.  */
+
+extern struct value *value_from_component_bitsize (struct value *whole,
+						   struct type *type,
+						   LONGEST bit_offset,
+						   LONGEST bit_length);
+
 extern struct value *value_at (struct type *type, CORE_ADDR addr);
 extern struct value *value_at_lazy (struct type *type, CORE_ADDR addr);
 
-- 
2.34.3

Re: [PATCH 0/2] Fix two bugs with packed array printing

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> This series fixes a couple of bugs involving the printing of packed
Tom> arrays.

Tom> Regression tested on x86-64 Fedora 34.  I've also run these patches
Tom> through the internal test suite on many more targets (and in
Tom> particular for the second patch, some big-endian targets).

I'm checking these in now.

Tom