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

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

[Hannes Domani]

Sizes of stubbed types are calculated on demand in check_typedef, so the same
must also be done for arrays of stubbed types.

gdb/ChangeLog:

2020-04-27  Hannes Domani  <ssbssa@yahoo.de>

	PR gdb/18706
	* gdbtypes.c (check_typedef): Calculate size of array of stubbed type.

gdb/testsuite/ChangeLog:

2020-04-27  Hannes Domani  <ssbssa@yahoo.de>

	PR gdb/18706
	* gdb.cp/stub-array-size.cc: New test.
	* gdb.cp/stub-array-size.exp: New file.
	* gdb.cp/stub-array-size.h: New test.
	* gdb.cp/stub-array-size2.cc: New test.
---
 gdb/gdbtypes.c                           | 16 ++++++++++++++
 gdb/testsuite/gdb.cp/stub-array-size.cc  | 25 ++++++++++++++++++++++
 gdb/testsuite/gdb.cp/stub-array-size.exp | 27 ++++++++++++++++++++++++
 gdb/testsuite/gdb.cp/stub-array-size.h   | 21 ++++++++++++++++++
 gdb/testsuite/gdb.cp/stub-array-size2.cc | 22 +++++++++++++++++++
 5 files changed, 111 insertions(+)
 create mode 100644 gdb/testsuite/gdb.cp/stub-array-size.cc
 create mode 100644 gdb/testsuite/gdb.cp/stub-array-size.exp
 create mode 100644 gdb/testsuite/gdb.cp/stub-array-size.h
 create mode 100644 gdb/testsuite/gdb.cp/stub-array-size2.cc

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)
+	{
+	  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)))
+	    {
+	      ULONGEST len = 0;
+	      LONGEST low_bound = TYPE_LOW_BOUND (range_type);
+	      LONGEST high_bound = TYPE_HIGH_BOUND (range_type);
+	      if (high_bound >= low_bound)
+		len = (high_bound - low_bound + 1) * TYPE_LENGTH (target_type);
+	      TYPE_LENGTH (type) = len;
+	      TYPE_TARGET_STUB (type) = 0;
+	    }
+	}
     }
 
   type = make_qualified_type (type, instance_flags, NULL);
diff --git a/gdb/testsuite/gdb.cp/stub-array-size.cc b/gdb/testsuite/gdb.cp/stub-array-size.cc
new file mode 100644
index 0000000000..281d70de4f
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/stub-array-size.cc
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+#include "stub-array-size.h"
+
+A a[10];
+
+int main()
+{
+  return 0;
+};
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.
+
+if {[skip_cplus_tests]} { continue }
+
+standard_testfile .cc stub-array-size2.cc
+
+if {[prepare_for_testing "failed to prepare" $testfile "$srcfile $srcfile2" \
+	{c++ debug}]} {
+    return -1
+}
+
+gdb_test "print sizeof(a)/sizeof(a\[0\])" "10"
diff --git a/gdb/testsuite/gdb.cp/stub-array-size.h b/gdb/testsuite/gdb.cp/stub-array-size.h
new file mode 100644
index 0000000000..5006633246
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/stub-array-size.h
@@ -0,0 +1,21 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+struct A
+{
+  virtual ~A();
+};
diff --git a/gdb/testsuite/gdb.cp/stub-array-size2.cc b/gdb/testsuite/gdb.cp/stub-array-size2.cc
new file mode 100644
index 0000000000..8eda4bd1dd
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/stub-array-size2.cc
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+#include "stub-array-size.h"
+
+A::~A()
+{
+}
-- 
2.26.2

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

[Simon Marchi]

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.

> 
> gdb/ChangeLog:
> 
> 2020-04-27  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	PR gdb/18706
> 	* gdbtypes.c (check_typedef): Calculate size of array of stubbed type.
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-04-27  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	PR gdb/18706
> 	* gdb.cp/stub-array-size.cc: New test.
> 	* gdb.cp/stub-array-size.exp: New file.
> 	* gdb.cp/stub-array-size.h: New test.
> 	* gdb.cp/stub-array-size2.cc: New test.
> ---
>  gdb/gdbtypes.c                           | 16 ++++++++++++++
>  gdb/testsuite/gdb.cp/stub-array-size.cc  | 25 ++++++++++++++++++++++
>  gdb/testsuite/gdb.cp/stub-array-size.exp | 27 ++++++++++++++++++++++++
>  gdb/testsuite/gdb.cp/stub-array-size.h   | 21 ++++++++++++++++++
>  gdb/testsuite/gdb.cp/stub-array-size2.cc | 22 +++++++++++++++++++
>  5 files changed, 111 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.cp/stub-array-size.cc
>  create mode 100644 gdb/testsuite/gdb.cp/stub-array-size.exp
>  create mode 100644 gdb/testsuite/gdb.cp/stub-array-size.h
>  create mode 100644 gdb/testsuite/gdb.cp/stub-array-size2.cc
> 
> 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?

> +	{
> +	  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?

> 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.

Simon

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

[Hannes Domani]

 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.


> >
> > gdb/ChangeLog:
> >
> > 2020-04-27  Hannes Domani  <ssbssa@yahoo.de>
> >
> >     PR gdb/18706
> >     * gdbtypes.c (check_typedef): Calculate size of array of stubbed type.
> >
> > gdb/testsuite/ChangeLog:
> >
> > 2020-04-27  Hannes Domani  <ssbssa@yahoo.de>
> >
> >     PR gdb/18706
> >     * gdb.cp/stub-array-size.cc: New test.
> >     * gdb.cp/stub-array-size.exp: New file.
> >     * gdb.cp/stub-array-size.h: New test.
> >     * gdb.cp/stub-array-size2.cc: New test.
> > ---
> >  gdb/gdbtypes.c                          | 16 ++++++++++++++
> >  gdb/testsuite/gdb.cp/stub-array-size.cc  | 25 ++++++++++++++++++++++
> >  gdb/testsuite/gdb.cp/stub-array-size.exp | 27 ++++++++++++++++++++++++
> >  gdb/testsuite/gdb.cp/stub-array-size.h  | 21 ++++++++++++++++++
> >  gdb/testsuite/gdb.cp/stub-array-size2.cc | 22 +++++++++++++++++++
> >  5 files changed, 111 insertions(+)
> >  create mode 100644 gdb/testsuite/gdb.cp/stub-array-size.cc
> >  create mode 100644 gdb/testsuite/gdb.cp/stub-array-size.exp
> >  create mode 100644 gdb/testsuite/gdb.cp/stub-array-size.h
> >  create mode 100644 gdb/testsuite/gdb.cp/stub-array-size2.cc
> >
> > 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.


> > +    {
> > +      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.


> > 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).


Hannes

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

[Simon Marchi]

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.

>>> 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.

>>> 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.

Simon

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

[Hannes Domani]

 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

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

[Simon Marchi]

On 2020-04-29 12:58 p.m., Hannes Domani via Gdb-patches wrote:
>> 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.

Ah I see the lookup_symbol call now, thanks.

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

Ok, thanks (although please give it a quick look and make sure you reach the
same conclusion as me).

Simon