[3,PR,gdb/16959] gdb hangs in infinite recursion
Commit Message
The original problem was fixed (see related PR 22242). But using a typedef
as the declared type for a static member variable, as commented in this PR,
is still causing gdb to get into infinite loop when printing the static
member's value. This problem can be reproduced as follows:
% cat t.cc
class A {
typedef A type;
public:
bool operator==(const type& other) { return true; }
static const type INSTANCE;
};
const A A::INSTANCE;
int main() {
A a;
if (a == A::INSTANCE) {
return -1;
}
return 0;
}
% g++ -g t.cc
% gdb -ex "start" -ex "p a" a.out
The fix is rather trivial - in cp_print_static_field(), should call
check_typedef() to get the static member's real type and use it to
check whether it's a struct or an array.
As Simon suggested, I've added a new test case to the testsuite
and am passing the original type, not the real type, as argument
to function val_print().
Re-tested on both aarch64-linux-gnu and amd64-linux-gnu. No regressions.
---
---
gdb/ChangeLog | 7 ++++
gdb/cp-valprint.c | 8 ++--
gdb/testsuite/ChangeLog | 5 +++
gdb/testsuite/gdb.cp/static-typedef-print.cc | 35 +++++++++++++++++++++
gdb/testsuite/gdb.cp/static-typedef-print.exp | 40 +++++++++++++++++++++++++
5 files changed, 91 insertions(+), 4 deletions(-)
create mode 100644 gdb/testsuite/gdb.cp/static-typedef-print.cc
create mode 100644 gdb/testsuite/gdb.cp/static-typedef-print.exp
Comments
Hi Weimin,
The commit title should state what the patch does/changes, not what the problem
is. So it could be "Fix infinite recursion when printing static type", or
something like that.
Also, when applying your patch, I get this:
.git/rebase-apply/patch:20: trailing whitespace.
* cp-valprint.c: (cp_print_static_field) Use check_typedef() to get
.git/rebase-apply/patch:21: trailing whitespace.
static member's real type for TYPE_CODE_STRUCT and TYPE_CODE_ARRAY
.git/rebase-apply/patch:22: trailing whitespace.
comparisons.
.git/rebase-apply/patch:112: new blank line at EOF.
+
warning: 4 lines add whitespace errors.
Could you fix those?
> diff --git a/gdb/testsuite/gdb.cp/static-typedef-print.exp b/gdb/testsuite/gdb.cp/static-typedef-print.exp
> new file mode 100644
> index 0000000..e0da0c9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/static-typedef-print.exp
> @@ -0,0 +1,40 @@
> +# Copyright 2018 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/>.
> +
> +if { [skip_cplus_tests] } { continue }
> +
> +standard_testfile .cc
> +
> +if [get_compiler_info "c++"] {
> + return -1
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
> + return -1
> +}
> +
> +clean_restart $testfile
> +
> +if ![runto_main] {
> + untested "could not run to main"
> + return -1
> +}
> +
> +gdb_test "print a" \
> + "static INSTANCE = <same as static member of an already seen type>}}.*" \
> + "print static member"
> +
> +gdb_exit
> +return 0
You can remove these last two lines.
Some boring administrative stuff:
If you plan on submitting more patches, it would be a good idea for you to have
an account on sourceware.org, so that you can push your patches once they are
approved. If so, you can use the following form to request an account:
https://sourceware.org/cgi-bin/pdw/ps_form.cgi
You can put me as the person that referred you.
Also, from what I can see, Oracle has a blanket copyright assignment, so you
don't need one specifically for you, is that right?
Simon
On 03/23/2018 09:25 PM, Weimin Pan wrote:
> - if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
> + struct type *real_type = check_typedef (type);
> + if (TYPE_CODE (real_type) == TYPE_CODE_STRUCT)
> {
> CORE_ADDR *first_dont_print;
> CORE_ADDR addr;
> @@ -658,15 +659,14 @@ cp_print_static_field (struct type *type,
> addr = value_address (val);
> obstack_grow (&dont_print_statmem_obstack, (char *) &addr,
> sizeof (CORE_ADDR));
> - type = check_typedef (type);
> - cp_print_value_fields (type, value_enclosing_type (val),
> + cp_print_value_fields (real_type, value_enclosing_type (val),
This is still passing the resolved type down instead of the
original type.
Pedro Alves
On 2018-03-24 06:10, Pedro Alves wrote:
> On 03/23/2018 09:25 PM, Weimin Pan wrote:
>
>> - if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
>> + struct type *real_type = check_typedef (type);
>> + if (TYPE_CODE (real_type) == TYPE_CODE_STRUCT)
>> {
>> CORE_ADDR *first_dont_print;
>> CORE_ADDR addr;
>> @@ -658,15 +659,14 @@ cp_print_static_field (struct type *type,
>> addr = value_address (val);
>> obstack_grow (&dont_print_statmem_obstack, (char *) &addr,
>> sizeof (CORE_ADDR));
>> - type = check_typedef (type);
>> - cp_print_value_fields (type, value_enclosing_type (val),
>> + cp_print_value_fields (real_type, value_enclosing_type (val),
>
> This is still passing the resolved type down instead of the
> original type.
I did not point this out because cp_print_value_fields does a
check_typedef anyway, so it doesn't change anything. But it's true that
to be consistent it would be better to always pass down the original
type, and let the callee decide whether it wants to resolve the typedef
or not. Please consider this comment when preparing the next version.
Thanks,
Simon
On 3/24/2018 5:33 AM, Simon Marchi wrote:
> On 2018-03-24 06:10, Pedro Alves wrote:
>> On 03/23/2018 09:25 PM, Weimin Pan wrote:
>>
>>> - if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
>>> + struct type *real_type = check_typedef (type);
>>> + if (TYPE_CODE (real_type) == TYPE_CODE_STRUCT)
>>> {
>>> CORE_ADDR *first_dont_print;
>>> CORE_ADDR addr;
>>> @@ -658,15 +659,14 @@ cp_print_static_field (struct type *type,
>>> addr = value_address (val);
>>> obstack_grow (&dont_print_statmem_obstack, (char *) &addr,
>>> sizeof (CORE_ADDR));
>>> - type = check_typedef (type);
>>> - cp_print_value_fields (type, value_enclosing_type (val),
>>> + cp_print_value_fields (real_type, value_enclosing_type (val),
>>
>> This is still passing the resolved type down instead of the
>> original type.
>
> I did not point this out because cp_print_value_fields does a
> check_typedef anyway, so it doesn't change anything. But it's true
> that to be consistent it would be better to always pass down the
> original type, and let the callee decide whether it wants to resolve
> the typedef or not. Please consider this comment when preparing the
> next version.
>
Please note the check_typedef() call (now redundant and removed) before
calling cp_print_value_fields().
So passing the resolved type is correct.
Thanks,
Weimin
> Thanks,
>
> Simon
On 2018-03-24 14:54, Wei-min Pan wrote:
> Please note the check_typedef() call (now redundant and removed)
> before calling cp_print_value_fields().
> So passing the resolved type is correct.
I think it's better if functions don't assume too much what other
functions need (original type vs real type). cp_print_value_fields may
not require the real type today, but maybe that will change one day, and
that call will have to be changed (if we even notice it). So in that
sense, it's better to always pass down the original type and let
functions get the real type for their own use if they need it.
Simon
On 3/24/2018 12:23 PM, Simon Marchi wrote:
> On 2018-03-24 14:54, Wei-min Pan wrote:
>> Please note the check_typedef() call (now redundant and removed)
>> before calling cp_print_value_fields().
>> So passing the resolved type is correct.
>
> I think it's better if functions don't assume too much what other
> functions need (original type vs real type). cp_print_value_fields may
> not require the real type today, but maybe that will change one day,
> and that call will have to be changed (if we even notice it). So in
> that sense, it's better to always pass down the original type and let
> functions get the real type for their own use if they need it.
>
> Simon
The original code is like:
- type = check_typedef (type);
- cp_print_value_fields (type, value_enclosing_type (val),
So it's passing the real type. Do you think that we still need to pass
the original type?
Weimin
HI Simon,
On 3/23/2018 7:41 PM, Simon Marchi wrote:
> Hi Weimin,
>
> The commit title should state what the patch does/changes, not what the problem
> is. So it could be "Fix infinite recursion when printing static type", or
> something like that.
I see, done.
> Also, when applying your patch, I get this:
>
> .git/rebase-apply/patch:20: trailing whitespace.
> * cp-valprint.c: (cp_print_static_field) Use check_typedef() to get
> .git/rebase-apply/patch:21: trailing whitespace.
> static member's real type for TYPE_CODE_STRUCT and TYPE_CODE_ARRAY
> .git/rebase-apply/patch:22: trailing whitespace.
> comparisons.
> .git/rebase-apply/patch:112: new blank line at EOF.
> +
> warning: 4 lines add whitespace errors.
>
>
> Could you fix those?
I have fixed the first three warnings which complained the trailing
whitespace
but couldn't figure out what caused the fourth one - new blank line at EOF?
>> diff --git a/gdb/testsuite/gdb.cp/static-typedef-print.exp b/gdb/testsuite/gdb.cp/static-typedef-print.exp
>> new file mode 100644
>> index 0000000..e0da0c9
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/static-typedef-print.exp
>> @@ -0,0 +1,40 @@
>> +# Copyright 2018 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/>.
>> +
>> +if { [skip_cplus_tests] } { continue }
>> +
>> +standard_testfile .cc
>> +
>> +if [get_compiler_info "c++"] {
>> + return -1
>> +}
>> +
>> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
>> + return -1
>> +}
>> +
>> +clean_restart $testfile
>> +
>> +if ![runto_main] {
>> + untested "could not run to main"
>> + return -1
>> +}
>> +
>> +gdb_test "print a" \
>> + "static INSTANCE = <same as static member of an already seen type>}}.*" \
>> + "print static member"
>> +
>> +gdb_exit
>> +return 0
> You can remove these last two lines.
Done.
> Some boring administrative stuff:
>
> If you plan on submitting more patches, it would be a good idea for you to have
> an account on sourceware.org, so that you can push your patches once they are
> approved. If so, you can use the following form to request an account:
>
> https://sourceware.org/cgi-bin/pdw/ps_form.cgi
>
> You can put me as the person that referred you.
Will give it a try. Thanks.
>
> Also, from what I can see, Oracle has a blanket copyright assignment, so you
> don't need one specifically for you, is that right?
I think that's correct. Will double-check.
Weimin
>
> Simon
On 2018-03-24 15:32, Wei-min Pan wrote:
> The original code is like:
>
> - type = check_typedef (type);
> - cp_print_value_fields (type, value_enclosing_type (val),
>
> So it's passing the real type. Do you think that we still need to pass
> the original type?
As I explained, I think it would be a good general rule/convention. And
since cp_print_value_fields does it's own check_typedef, it shouldn't
change the behavior.
Simon
On 2018-03-24 15:40, Wei-min Pan wrote:
> I have fixed the first three warnings which complained the trailing
> whitespace
> but couldn't figure out what caused the fourth one - new blank line at
> EOF?
I think it's an extra blank line at the end of static-typedef-print.cc.
Simon
On 3/24/2018 12:51 PM, Simon Marchi wrote:
> On 2018-03-24 15:40, Wei-min Pan wrote:
>> I have fixed the first three warnings which complained the trailing
>> whitespace
>> but couldn't figure out what caused the fourth one - new blank line
>> at EOF?
>
> I think it's an extra blank line at the end of static-typedef-print.cc.
>
> Simon
Just submitted a revised patch that took out the blank line at the end of
both static-typedef-print.cc and static-typedef-print.exp.
Thanks,
Weimin
@@ -1,3 +1,10 @@
+2018-02-07 Weimin Pan <weimin.pan@oracle.com>
+
+ PR gdb/16959
+ * cp-valprint.c: (cp_print_static_field) Use check_typedef() to get
+ static member's real type for TYPE_CODE_STRUCT and TYPE_CODE_ARRAY
+ comparisons.
+
2018-01-24 Pedro Alves <palves@redhat.com>
GCC PR libstdc++/83906
@@ -633,7 +633,8 @@ cp_print_static_field (struct type *type,
return;
}
- if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
+ struct type *real_type = check_typedef (type);
+ if (TYPE_CODE (real_type) == TYPE_CODE_STRUCT)
{
CORE_ADDR *first_dont_print;
CORE_ADDR addr;
@@ -658,15 +659,14 @@ cp_print_static_field (struct type *type,
addr = value_address (val);
obstack_grow (&dont_print_statmem_obstack, (char *) &addr,
sizeof (CORE_ADDR));
- type = check_typedef (type);
- cp_print_value_fields (type, value_enclosing_type (val),
+ cp_print_value_fields (real_type, value_enclosing_type (val),
value_embedded_offset (val), addr,
stream, recurse, val,
options, NULL, 1);
return;
}
- if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
+ if (TYPE_CODE (real_type) == TYPE_CODE_ARRAY)
{
struct type **first_dont_print;
int i;
@@ -1,3 +1,8 @@
+2018-03-20 Weimin Pan <weimin.pan@oracle.com>
+
+ * gdb.cp/static-typedef-print.exp: New file.
+ * gdb.cp/static-typedef-print.cc: New file.
+
2018-01-22 Pedro Alves <palves@redhat.com>
Sergio Durigan Junior <sergiodj@redhat.com>
new file mode 100644
@@ -0,0 +1,35 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2018 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/>. */
+
+class A {
+ typedef A type;
+public:
+ bool operator==(const type& other) { return true; }
+
+ static const type INSTANCE;
+};
+
+const A A::INSTANCE = {};
+
+int main() {
+ A a;
+ if (a == A::INSTANCE) {
+ return -1;
+ }
+ return 0;
+}
+
new file mode 100644
@@ -0,0 +1,40 @@
+# Copyright 2018 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/>.
+
+if { [skip_cplus_tests] } { continue }
+
+standard_testfile .cc
+
+if [get_compiler_info "c++"] {
+ return -1
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
+ return -1
+}
+
+clean_restart $testfile
+
+if ![runto_main] {
+ untested "could not run to main"
+ return -1
+}
+
+gdb_test "print a" \
+ "static INSTANCE = <same as static member of an already seen type>}}.*" \
+ "print static member"
+
+gdb_exit
+return 0