Fix broken recursion detection when printing static members

  The fix shrinks the stack using obstack_blank_fast() with a negative value as described at the bottom of this page: https://gcc.gnu.org/onlinedocs/libiberty/Extra-Fast-Growing.html
"You can use obstack_blank_fast 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. Earlier versions of obstacks allowed you to use obstack_blank to shrink objects. This will no longer work."

A unit test (gdb.cp/printstaticrecursion.exp) was added. No new regression has been observed in testsuite/gdb.cp/*.exp.
---
 gdb/cp-valprint.c                             |  9 ++-----
 gdb/testsuite/gdb.cp/printstaticrecursion.cc  | 18 +++++++++++++
 gdb/testsuite/gdb.cp/printstaticrecursion.exp | 37 +++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/printstaticrecursion.cc
 create mode 100644 gdb/testsuite/gdb.cp/printstaticrecursion.exp
  

I am so sorry, looks like git send-email removed the first N lines. Here is
the complete changelog:

Fix broken recursion detection when printing static members

Recursion detection for static members was broken. The implementation uses
a growing (and shrinking) obstack object to simulate a stack of addresses
(CORE_ADDR). Pushing addresses is implemented by calling obstack_grow(),
while popping is implemented by calling obstack_free(). The latter is
problematic because obstack_free() expects a pointer to the base of an
object. When popping elements of the stack however, obstack_free() was
called with the new top, which potentially is not the same as the base of
the stack. This is unintended use and the effect is that obstack->next_free
and obstack->object_base members are assigned the value of the new top,
which equals an empty stack. Summary: popping elements would always result
in an empty stack, which breaks the recursion detection.

The fix shrinks the stack using obstack_blank_fast() with a negative value
as described at the bottom of this page:
https://gcc.gnu.org/onlinedocs/libiberty/Extra-Fast-Growing.html
"You can use obstack_blank_fast 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. Earlier versions
of obstacks allowed you to use obstack_blank to shrink objects. This will
no longer work."

A unit test (gdb.cp/printstaticrecursion.exp) was added. No new regression
has been observed in testsuite/gdb.cp/*.exp.



On Tue, Oct 24, 2017 at 12:55 PM, Patrick Frants <osscontribute@gmail.com>
wrote:

> The fix shrinks the stack using obstack_blank_fast() with a negative value
> as described at the bottom of this page: https://gcc.gnu.org/
> onlinedocs/libiberty/Extra-Fast-Growing.html
> "You can use obstack_blank_fast 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. Earlier versions
> of obstacks allowed you to use obstack_blank to shrink objects. This will
> no longer work."
>
> A unit test (gdb.cp/printstaticrecursion.exp) was added. No new
> regression has been observed in testsuite/gdb.cp/*.exp.
> ---
>  gdb/cp-valprint.c                             |  9 ++-----
>  gdb/testsuite/gdb.cp/printstaticrecursion.cc  | 18 +++++++++++++
>  gdb/testsuite/gdb.cp/printstaticrecursion.exp | 37
> +++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+), 7 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 fb9bfd904f..927273f8a2 100644
> --- a/gdb/cp-valprint.c
> +++ b/gdb/cp-valprint.c
> @@ -371,13 +371,8 @@ 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);
> +             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 0000000000..40fd3c0c27
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/printstaticrecursion.cc
> @@ -0,0 +1,18 @@
> +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 0000000000..a71adc7d03
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/printstaticrecursion.exp
> @@ -0,0 +1,37 @@
> +# 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"
> --
> 2.14.0
>
>
  

On 2017-10-24 06:55, Patrick Frants wrote:
> The fix shrinks the stack using obstack_blank_fast() with a negative
> value as described at the bottom of this page:
> https://gcc.gnu.org/onlinedocs/libiberty/Extra-Fast-Growing.html
> "You can use obstack_blank_fast 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.
> Earlier versions of obstacks allowed you to use obstack_blank to
> shrink objects. This will no longer work."
> 
> A unit test (gdb.cp/printstaticrecursion.exp) was added. No new
> regression has been observed in testsuite/gdb.cp/*.exp.

As mentioned in my last review, did you have a chance to see look if it 
was possible to improve the existing test about recursive static fields 
in gdb.cp/classes.exp, rather than introducing a new one?

Thanks,

Simon
  

On 2017-10-24 06:59, Patrick Frants wrote:
> I am so sorry, looks like git send-email removed the first N lines. 
> Here is
> the complete changelog:
> 
> Fix broken recursion detection when printing static members
> 
> Recursion detection for static members was broken. The implementation 
> uses
> a growing (and shrinking) obstack object to simulate a stack of 
> addresses
> (CORE_ADDR). Pushing addresses is implemented by calling 
> obstack_grow(),
> while popping is implemented by calling obstack_free(). The latter is
> problematic because obstack_free() expects a pointer to the base of an
> object. When popping elements of the stack however, obstack_free() was
> called with the new top, which potentially is not the same as the base 
> of
> the stack. This is unintended use and the effect is that 
> obstack->next_free
> and obstack->object_base members are assigned the value of the new top,
> which equals an empty stack. Summary: popping elements would always 
> result
> in an empty stack, which breaks the recursion detection.
> 
> The fix shrinks the stack using obstack_blank_fast() with a negative 
> value
> as described at the bottom of this page:
> https://gcc.gnu.org/onlinedocs/libiberty/Extra-Fast-Growing.html
> "You can use obstack_blank_fast 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. Earlier 
> versions
> of obstacks allowed you to use obstack_blank to shrink objects. This 
> will
> no longer work."

Thanks for the nice description!

Simon
  

I merged the test into test_static_members() in classes.exp. Patch has been submitted.

Thanks

Verstuurd vanaf mijn iPhone

> Op 25 okt. 2017 om 04:23 heeft Simon Marchi <simon.marchi@polymtl.ca> het volgende geschreven:
> 
>> On 2017-10-24 06:55, Patrick Frants wrote:
>> The fix shrinks the stack using obstack_blank_fast() with a negative
>> value as described at the bottom of this page:
>> https://gcc.gnu.org/onlinedocs/libiberty/Extra-Fast-Growing.html
>> "You can use obstack_blank_fast 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.
>> Earlier versions of obstacks allowed you to use obstack_blank to
>> shrink objects. This will no longer work."
>> A unit test (gdb.cp/printstaticrecursion.exp) was added. No new
>> regression has been observed in testsuite/gdb.cp/*.exp.
> 
> As mentioned in my last review, did you have a chance to see look if it was possible to improve the existing test about recursive static fields in gdb.cp/classes.exp, rather than introducing a new one?
> 
> Thanks,
> 
> Simon