Submitter | Joel Brobecker |
---|---|
Date | Jan. 12, 2018, 9:50 a.m. |
Message ID | <1515750619-102754-1-git-send-email-brobecker@adacore.com> |
Download | mbox | patch |
Permalink | /patch/25362/ |
State | New |
Headers | show |
Comments
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.
Patch
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 => .*\\)\\)"