[PATCH][PR gdb/18706] Calculate size of array of stubbed type

Wed Apr 29 16:58:26 GMT 2020

 Am Mittwoch, 29. April 2020, 18:39:47 MESZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

> On 2020-04-29 8:47 a.m., Hannes Domani via Gdb-patches wrote:
> >  Am Dienstag, 28. April 2020, 17:10:29 MESZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:
> >
> >> On 2020-04-27 7:41 a.m., Hannes Domani via Gdb-patches wrote:
> >>> Sizes of stubbed types are calculated on demand in check_typedef, so the same
> >>> must also be done for arrays of stubbed types.
> >>
> >> For the uninitiated, can you please explain what's happening here, which types
> >> are involved, which ones are stubs, and what that means.  That would help me
> >> understand what happens here and be confident that the patch is correct.
> >
> > Something like?:
> >
> > A stubbed type is usually a structure that has only been forward declared, but
> > can also happen if the structure has a virtual function that's not inline in
> > the class definition.
> >
> > For these stubbed types, the size must be recalculated once the full definition
> > is available.
>
> I'm mostly interested in understanding how we get there.
>
> In the CU for stub-array-size.cc, we have:
>
> 0x0000002d:  DW_TAG_array_type
>                 DW_AT_type      (0x00000044 "A")
>                 DW_AT_sibling  (0x0000003d)
>
> which points to
>
> 0x00000044:  DW_TAG_structure_type
>                 DW_AT_name      ("A")
>                 DW_AT_declaration      (true)
>
> I understand that this creates a stub type for `struct A`, since we only
> see a declaration.  We then create a type that is an array of a stub type.
> So the `struct type` we have created for the array points to a `struct type`
> that has no size information, is that right?
>
> The other CU, for stub-array-size2.cc, contains an actual definition for
> the structure type:
>
> 0x0000012e:  DW_TAG_structure_type
>                 DW_AT_name      ("A")
>                 DW_AT_byte_size (0x08)
>                 DW_AT_decl_file ("/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.cp/stub-array-size.h")
>                 DW_AT_decl_line (18)
>                 DW_AT_decl_column      (0x08)
>                 DW_AT_containing_type  (0x0000012e)
>                 DW_AT_sibling  (0x00000167)
>
> For your code to work (and it appears to work, I just want to understand),
> it means that at some point, we changed the target of the array type to
> point to a `struct type` that does have size information.  I'm guessing
> using the information from the other CU.  I'm wondering when/how this happens.

It happens a few lines above the array stuff:

  if (TYPE_TARGET_STUB (type))
    {
      struct type *target_type = check_typedef (TYPE_TARGET_TYPE (type));

At this point, the struct has TYPE_STUB set, so check_typedef replaces the
stub with the complete type.

> >>> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> >>> index 157b3c5e61..5e9de2ccb1 100644
> >>> --- a/gdb/gdbtypes.c
> >>> +++ b/gdb/gdbtypes.c
> >>> @@ -2637,6 +2637,22 @@ check_typedef (struct type *type)
> >>>         TYPE_LENGTH (type) = TYPE_LENGTH (target_type);
> >>>         TYPE_TARGET_STUB (type) = 0;
> >>>       }
> >>> +      else if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> >>> +          && TYPE_NFIELDS (type) == 1)
> >>
> >> Why is the `TYPE_NFIELDS (type) == 1` check necessary here?
> >
> > This is de define of TYPE_INDEX_TYPE:
> > #define TYPE_INDEX_TYPE(type) TYPE_FIELD_TYPE (type, 0)
> >
> > So I assumed I had to check if the field exists before using it.
> >
> > This check is not always done (resolve_dynamic_array_or_string), sometimes
> > with gdb_assert (is_dynamic_type_internal), or like I did
> > (is_scalar_type_recursive).
> >
> > I don't know which is the most correct way.
>
> If it is an invariant that all `struct type` that are TYPE_CODE_ARRAY
> have exactly one field (to store the index type), then we don't need
> to check it here.  If we stumble on an array type that does not have
> one field, it means there's a bug somewhere else in GDB, but this code
> here should not try to compensate / work around that.
>
> It might make sense to assert for for it, to make sure that the invariant
> is respected:
>
>   gdb_assert (TYPE_NFIELDS (type) == 1);
>
> But it would be silly to do this everywhere we use TYPE_INDEX_TYPE.
> Instead, I could see this macro being replaced with a `struct type`
> method that does it:
>
> struct type *
> type::get_index_type ()
> {
>   gdb_assert (TYPE_NFIELDS (this) == 1);
>
>   return ...
> }
>
> That's of course outside the scope of this patch.  But all this to say
> that if the `TYPE_NFIELDS (type) == 1` check is only there to check
> something that's always supposed to be true, it's not necessary, and/or
> should be an assertion.
>
> If there are valid cases where we have an array type with NFIELDS != 1,
> then forget about all this.  But looking at create_array_type_with_stride,
> I don't think there are.
>
> >>> +    {
> >>> +      struct type *range_type = check_typedef (TYPE_INDEX_TYPE (type));
> >>> +      if (TYPE_CODE (range_type) == TYPE_CODE_RANGE
> >>> +          && has_static_range (TYPE_RANGE_DATA (range_type)))
> >>
> >> Is it possible to have an array type where the index type is not TYPE_CODE_RANGE?
> >>
> >> If it is not, then we should assert `TYPE_CODE (range_type) == TYPE_CODE_RANGE`.  If it
> >> can be of another type, what are the other possibilities?
> >
> > Again here, this check is done differently in various functions.
> >
> > Either gdb_assert (resolve_dynamic_range), or explicit check
> > (is_scalar_type_recursive).
> >
> > If I see these different approaches, I tend to use the safest myself.
>
> Same argument as above for NFIELDS.
>
> It might look silly, but I think it's important to use the right one, because
> as a reader,
>
>   if (TYPE_CODE (range_type) == TYPE_CODE_RANGE)
>
> and
>
>   gdb_assert (TYPE_CODE (range_type) == TYPE_CODE_RANGE)
>
> don't communicate the same thing to me.  If we use the first one
> when in reality the index types of arrays always are TYPE_CODE_RANGE,
> then it makes me continue with the idea that things are more complex
> than they are in reality.  Using the second one helps reduce the
> cognitive load, because it tells me there aren't other cases to
> consider.
>
> Looking at the various callers of create_array_type_with_stride and
> create_array_type, I think that index types are always TYPE_CODE_RANGE.

OK, I will remove both the TYPE_NFIELDS and TYPE_CODE_RANGE checks.

> >>> diff --git a/gdb/testsuite/gdb.cp/stub-array-size.exp b/gdb/testsuite/gdb.cp/stub-array-size.exp
> >>> new file mode 100644
> >>> index 0000000000..cb486cd459
> >>> --- /dev/null
> >>> +++ b/gdb/testsuite/gdb.cp/stub-array-size.exp
> >>> @@ -0,0 +1,27 @@
> >>> +# 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/>.
> >>> +
> >>> +# This file is part of the gdb testsuite.
> >>
> >>
> >> Please add a short introductory comment that explains what this intends to test.
> >
> > Like?:
> >
> > Test size of arrays of stubbed types (structures where the full definition is
> > not immediately available).
>
> Yes, that's perfect, thanks.  It's just enough to save the trouble to the reader
> to read an interpret the test to know what this is about.

Hannes