Re: [PATCH] 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.

[Simon Marchi <simon dot marchi at ericsson dot com>]

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