[3,PR,gdb/16959] gdb hangs in infinite recursion

  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
  

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