Fix faulty use of obstack_free() to *shrink* dont_print_statmem_obstack. Instead use obstack_blank_fast() with a "negative" size. A real stack data structured would be appropriate here. Added unit test gdb/testsuite/gdb.cp/printstaticrecursion.exp.

  Recursion detection for static members was not working because when popping addresses of the stack, the stack was unintentionally cleared, leaving the stack empty. This was caused by a call to obstack_next_free(), which frees the current object.
The fix uses obstack_blank_fast with a negative size to shrink the current object with the desired amount.
A unit test (gdb.cp/printstaticrecursion.exp) was added. No new regression has been observed in testsuite/gdb.cp/*.exp.
---
 gdb/cp-valprint.c                             | 11 +++-----
 gdb/testsuite/gdb.cp/printstaticrecursion.cc  | 21 +++++++++++++++
 gdb/testsuite/gdb.cp/printstaticrecursion.exp | 39 +++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/printstaticrecursion.cc
 create mode 100644 gdb/testsuite/gdb.cp/printstaticrecursion.exp
  

I am 99% sure the use of obstack_free() on line 390 (also cp-valprint.c)
is used in exactly the same way (shrinking) and is therefore also wrong.
However, I don't have a unit test for that case.

On Thu, Oct 19, 2017 at 11:29 AM, Patrick Frants <osscontribute@gmail.com>
wrote:

> Recursion detection for static members was not working because when
> popping addresses of the stack, the stack was unintentionally cleared,
> leaving the stack empty. This was caused by a call to obstack_next_free(),
> which frees the current object.
> The fix uses obstack_blank_fast with a negative size to shrink the current
> object with the desired amount.
> A unit test (gdb.cp/printstaticrecursion.exp) was added. No new
> regression has been observed in testsuite/gdb.cp/*.exp.
> ---
>  gdb/cp-valprint.c                             | 11 +++-----
>  gdb/testsuite/gdb.cp/printstaticrecursion.cc  | 21 +++++++++++++++
>  gdb/testsuite/gdb.cp/printstaticrecursion.exp | 39
> +++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 8 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.cp/printstaticrecursion.cc
>  create mode 100644 gdb/testsuite/gdb.cp/printstaticrecursion.exp
>
> diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
> index fb9bfd9..8f9658d 100644
> --- a/gdb/cp-valprint.c
> +++ b/gdb/cp-valprint.c
> @@ -370,14 +370,9 @@ cp_print_value_fields (struct type *type, struct type
> *real_type,
>
>           if (obstack_final_size > statmem_obstack_initial_size)
>             {
> -             /* In effect, a pop of the printed-statics stack.  */
> -
> -             void *free_to_ptr =
> -               (char *) obstack_next_free (&dont_print_statmem_obstack) -
> -               (obstack_final_size - statmem_obstack_initial_size);
> -
> -             obstack_free (&dont_print_statmem_obstack,
> -                           free_to_ptr);
> +          /* In effect, a pop of the printed-statics stack.  */
> +          size_t shrink_bytes = statmem_obstack_initial_size -
> obstack_final_size;
> +          obstack_blank_fast(&dont_print_statmem_obstack, shrink_bytes);
>             }
>
>           if (last_set_recurse != recurse)
> diff --git a/gdb/testsuite/gdb.cp/printstaticrecursion.cc
> b/gdb/testsuite/gdb.cp/printstaticrecursion.cc
> new file mode 100644
> index 0000000..616c00d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/printstaticrecursion.cc
> @@ -0,0 +1,21 @@
> +#include <cstdio>
> +
> +struct Inner
> +{
> +    static Inner instance;
> +};
> +
> +
> +struct Outer
> +{
> +    Inner inner;
> +    static Outer instance;
> +};
> +
> +Inner Inner::instance;
> +Outer Outer::instance;
> +
> +int main()
> +{
> +    return 0; /* break-here */
> +}
> \ No newline at end of file
> diff --git a/gdb/testsuite/gdb.cp/printstaticrecursion.exp
> b/gdb/testsuite/gdb.cp/printstaticrecursion.exp
> new file mode 100644
> index 0000000..1407c12
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/printstaticrecursion.exp
> @@ -0,0 +1,39 @@
> +# Copyright 2008-2017 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 printstaticrecursion.cc
> +
> +# Create and source the file that provides information about the compiler
> +# used to compile the test case.
> +if [get_compiler_info "c++"] {
> +    untested "couldn't find a valid c++ compiler"
> +    return -1
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug
> c++}]} {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    perror "couldn't run to main"
> +    continue
> +}
> +
> +gdb_test "print Outer::instance" "{inner = {static instance = {static
> instance = <same as static member of an already seen type>}}, static
> instance = {inner = {static instance = {static instance = <same as static
> member of an already seen type>}}, static instance = <same as static member
> of an already seen type>}}" "Print recursive static member"
> +gdb_exit
> +return 0
> --
> 1.8.3.1
>
>
  

Hi Patrick,

When creating your commit, please make sure you put the subject (something
short but descriptive) on the first line and follow with an empty line.
git-send-email will use that as the subject of the email.

On 2017-10-19 05:29 AM, Patrick Frants wrote:
> Recursion detection for static members was not working because when popping addresses of the stack, the stack was unintentionally cleared, leaving the stack empty. This was caused by a call to obstack_next_free(), which frees the current object.

I am not very familiar with how obstacks work, but I don't understand what you mean.
How does calling obstack_next_free frees the current object, since it only does this:

/* Pointer to next byte not yet allocated in current chunk.  */

#define obstack_next_free(h)    ((h)->next_free)

From what I read about obstack_free, it sounds like the current code is doing
the same thing as the code in this patch.  It computes how far to go back and then
frees everything past that.  That should effectively reset obstack->next_free to its
original position at function entry.  Where is it going wrong?

> The fix uses obstack_blank_fast with a negative size to shrink the current object with the desired amount.
> A unit test (gdb.cp/printstaticrecursion.exp) was added. No new regression has been observed in testsuite/gdb.cp/*.exp.

There is already a test for "same as static member of an already seen type" in
gdb.cp/classes.exp.  Could you check why it doesn't catch this bug?  Could you
also check if it would be feasible/easy to improve that test, instead of creating
a new test file?

> ---
>  gdb/cp-valprint.c                             | 11 +++-----
>  gdb/testsuite/gdb.cp/printstaticrecursion.cc  | 21 +++++++++++++++
>  gdb/testsuite/gdb.cp/printstaticrecursion.exp | 39 +++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 8 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.cp/printstaticrecursion.cc
>  create mode 100644 gdb/testsuite/gdb.cp/printstaticrecursion.exp
> 
> diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
> index fb9bfd9..8f9658d 100644
> --- a/gdb/cp-valprint.c
> +++ b/gdb/cp-valprint.c
> @@ -370,14 +370,9 @@ cp_print_value_fields (struct type *type, struct type *real_type,
>  
>  	  if (obstack_final_size > statmem_obstack_initial_size)
>  	    {
> -	      /* In effect, a pop of the printed-statics stack.  */
> -
> -	      void *free_to_ptr =
> -		(char *) obstack_next_free (&dont_print_statmem_obstack) -
> -		(obstack_final_size - statmem_obstack_initial_size);
> -
> -	      obstack_free (&dont_print_statmem_obstack,
> -			    free_to_ptr);
> +          /* In effect, a pop of the printed-statics stack.  */
> +          size_t shrink_bytes = statmem_obstack_initial_size - obstack_final_size;
> +          obstack_blank_fast(&dont_print_statmem_obstack, shrink_bytes);

The indentation should be 1 tab + 6 spaces.

>  	    }
>  
>  	  if (last_set_recurse != recurse)

The code below that (which seems to be handling a similar situation, but for arrays) uses
obstack_next_free as well.  Is there the same problem there?

> diff --git a/gdb/testsuite/gdb.cp/printstaticrecursion.cc b/gdb/testsuite/gdb.cp/printstaticrecursion.cc
> new file mode 100644
> index 0000000..616c00d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/printstaticrecursion.cc
> @@ -0,0 +1,21 @@
> +#include <cstdio>

You can remove this include.

> +
> +struct Inner
> +{
> +    static Inner instance;

In the test.c file, please use standard GNU code style/indentation (2 columns) (unless the
purpose of the test is related to how the code is formatted).

> +};
> +
> +
> +struct Outer
> +{
> +    Inner inner;
> +    static Outer instance;
> +};
> +
> +Inner Inner::instance;
> +Outer Outer::instance;
> +
> +int main()
> +{
> +    return 0; /* break-here */
> +}
> \ No newline at end of file
> diff --git a/gdb/testsuite/gdb.cp/printstaticrecursion.exp b/gdb/testsuite/gdb.cp/printstaticrecursion.exp
> new file mode 100644
> index 0000000..1407c12
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/printstaticrecursion.exp
> @@ -0,0 +1,39 @@
> +# Copyright 2008-2017 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 printstaticrecursion.cc
> +
> +# Create and source the file that provides information about the compiler
> +# used to compile the test case.
> +if [get_compiler_info "c++"] {
> +    untested "couldn't find a valid c++ compiler"
> +    return -1
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    perror "couldn't run to main"
> +    continue
> +} 
> +
> +gdb_test "print Outer::instance" "{inner = {static instance = {static instance = <same as static member of an already seen type>}}, static instance = {inner = {static instance = {static instance = <same as static member of an already seen type>}}, static instance = <same as static member of an already seen type>}}" "Print recursive static member"

The test name should begin with a lowercase letter, "print recursive static member".

> +gdb_exit
> +return 0
> 

You don't need gdb_exit / return 0 at the end.

Thanks!

Simon
  

On 2017-10-23 12:09 PM, Simon Marchi wrote:
>> diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
>> index fb9bfd9..8f9658d 100644
>> --- a/gdb/cp-valprint.c
>> +++ b/gdb/cp-valprint.c
>> @@ -370,14 +370,9 @@ cp_print_value_fields (struct type *type, struct type *real_type,
>>  
>>  	  if (obstack_final_size > statmem_obstack_initial_size)
>>  	    {
>> -	      /* In effect, a pop of the printed-statics stack.  */
>> -
>> -	      void *free_to_ptr =
>> -		(char *) obstack_next_free (&dont_print_statmem_obstack) -
>> -		(obstack_final_size - statmem_obstack_initial_size);
>> -
>> -	      obstack_free (&dont_print_statmem_obstack,
>> -			    free_to_ptr);
>> +          /* In effect, a pop of the printed-statics stack.  */
>> +          size_t shrink_bytes = statmem_obstack_initial_size - obstack_final_size;

Hmm, size_t is unsigned, maybe it would be better to use ssize_t?

>> +          obstack_blank_fast(&dont_print_statmem_obstack, shrink_bytes);
> 
> The indentation should be 1 tab + 6 spaces.
> 
>>  	    }
>>  
>>  	  if (last_set_recurse != recurse)
> 
> The code below that (which seems to be handling a similar situation, but for arrays) uses
> obstack_next_free as well.  Is there the same problem there?

Never mind about this, I saw your other message after reading this one.

Simon
  

On 2017-10-23 01:17 PM, Simon Marchi wrote:
> On 2017-10-23 12:09 PM, Simon Marchi wrote:
>>> diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
>>> index fb9bfd9..8f9658d 100644
>>> --- a/gdb/cp-valprint.c
>>> +++ b/gdb/cp-valprint.c
>>> @@ -370,14 +370,9 @@ cp_print_value_fields (struct type *type, struct type *real_type,
>>>  
>>>  	  if (obstack_final_size > statmem_obstack_initial_size)
>>>  	    {
>>> -	      /* In effect, a pop of the printed-statics stack.  */
>>> -
>>> -	      void *free_to_ptr =
>>> -		(char *) obstack_next_free (&dont_print_statmem_obstack) -
>>> -		(obstack_final_size - statmem_obstack_initial_size);
>>> -
>>> -	      obstack_free (&dont_print_statmem_obstack,
>>> -			    free_to_ptr);
>>> +          /* In effect, a pop of the printed-statics stack.  */
>>> +          size_t shrink_bytes = statmem_obstack_initial_size - obstack_final_size;
> 
> Hmm, size_t is unsigned, maybe it would be better to use ssize_t?
> 
>>> +          obstack_blank_fast(&dont_print_statmem_obstack, shrink_bytes);
>>
>> The indentation should be 1 tab + 6 spaces.
>>
>>>  	    }
>>>  
>>>  	  if (last_set_recurse != recurse)
>>
>> The code below that (which seems to be handling a similar situation, but for arrays) uses
>> obstack_next_free as well.  Is there the same problem there?
> 
> Never mind about this, I saw your other message after reading this one.
> 
> Simon
> 

I've stepped over the obstack_free call and noticed something strange, it changes the
value of obstack::object_base...

(top-gdb) p dont_print_statmem_obstack
$6 = {chunk_size = 256, chunk = 0x3b8b5a0, object_base = 0x3b8b5b0 "2\020`",
  next_free = 0x3b8sb5c0 "0\270\270\003", chunk_limit = 0x3b8b6a0 "p\265\270\003", temp = {i = 0,
    p = 0x0}, alignment_mask = 15, chunkfun = {plain = 0x798604 <xmalloc(size_t)>,
    extra = 0x798604 <xmalloc(size_t)>}, freefun = {plain = 0x798732 <xfree(void*)>,
    extra = 0x798732 <xfree(void*)>}, extra_arg = 0x0, use_extra_arg = 0, maybe_empty_object = 0,
  alloc_failed = 0}
(top-gdb) p free_to_ptr
$7 = (void *) 0x3b8b5b8
(top-gdb) n
383		  if (last_set_recurse != recurse)
(top-gdb) p dont_print_statmem_obstack
$8 = {chunk_size = 256, chunk = 0x3b8b5a0, object_base = 0x3b8b5b8 "1\020`",
  next_free = 0x3b8b5b8 "1\020`", chunk_limit = 0x3b8b6a0 "p\265\270\003", temp = {i = 0,
    p = 0x0}, alignment_mask = 15, chunkfun = {plain = 0x798604 <xmalloc(size_t)>,
    extra = 0x798604 <xmalloc(size_t)>}, freefun = {plain = 0x798732 <xfree(void*)>,
    extra = 0x798732 <xfree(void*)>}, extra_arg = 0x0, use_extra_arg = 0, maybe_empty_object = 0,
  alloc_failed = 0}


As you can see, object_base goes from 0x3b8b5b0 to 0x3b8b5b8.  And indeed, looking at obstack_free,
I see:

       if (__obj > (void *) __o->chunk && __obj < (void *) __o->chunk_limit)  \
	 __o->next_free = __o->object_base = (char *) __obj;		      \

So when you free, it resets object_base to that point... why does it do that?  It doesn't make sense
to me.

At least, that seems to explain why the obstack is empty after having called
obstack_free, even though we didn't ask it to free the whole obstack.

Simon
  

The obstack_free() interface is pretty bad. It actually expects a pointer to an existing base (of an object) and not a pointer in the middle of a growing object.

Verstuurd vanaf mijn iPhone

> Op 23 okt. 2017 om 21:37 heeft Simon Marchi <simon.marchi@ericsson.com> het volgende geschreven:
> 
> obstack_free
  

On 2017-10-23 03:37 PM, Simon Marchi wrote:
> I've stepped over the obstack_free call and noticed something strange, it changes the
> value of obstack::object_base...
> 
> (top-gdb) p dont_print_statmem_obstack
> $6 = {chunk_size = 256, chunk = 0x3b8b5a0, object_base = 0x3b8b5b0 "2\020`",
>   next_free = 0x3b8sb5c0 "0\270\270\003", chunk_limit = 0x3b8b6a0 "p\265\270\003", temp = {i = 0,
>     p = 0x0}, alignment_mask = 15, chunkfun = {plain = 0x798604 <xmalloc(size_t)>,
>     extra = 0x798604 <xmalloc(size_t)>}, freefun = {plain = 0x798732 <xfree(void*)>,
>     extra = 0x798732 <xfree(void*)>}, extra_arg = 0x0, use_extra_arg = 0, maybe_empty_object = 0,
>   alloc_failed = 0}
> (top-gdb) p free_to_ptr
> $7 = (void *) 0x3b8b5b8
> (top-gdb) n
> 383		  if (last_set_recurse != recurse)
> (top-gdb) p dont_print_statmem_obstack
> $8 = {chunk_size = 256, chunk = 0x3b8b5a0, object_base = 0x3b8b5b8 "1\020`",
>   next_free = 0x3b8b5b8 "1\020`", chunk_limit = 0x3b8b6a0 "p\265\270\003", temp = {i = 0,
>     p = 0x0}, alignment_mask = 15, chunkfun = {plain = 0x798604 <xmalloc(size_t)>,
>     extra = 0x798604 <xmalloc(size_t)>}, freefun = {plain = 0x798732 <xfree(void*)>,
>     extra = 0x798732 <xfree(void*)>}, extra_arg = 0x0, use_extra_arg = 0, maybe_empty_object = 0,
>   alloc_failed = 0}
> 
> 
> As you can see, object_base goes from 0x3b8b5b0 to 0x3b8b5b8.  And indeed, looking at obstack_free,
> I see:
> 
>        if (__obj > (void *) __o->chunk && __obj < (void *) __o->chunk_limit)  \
> 	 __o->next_free = __o->object_base = (char *) __obj;		      \
> 
> So when you free, it resets object_base to that point... why does it do that?  It doesn't make sense
> to me.
> 
> At least, that seems to explain why the obstack is empty after having called
> obstack_free, even though we didn't ask it to free the whole obstack.
> 
> Simon
> 

After following a crash course about obstacks on IRC (thanks Tom!), I understood that obstack_free
is normally used to free completed objects.  object_base points to the beginning of the object
currently being constructed, so obstack_free sets it to the beginning of the newly freed area,
so that the next object to be constructed will be placed there.

In the libc manual [1], it is stated clearly that obstack_blank should be used with a negative
size to make the current object smaller:

  You can use obstack_blank with a negative size argument to make the current object smaller.
  Just don’t try to shrink it beyond zero length—there’s no telling what will happen if you
  do that.

For negative sizes, obstack_blank seems equivalent to obstack_blank_fast.  So I am now
pretty much convinced that your fix is good, in that it does what the original code
intended (but failed) to do.  Could you address the comments from my previous mails
and submit a new version?  While at it, I noticed there is a missing space in:

  obstack_blank_fast(&dont_print_statmem_obstack, shrink_bytes);

before the opening parenthesis.

Thanks!

Simon

[1] https://www.gnu.org/software/libc/manual/html_node/Growing-Objects.html#Growing-Objects
  

On 2017-10-23 15:55, osscontribute wrote:
> The obstack_free() interface is pretty bad. It actually expects a
> pointer to an existing base (of an object) and not a pointer in the
> middle of a growing object.

Oops, I saw that reply too late.  It would be nice if you could mention 
that in the commit log, for anyone in the future wondering why that 
change was made.

Thanks,

Simon