[RFA] internal-error using '@' (repeat) operator on array of dynamic objects

  Hello,

Using the following Ada declarations (the same as in
gdb.ada/dyn_stride.exp)...

      subtype Small_Type is Integer range L .. U;
      type Record_Type (I : Small_Type := L) is record
         S : String (1 .. I);
      end record;
      type Array_Type is array (Integer range <>) of Record_Type;

      A1 : Array_Type :=
        (1 => (I => U, S => (others => ASCII.NUL)),
         2 => (I => 1, S => "A"),
         3 => (I => 2, S => "AB"));

... where "L" and "U" are variables, trying to apply the repeat
operator to "A1(1)" yields to an internal error:

  | (gdb) print a1(1)@3
  | $5 = /[...]/gdbtypes.c:4883: internal-error: type* copy_type(const type*):
  | Assertion `TYPE_OBJFILE_OWNED (type)' failed.

What happens first is that the ada-lang module evaluated the "A1(1)"
sub-expression returning a structure where "I" (one of the fields
in that structure) has a type which is dynamic, because it is
a range type whose bounds are not statically known.

Next, we apply the repeat ('@') operator, which is done via
allocate_repeat_value, which creates an array type with the correct
bounds to associate to our value, by calling lookup_array_range_type:

  | struct type *
  | lookup_array_range_type (struct type *element_type,
  |                          LONGEST low_bound, LONGEST high_bound)
  | {
  |   struct gdbarch *gdbarch = get_type_arch (element_type);
  |   struct type *index_type = builtin_type (gdbarch)->builtin_int;
  |   struct type *range_type
  |     = create_static_range_type (NULL, index_type, low_bound, high_bound);
  |
  |   return create_array_type (NULL, element_type, range_type);
  | }

As we can see, this creates an array type whose index type is
always owned by the gdbarch. This is where the problem lies.

Next, we use that type to construct a struct value. That value
then gets passed to the valprint module, which then checks
whether our object is dynamic or not. And because field "I" above
had a dynamic range type, we end up determining by association
that the artificial repeat array itself is also dynamic. So
we attempt to resolve the type, which leads to trying to copying
that type. And because the artifical array created by
lookup_array_range_type has an index which is not objfile-owned,
we trip the assertion.

This patch fixes the issue by enhancing lookup_array_range_type
to create an index type which has the same owner as the element
type.

gdb/ChangeLog:

        * gdbtypes.c (lookup_array_range_type): Make sure the array's
        index type is objfile-owned if the element type is as well.

gdb/testsuite/ChangeLog:

        * testsuite/gdb.ada/dyn_stride.exp: Add "print a1(1)@3" test.

Tested on x86_64-linux. OK to push?

Thanks,
  

On 2018-01-12 04:50, Joel Brobecker wrote:
> Hello,
> 
> Using the following Ada declarations (the same as in
> gdb.ada/dyn_stride.exp)...
> 
>       subtype Small_Type is Integer range L .. U;
>       type Record_Type (I : Small_Type := L) is record
>          S : String (1 .. I);
>       end record;
>       type Array_Type is array (Integer range <>) of Record_Type;
> 
>       A1 : Array_Type :=
>         (1 => (I => U, S => (others => ASCII.NUL)),
>          2 => (I => 1, S => "A"),
>          3 => (I => 2, S => "AB"));
> 
> ... where "L" and "U" are variables, trying to apply the repeat
> operator to "A1(1)" yields to an internal error:
> 
>   | (gdb) print a1(1)@3
>   | $5 = /[...]/gdbtypes.c:4883: internal-error: type* copy_type(const 
> type*):
>   | Assertion `TYPE_OBJFILE_OWNED (type)' failed.
> 
> What happens first is that the ada-lang module evaluated the "A1(1)"
> sub-expression returning a structure where "I" (one of the fields
> in that structure) has a type which is dynamic, because it is
> a range type whose bounds are not statically known.
> 
> Next, we apply the repeat ('@') operator, which is done via
> allocate_repeat_value, which creates an array type with the correct
> bounds to associate to our value, by calling lookup_array_range_type:
> 
>   | struct type *
>   | lookup_array_range_type (struct type *element_type,
>   |                          LONGEST low_bound, LONGEST high_bound)
>   | {
>   |   struct gdbarch *gdbarch = get_type_arch (element_type);
>   |   struct type *index_type = builtin_type (gdbarch)->builtin_int;
>   |   struct type *range_type
>   |     = create_static_range_type (NULL, index_type, low_bound, 
> high_bound);
>   |
>   |   return create_array_type (NULL, element_type, range_type);
>   | }
> 
> As we can see, this creates an array type whose index type is
> always owned by the gdbarch. This is where the problem lies.
> 
> Next, we use that type to construct a struct value. That value
> then gets passed to the valprint module, which then checks
> whether our object is dynamic or not. And because field "I" above
> had a dynamic range type, we end up determining by association
> that the artificial repeat array itself is also dynamic. So
> we attempt to resolve the type, which leads to trying to copying
> that type. And because the artifical array created by
> lookup_array_range_type has an index which is not objfile-owned,
> we trip the assertion.
> 
> This patch fixes the issue by enhancing lookup_array_range_type
> to create an index type which has the same owner as the element
> type.
> 
> gdb/ChangeLog:
> 
>         * gdbtypes.c (lookup_array_range_type): Make sure the array's
>         index type is objfile-owned if the element type is as well.
> 
> gdb/testsuite/ChangeLog:
> 
>         * testsuite/gdb.ada/dyn_stride.exp: Add "print a1(1)@3" test.
> 
> Tested on x86_64-linux. OK to push?
> 
> Thanks,
> --
> Joel
> 
> ---
>  gdb/gdbtypes.c                       | 13 +++++++++----
>  gdb/testsuite/gdb.ada/dyn_stride.exp | 19 +++++++++++++++++++
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 7ba62df..0a30223 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1194,10 +1194,15 @@ struct type *
>  lookup_array_range_type (struct type *element_type,
>  			 LONGEST low_bound, LONGEST high_bound)
>  {
> -  struct gdbarch *gdbarch = get_type_arch (element_type);
> -  struct type *index_type = builtin_type (gdbarch)->builtin_int;
> -  struct type *range_type
> -    = create_static_range_type (NULL, index_type, low_bound, 
> high_bound);
> +  struct type *index_type;
> +  struct type *range_type;
> +
> +  if (TYPE_OBJFILE_OWNED (element_type))
> +    index_type = objfile_type (TYPE_OWNER 
> (element_type).objfile)->builtin_int;
> +  else
> +    index_type = builtin_type (get_type_arch 
> (element_type))->builtin_int;
> +  range_type = create_static_range_type (NULL, index_type,
> +					 low_bound, high_bound);
> 
>    return create_array_type (NULL, element_type, range_type);
>  }
> diff --git a/gdb/testsuite/gdb.ada/dyn_stride.exp
> b/gdb/testsuite/gdb.ada/dyn_stride.exp
> index 0267ca1..dae5106 100644
> --- a/gdb/testsuite/gdb.ada/dyn_stride.exp
> +++ b/gdb/testsuite/gdb.ada/dyn_stride.exp
> @@ -39,3 +39,22 @@ gdb_test "print A1(3)" \
> 
>  gdb_test "print A1(1..3)" \
>           "\\(\\(i => 0, s => \"\"\\), \\(i => 1, s => \"A\"\\), \\(i
> => 2, s => \"AB\"\\)\\)"
> +
> +# Test the use of the "repeat" operator (@).
> +#
> +# Note that, in this case, the repeat operator makes little sense
> +# and is NOT equivalent to requesting an array slice. In the case
> +# of "print a1(1)@3", the size of each element in the array is
> +# variable from element to element.  So, when asked to repeat
> +# one element of the array a number of times, you're not guaranteed
> +# to get the same answer as in a slice; while the slice will
> +# take into account the array stride, the repeat operator uses
> +# the size of the object being repeated as its stride, which
> +# is often not the same. So, in the test below, the output for
> +# the second and third element is not entirely predictable, because
> +# it reads part of their contents from a memory region which has
> +# an undefined value (the "holes" between the each actual element
> +# of the array).
> +
> +gdb_test "print a1(1)@3" \
> +         " = \\(\\(i => 0, s => \"\"\\), \\(i => -?$decimal, s =>
> .*\\), \\(i => -?$decimal, s => .*\\)\\)"

Hi Joel,

I looked at this, more as a learning experience than anything else, but 
it looks good to me.  So to sum up, because the index_type is created as 
gdbarch owned, the range_type is gdb-arch owned too, and because of that 
the array_type is gdbarch owned too.  Any idea why copy_type only deals 
with objfile-owned types?

Regardless of that, it's probably the right thing to do to make the 
index_type/range_type/array_type have the same owner than the 
element_type they originate from.

Simon
  

> > gdb/ChangeLog:
> > 
> >         * gdbtypes.c (lookup_array_range_type): Make sure the array's
> >         index type is objfile-owned if the element type is as well.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> >         * testsuite/gdb.ada/dyn_stride.exp: Add "print a1(1)@3" test.

> I looked at this, more as a learning experience than anything else, but it
> looks good to me.

Thanks for having taken a look. I just pushed the patch to master.

> Any idea why copy_type only deals with objfile-owned types?

I am not sure, unfortunately.  This comes from a patch pushed in Jul 2009:

    https://www.sourceware.org/ml/gdb-patches/2009-06/msg00741.html
    Subject: [6/7] Introduce per-type architecture

    | The existing type allocation routines alloc_type and init_type
    | now always create per-objfile types; it is now a bug to call
    | them with a NULL objfile parameter.  The helpers init_float_type,
    | init_complex_type, init_flags_type, and init_composite_type are
    | removed (and replaced by per-arch variants).
    |
    | There is new routine alloc_type_arch that allocates a non-
    | objfile related type; you have to specify a (non-NULL) gdbarch
    | argument instead.  A number of helper routines are provided
    | to simplify architecture-specific type creation (arch_type,
    | arch_integer_type, arch_character_type, arch_boolean_type,
    | arch_float_type, arch_complex_type, arch_flags_type, and
    | arch_composite_type).
    |
    | Finally, if you want to allocate a new type with the same
    | ownership information (objfile or architecture) as an existing
    | type, you can use alloc_type_copy to do so.
    |
    | The patch goes through all remaining places in GDB that allocate
    | types and makes sure the appropriate allocation routine is used.

One thing I can say is that, since that patch was pushed, we later on
added some code which assumes the type is objfile-owned:

  if (TYPE_DYN_PROP_LIST (type) != NULL)
    TYPE_DYN_PROP_LIST (new_type)
      = copy_dynamic_prop_list (&TYPE_OBJFILE (type) -> objfile_obstack,
                                TYPE_DYN_PROP_LIST (type));

Interestingly, a contributor sent a patch to remove that assertion,
and I was hoping my patch would fix his problem, but only partially.

https://www.sourceware.org/ml/gdb-patches/2018-01/msg00494.html

He's asking questions about lifetime and implicitly why we can't
mix-and-match ownership types, and besides a vague "I don't think
this is a good idea / this is an added risk / might make it harder
to eliminate one objfile if we want to use a different memory
management system", I don't know what to answer.

I will send a first answer nonetheless. Comments on that thread
always welcome.