PR9167 Derived class static member CRTP infinite recursion on print

PR9167 Derived class static member CRTP infinite recursion on print

[Chris Moller]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

Attached is a patch for this.

The problem was that, at some point in history, pushes and pops to a 
stack implemented in an obstack were done with a sequence that looked like

    base_ptr = obstack_base(...)
    obstack_grow(...)
    obstack_grow(...)
        .
        .
        .
    obstack_free(..., base_ptr)

You can't do that.  obstack_base() returns a pointer to the current 
allocation, which may change depending on what's stuffed into the 
obstack--by the time the obstack_free is hit, the base may have changed.

This patch fixes this.  Unfortunately, there are at least two other 
identical abuses of obstacks in cp-valprint.c alone, one of which I 
introduced myself in the patch to PR9067 (the down-side of 
cut'n'paste...).  I'll fix that in a separate patch.  Also 
unfortunately, this current patch conflicts with the patch I sent out a 
day or two ago for PR10687--I'll reconcile that before committing anything.



[-- Attachment #2: pr9167.patch --]
[-- Type: text/x-patch, Size: 5127 bytes --]

Index: cp-valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-valprint.c,v
retrieving revision 1.64
diff -u -r1.64 cp-valprint.c
--- cp-valprint.c	8 Feb 2010 18:04:16 -0000	1.64
+++ cp-valprint.c	16 Apr 2010 03:24:22 -0000
@@ -157,7 +157,7 @@
   int fields_seen = 0;
 
   CHECK_TYPEDEF (type);
-  
+
   if (recurse == 0
       && obstack_object_size (&dont_print_statmem_obstack) > 0)
     obstack_free (&dont_print_statmem_obstack, NULL);
@@ -180,14 +180,10 @@
     fprintf_filtered (stream, "<No data fields>");
   else
     {
-      void *statmem_obstack_top = NULL;
+      int obstack_initial_size = 0;
       
       if (dont_print_statmem == 0)
-	{
-	  /* Set the current printed-statics stack top.  */
-	  statmem_obstack_top
-	    = obstack_next_free (&dont_print_statmem_obstack);
-	}
+	  obstack_initial_size = obstack_object_size (&dont_print_statmem_obstack);
 
       for (i = n_baseclasses; i < len; i++)
 	{
@@ -307,9 +303,19 @@
 
       if (dont_print_statmem == 0)
 	{
-	  /* In effect, a pop of the printed-statics stack.  */
-	  if (obstack_object_size (&dont_print_statmem_obstack) > 0) 
-	    obstack_free (&dont_print_statmem_obstack, statmem_obstack_top);
+	  int obstack_final_size =
+	    obstack_object_size (&dont_print_statmem_obstack);
+
+	  if (obstack_final_size > obstack_initial_size) {
+	    /* In effect, a pop of the printed-statics stack.  */
+
+	    void * free_to_ptr =
+	      obstack_next_free (&dont_print_statmem_obstack) -
+	      (obstack_final_size - obstack_initial_size);
+
+	    obstack_free (&dont_print_statmem_obstack,
+			  free_to_ptr);
+	  }
 	}
 
       if (options->pretty)
@@ -508,6 +514,7 @@
 		       const struct value_print_options *options)
 {
   struct value_print_options opts;
+
   if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
     {
       CORE_ADDR *first_dont_print;
@@ -533,7 +540,6 @@
       addr = value_address (val);
       obstack_grow (&dont_print_statmem_obstack, (char *) &addr,
 		    sizeof (CORE_ADDR));
-
       CHECK_TYPEDEF (type);
       cp_print_value_fields (type, value_enclosing_type (val),
 			     value_contents_all (val),
Index: testsuite/gdb.cp/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/Makefile.in,v
retrieving revision 1.9
diff -u -r1.9 Makefile.in
--- testsuite/gdb.cp/Makefile.in	8 Feb 2010 18:27:53 -0000	1.9
+++ testsuite/gdb.cp/Makefile.in	16 Apr 2010 03:24:24 -0000
@@ -5,7 +5,7 @@
 	derivation inherit local member-ptr method misc \
         overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace \
 	ref-types ref-params method2 pr9594 gdb2495 virtfunc2 pr9067 \
-	pr1072
+	pr1072 pr9167
 
 all info install-info dvi install uninstall installcheck check:
 	@echo "Nothing to be done for $@..."
Index: testsuite/gdb.cp/pr9167.cc
===================================================================
RCS file: testsuite/gdb.cp/pr9167.cc
diff -N testsuite/gdb.cp/pr9167.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr9167.cc	16 Apr 2010 03:24:25 -0000
@@ -0,0 +1,36 @@
+#include <iostream>
+
+template<typename DATA>
+struct ATB
+{
+    int data;
+    ATB() : data(0) {}
+};
+
+
+template<typename DATA,
+	 typename DerivedType >
+class A : public ATB<DATA>
+{
+public:
+    static DerivedType const DEFAULT_INSTANCE;
+};
+
+template<typename DATA, typename DerivedType>
+const DerivedType A<DATA, DerivedType>::DEFAULT_INSTANCE;
+
+class B : public A<int, B>
+{
+    
+};
+
+int main()
+{
+    B b;
+    // If this if-block is removed then GDB shall
+    // not infinitely recurse when trying to print b.
+
+    return 0;		// marker
+}
+
+
Index: testsuite/gdb.cp/pr9167.exp
===================================================================
RCS file: testsuite/gdb.cp/pr9167.exp
diff -N testsuite/gdb.cp/pr9167.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr9167.exp	16 Apr 2010 03:24:25 -0000
@@ -0,0 +1,31 @@
+#Copyright 2010 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/>.
+
+set testfile pr9167
+set srcfile ${testfile}.cc
+if [prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}] {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return
+}
+
+gdb_breakpoint [gdb_get_line_number "marker"]
+gdb_continue_to_breakpoint "marker"
+
+gdb_test "p b" "{<A<int, B>> = {<ATB<int>> = {data = 0}, static DEFAULT_INSTANCE = <optimized out>}, <No data fields>}"
+

[-- Attachment #3: pr9167-check.diff --]
[-- Type: text/x-patch, Size: 645 bytes --]

1c1
< Test Run By moller on Wed Apr 14 10:53:26 2010
---
> Test Run By moller on Thu Apr 15 23:21:07 2010
12271a12272,12274
> Running ../../../src/gdb/testsuite/gdb.cp/pr9167.exp ...
> PASS: gdb.cp/pr9167.exp: continue to breakpoint: marker
> PASS: gdb.cp/pr9167.exp: p b
16431c16434
< PASS: gdb.threads/watchthreads.exp: disable 3
---
> PASS: gdb.threads/watchthreads.exp: disable 2
16444c16447
< PASS: gdb.threads/watchthreads2.exp: all threads incremented x
---
> KFAIL: gdb.threads/watchthreads2.exp: gdb can drop watchpoints in multithreaded app (PRMS: gdb/10116)
16712c16715
< # of expected passes		15889
---
> # of expected passes		15890

Re: PR9167 Derived class static member CRTP infinite recursion on print

[Tom Tromey]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> You can't do that.  obstack_base() returns a pointer to the current
Chris> allocation, which may change depending on what's stuffed into the
Chris> obstack--by the time the obstack_free is hit, the base may have
Chris> changed.

Why do we need to pop the stack at all?
It seems to me that if we printed something once, during a given call
into val_print, then we should never try to print it again.
Am I missing something?

Chris> +	    void * free_to_ptr =

No space after the "*".

Chris>        obstack_grow (&dont_print_statmem_obstack, (char *) &addr,
Chris>  		    sizeof (CORE_ADDR));
Chris> -
Chris>        CHECK_TYPEDEF (type);

Gratuitious whitespace change.

This is ok with those changes.  Thanks.

Tom

Re: PR9167 Derived class static member CRTP infinite recursion on print

[Chris Moller]

Fixed up and committed.

On 04/20/10 14:46, Tom Tromey wrote:
>>>>>> "Chris" == Chris Moller<cmoller@redhat.com>  writes:
>>>>>>              
>
> Chris>  You can't do that.  obstack_base() returns a pointer to the current
> Chris>  allocation, which may change depending on what's stuffed into the
> Chris>  obstack--by the time the obstack_free is hit, the base may have
> Chris>  changed.
>
> Why do we need to pop the stack at all?
> It seems to me that if we printed something once, during a given call
> into val_print, then we should never try to print it again.
> Am I missing something?
>
> Chris>  +	    void * free_to_ptr =
>
> No space after the "*".
>
> Chris>         obstack_grow (&dont_print_statmem_obstack, (char *)&addr,
> Chris>   		sizeof (CORE_ADDR));
> Chris>  -
> Chris>         CHECK_TYPEDEF (type);
>
> Gratuitious whitespace change.
>
> This is ok with those changes.  Thanks.
>
> Tom
>    

Re: PR9167 Derived class static member CRTP infinite recursion on print

[Chris Moller]

On 04/20/10 14:46, Tom Tromey wrote:
>>>>>> "Chris" == Chris Moller<cmoller@redhat.com>  writes:
>>>>>>              
>
> Chris>  You can't do that.  obstack_base() returns a pointer to the current
> Chris>  allocation, which may change depending on what's stuffed into the
> Chris>  obstack--by the time the obstack_free is hit, the base may have
> Chris>  changed.
>
> Why do we need to pop the stack at all?
> It seems to me that if we printed something once, during a given call
> into val_print, then we should never try to print it again.
> Am I missing something?
>    

You need the pop in situations like:

    class A
    {
         // whatever
    };

    class B
    {
         static A a1[] = ...;
         static A a2[] = ...;
    };

    B b;

In this example, printing a1 of 'b' will stick class A on the stack--if 
you don't pop the stack, a2 won't be printed at all.  What the stack 
does is record, at each stack level, what's been printed.  In this case, 
without the use of the stack, printing the members of a1 would print A, 
which would print the members of a1, which would print A,...