[PATCH v2 0/5] Prevent more recursion in python based unwinders

[PATCH v2 0/5] Prevent more recursion in python based unwinders

[Kevin Buettner]

This is an updated series of patches for preventing recursion into
python based unwinders.

Pedro reviewed the earlier series; I've tried to address all of his
concerns in addition to several of my own concerns that I found along
the way.

The earlier patch series may be found here:

https://sourceware.org/ml/gdb-patches/2016-09/msg00391.html

To this series, I've also added a related testsuite patch which I had
before posted as a standalone patch.   This patch has changed slightly;
I discovered and fixed a typo in a comment since I first posted it.

Kevin

Re: [PATCH v2 1/5] Extend test gdb.python/py-recurse-unwind.exp

[Kevin Buettner]

This patch modifies the unwinder (sniffer) defined in
py-recurse-unwind.py so that, depending upon the value of one of its
class variables, it will take different paths through the code,
testing different functionality.

The original test attempted to obtain the value of an undefined
symbol.

This somewhat expanded test checks to see if 'pc' can be read via
gdb.PendingFrame.read_register() and also via gdb.parse_and_eval().

gdb/testsuite/ChangeLog:
    
    	* gdb.python/py-recurse-unwind.c (ccc): Delete.
    	(ccc0, ccc1, ccc2): New functions.
    	(bbb): Call ccc0, ccc1, and ccc2.
    	* gdb.python/py-recurse-unwind.py (TestUnwinder): Add calls
    	to read_register() and gdb.parse_and_eval().  Make each code
    	call a separate case that can be individually tested.
    	* gdb.python/py-recurse-unwind.exp (cont_and_backtrace): New
    	proc. Call cont_and_backtrace for each of the code paths that
    	we want to test in the unwinder.
---
 gdb/testsuite/gdb.python/py-recurse-unwind.c   | 16 ++++++-
 gdb/testsuite/gdb.python/py-recurse-unwind.exp | 61 ++++++++++++++++----------
 gdb/testsuite/gdb.python/py-recurse-unwind.py  | 29 +++++++++---
 3 files changed, 76 insertions(+), 30 deletions(-)

diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.c b/gdb/testsuite/gdb.python/py-recurse-unwind.c
index 02a835a..bd0330a 100644
--- a/gdb/testsuite/gdb.python/py-recurse-unwind.c
+++ b/gdb/testsuite/gdb.python/py-recurse-unwind.c
@@ -18,14 +18,26 @@
 /* This is the test program loaded into GDB by the py-recurse-unwind test.  */
 
 void
-ccc (int arg)
+ccc0 (int arg)
+{
+}
+
+void
+ccc1 (int arg)
+{
+}
+
+void
+ccc2 (int arg)
 {
 }
 
 void
 bbb (int arg)
 {
-  ccc (789);
+  ccc0 (789);
+  ccc1 (789);
+  ccc2 (789);
 }
 
 void
diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.exp b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
index 9629a97..ed5e600 100644
--- a/gdb/testsuite/gdb.python/py-recurse-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
@@ -45,29 +45,44 @@ if ![runto_main] then {
     return 0
 }
 
-gdb_breakpoint "ccc"
 
-gdb_continue_to_breakpoint "ccc"
-
-# If the unwinder is active, the usage count will increment while
-# running to the breakpoint.  Reset it prior to doing the backtrace.
-gdb_test_no_output "python TestUnwinder.reset_count()"
-
-# The python based unwinder should be called a number of times while
-# generating the backtrace, but its sniffer always returns None.  So
-# it doesn't really contribute to generating any of the frames below.
-#
-# But that's okay.  Our goal here is to make sure that GDB doesn't
-# get hung up in potentially infinite recursion when invoking the
-# Python-based unwinder.
-
-gdb_test_sequence "bt"  "backtrace" {
-    "\\r\\n#0 .* ccc \\(arg=789\\) at "
-    "\\r\\n#1 .* bbb \\(arg=456\\) at "
-    "\\r\\n#2 .* aaa \\(arg=123\\) at "
-    "\\r\\n#3 .* main \\(.*\\) at"
+proc cont_and_backtrace { tst func } {
+
+    gdb_breakpoint "$func"
+
+    # We're testing different code paths within the unwinder's sniffer.
+    # Set the current path to be tested here.
+    gdb_test_no_output "python TestUnwinder.set_test(\"$tst\")" \
+                       "set code path within python unwinder to $tst"
+
+    # If the unwinder is active, the usage count will increment while
+    # running to the breakpoint.  Reset it prior to doing the backtrace.
+    gdb_test_no_output "python TestUnwinder.reset_count()" \
+                       "reset count for $tst"
+
+    gdb_continue_to_breakpoint "$func"
+
+    # The python based unwinder should be called a number of times while
+    # generating the backtrace, but its sniffer always returns None.  So
+    # it doesn't really contribute to generating any of the frames below.
+    #
+    # But that's okay.  Our goal here is to make sure that GDB doesn't
+    # get hung up in potentially infinite recursion when invoking the
+    # Python-based unwinder.
+
+    gdb_test_sequence "bt"  "backtrace for $tst" {
+	"\\r\\n#0 .* ccc. \\(arg=789\\) at "
+	"\\r\\n#1 .* bbb \\(arg=456\\) at "
+	"\\r\\n#2 .* aaa \\(arg=123\\) at "
+	"\\r\\n#3 .* main \\(.*\\) at"
+    }
+
+    # Test that the python-based unwinder / sniffer was actually called
+    # during generation of the backtrace.
+    gdb_test "python print(TestUnwinder.count > 0)" "True" \
+             "python unwinder called for $tst"
 }
 
-# Test that the python-based unwinder / sniffer was actually called
-# during generation of the backtrace.
-gdb_test "python print(TestUnwinder.count > 0)" "True"
+cont_and_backtrace "check_undefined_symbol" "ccc0"
+cont_and_backtrace "check_user_reg_pc" "ccc1"
+cont_and_backtrace "check_pae_pc" "ccc2"
diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.py b/gdb/testsuite/gdb.python/py-recurse-unwind.py
index 1da7aca..5eb87bb 100644
--- a/gdb/testsuite/gdb.python/py-recurse-unwind.py
+++ b/gdb/testsuite/gdb.python/py-recurse-unwind.py
@@ -40,13 +40,18 @@ class TestUnwinder(Unwinder):
     def inc_count (cls):
         cls.count += 1
 
+    test = 'check_undefined_symbol'
+
+    @classmethod
+    def set_test (cls, test) :
+       cls.test = test
+
     def __init__(self):
         Unwinder.__init__(self, "test unwinder")
         self.recurse_level = 0
 
     def __call__(self, pending_frame):
 
-
         if self.recurse_level > 0:
             gdb.write("TestUnwinder: Recursion detected - returning early.\n")
             return None
@@ -54,11 +59,25 @@ class TestUnwinder(Unwinder):
         self.recurse_level += 1
         TestUnwinder.inc_count()
 
-        try:
-            val = gdb.parse_and_eval("undefined_symbol")
+        if TestUnwinder.test == 'check_user_reg_pc' :
+
+            pc = pending_frame.read_register('pc')
+            pc_as_int = int(pc.cast(gdb.lookup_type('int')))
+            # gdb.write("In unwinder: pc=%x\n" % pc_as_int)
+
+        elif TestUnwinder.test == 'check_pae_pc' :
+
+            pc = gdb.parse_and_eval('$pc')
+            pc_as_int = int(pc.cast(gdb.lookup_type('int')))
+            # gdb.write("In unwinder: pc=%x\n" % pc_as_int)
+
+        elif TestUnwinder.test == 'check_undefined_symbol' :
+
+            try:
+                val = gdb.parse_and_eval("undefined_symbol")
 
-        except Exception as arg:
-            pass
+            except Exception as arg:
+                pass
 
         self.recurse_level -= 1
 

Re: [PATCH v2 1/5] Extend test gdb.python/py-recurse-unwind.exp

[Pedro Alves]

On 11/02/2016 10:14 PM, Kevin Buettner wrote:
> This patch modifies the unwinder (sniffer) defined in
> py-recurse-unwind.py so that, depending upon the value of one of its
> class variables, it will take different paths through the code,
> testing different functionality.
> 
> The original test attempted to obtain the value of an undefined
> symbol.
> 
> This somewhat expanded test checks to see if 'pc' can be read via
> gdb.PendingFrame.read_register() and also via gdb.parse_and_eval().
> 
> gdb/testsuite/ChangeLog:
>     
>     	* gdb.python/py-recurse-unwind.c (ccc): Delete.
>     	(ccc0, ccc1, ccc2): New functions.
>     	(bbb): Call ccc0, ccc1, and ccc2.
>     	* gdb.python/py-recurse-unwind.py (TestUnwinder): Add calls
>     	to read_register() and gdb.parse_and_eval().  Make each code
>     	call a separate case that can be individually tested.
>     	* gdb.python/py-recurse-unwind.exp (cont_and_backtrace): New
>     	proc. Call cont_and_backtrace for each of the code paths that
>     	we want to test in the unwinder.
> ---
>  gdb/testsuite/gdb.python/py-recurse-unwind.c   | 16 ++++++-
>  gdb/testsuite/gdb.python/py-recurse-unwind.exp | 61 ++++++++++++++++----------
>  gdb/testsuite/gdb.python/py-recurse-unwind.py  | 29 +++++++++---
>  3 files changed, 76 insertions(+), 30 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.c b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> index 02a835a..bd0330a 100644
> --- a/gdb/testsuite/gdb.python/py-recurse-unwind.c
> +++ b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> @@ -18,14 +18,26 @@
>  /* This is the test program loaded into GDB by the py-recurse-unwind test.  */
>  
>  void
> -ccc (int arg)
> +ccc0 (int arg)
> +{
> +}
> +
> +void
> +ccc1 (int arg)
> +{
> +}
> +
> +void
> +ccc2 (int arg)
>  {
>  }
>  
>  void
>  bbb (int arg)
>  {
> -  ccc (789);
> +  ccc0 (789);
> +  ccc1 (789);
> +  ccc2 (789);
>  }
>  

Do we need separate functions?  You could do that with a
single function by making main call the same function more
than once, in a loop or unrolled, so that you don't need to
keep adding functions.  Or do with without continuing the
inferior, even, by using gdb's "flushregs" command.
Or was that to make sure test messages are unique below?

> +proc cont_and_backtrace { tst func } {
> +
> +    gdb_breakpoint "$func"
> +
> +    # We're testing different code paths within the unwinder's sniffer.
> +    # Set the current path to be tested here.
> +    gdb_test_no_output "python TestUnwinder.set_test(\"$tst\")" \
> +                       "set code path within python unwinder to $tst"
> +
> +    # If the unwinder is active, the usage count will increment while
> +    # running to the breakpoint.  Reset it prior to doing the backtrace.
> +    gdb_test_no_output "python TestUnwinder.reset_count()" \
> +                       "reset count for $tst"
> +
> +    gdb_continue_to_breakpoint "$func"
> +
> +    # The python based unwinder should be called a number of times while
> +    # generating the backtrace, but its sniffer always returns None.  So
> +    # it doesn't really contribute to generating any of the frames below.
> +    #
> +    # But that's okay.  Our goal here is to make sure that GDB doesn't
> +    # get hung up in potentially infinite recursion when invoking the
> +    # Python-based unwinder.
> +
> +    gdb_test_sequence "bt"  "backtrace for $tst" {
> +	"\\r\\n#0 .* ccc. \\(arg=789\\) at "
> +	"\\r\\n#1 .* bbb \\(arg=456\\) at "
> +	"\\r\\n#2 .* aaa \\(arg=123\\) at "
> +	"\\r\\n#3 .* main \\(.*\\) at"
> +    }
> +
> +    # Test that the python-based unwinder / sniffer was actually called
> +    # during generation of the backtrace.
> +    gdb_test "python print(TestUnwinder.count > 0)" "True" \
> +             "python unwinder called for $tst"
>  }

I would suggest using "with_test_prefix $tst" instead of manually
adding $tst.  The gdb_breakpoint / gdb_continue_to_breakpoint
calls don't include $tst, and while currently you'll end up with
unique test messages due the unique function names, that seems
like fragility easily avoided.

Otherwise, LGTM.

Thanks,
Pedro Alves

Re: [PATCH v2 1/5] Extend test gdb.python/py-recurse-unwind.exp

[Kevin Buettner]

On Wed, 9 Nov 2016 13:59:13 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 11/02/2016 10:14 PM, Kevin Buettner wrote:
> > diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.c b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> > index 02a835a..bd0330a 100644
> > --- a/gdb/testsuite/gdb.python/py-recurse-unwind.c
> > +++ b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> > @@ -18,14 +18,26 @@
> >  /* This is the test program loaded into GDB by the py-recurse-unwind test.  */
> >  
> >  void
> > -ccc (int arg)
> > +ccc0 (int arg)
> > +{
> > +}
> > +
> > +void
> > +ccc1 (int arg)
> > +{
> > +}
> > +
> > +void
> > +ccc2 (int arg)
> >  {
> >  }
> >  
> >  void
> >  bbb (int arg)
> >  {
> > -  ccc (789);
> > +  ccc0 (789);
> > +  ccc1 (789);
> > +  ccc2 (789);
> >  }
> >    
> 
> Do we need separate functions?  You could do that with a
> single function by making main call the same function more
> than once, in a loop or unrolled, so that you don't need to
> keep adding functions.  Or do with without continuing the
> inferior, even, by using gdb's "flushregs" command.
> Or was that to make sure test messages are unique below?

I don't remember why I added separate functions, though it might have
had something to do with uniqueness.  I don't see much value in it
now, so I added a loop in main() instead.

> > +    # Test that the python-based unwinder / sniffer was actually called
> > +    # during generation of the backtrace.
> > +    gdb_test "python print(TestUnwinder.count > 0)" "True" \
> > +             "python unwinder called for $tst"
> >  }  
> 
> I would suggest using "with_test_prefix $tst" instead of manually
> adding $tst.  The gdb_breakpoint / gdb_continue_to_breakpoint
> calls don't include $tst, and while currently you'll end up with
> unique test messages due the unique function names, that seems
> like fragility easily avoided.

I've done this too.

> Otherwise, LGTM.

Thanks again for the review.

This is what I've pushed...

commit 1a2f3d7ff1d79b1290704e48c71e905b987393a6
Author: Kevin Buettner <kevinb@redhat.com>
Date:   Mon Sep 26 15:00:37 2016 -0700

    Extend test gdb.python/py-recurse-unwind.exp
    
    This patch modifies the unwinder (sniffer) defined in
    py-recurse-unwind.py so that, depending upon the value of one of its
    class variables, it will take different paths through the code,
    testing different functionality.
    
    The original test attempted to obtain the value of an undefined
    symbol.
    
    This somewhat expanded test checks to see if 'pc' can be read via
    gdb.PendingFrame.read_register() and also via gdb.parse_and_eval().
    
    gdb/testsuite/ChangeLog:
    
    	* gdb.python/py-recurse-unwind.c (main): Add loop.
    	* gdb.python/py-recurse-unwind.py (TestUnwinder): Add calls
    	to read_register() and gdb.parse_and_eval().  Make each code
    	call a separate case that can be individually tested.
    	* gdb.python/py-recurse-unwind.exp (cont_and_backtrace): New
    	proc. Call cont_and_backtrace for each of the code paths that
    	we want to test in the unwinder.
---
 gdb/testsuite/ChangeLog                        | 10 ++++
 gdb/testsuite/gdb.python/py-recurse-unwind.c   |  6 ++-
 gdb/testsuite/gdb.python/py-recurse-unwind.exp | 63 ++++++++++++++++----------
 gdb/testsuite/gdb.python/py-recurse-unwind.py  | 29 ++++++++++--
 4 files changed, 79 insertions(+), 29 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index d9e61f4..82126c0 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,13 @@
+2016-11-16  Kevin Buettner  <kevinb@redhat.com>
+
+	* gdb.python/py-recurse-unwind.c (main): Add loop.
+	* gdb.python/py-recurse-unwind.py (TestUnwinder): Add calls
+	to read_register() and gdb.parse_and_eval().  Make each code
+	call a separate case that can be individually tested.
+	* gdb.python/py-recurse-unwind.exp (cont_and_backtrace): New
+	proc. Call cont_and_backtrace for each of the code paths that
+	we want to test in the unwinder.
+
 2016-11-15  Andreas Arnez  <arnez@linux.vnet.ibm.com>
 
 	* gdb.dwarf2/bitfield-parent-optimized-out.exp: Fix DWARF code for
diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.c b/gdb/testsuite/gdb.python/py-recurse-unwind.c
index 02a835a..77acec1 100644
--- a/gdb/testsuite/gdb.python/py-recurse-unwind.c
+++ b/gdb/testsuite/gdb.python/py-recurse-unwind.c
@@ -37,6 +37,10 @@ aaa (int arg)
 int
 main ()
 {
-  aaa (123);
+  int i;
+
+  for (i = 0; i < 10; i++)
+    aaa (123);
+
   return 0;
 }
diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.exp b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
index 9629a97..97c69f7 100644
--- a/gdb/testsuite/gdb.python/py-recurse-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
@@ -45,29 +45,46 @@ if ![runto_main] then {
     return 0
 }
 
-gdb_breakpoint "ccc"
 
-gdb_continue_to_breakpoint "ccc"
-
-# If the unwinder is active, the usage count will increment while
-# running to the breakpoint.  Reset it prior to doing the backtrace.
-gdb_test_no_output "python TestUnwinder.reset_count()"
-
-# The python based unwinder should be called a number of times while
-# generating the backtrace, but its sniffer always returns None.  So
-# it doesn't really contribute to generating any of the frames below.
-#
-# But that's okay.  Our goal here is to make sure that GDB doesn't
-# get hung up in potentially infinite recursion when invoking the
-# Python-based unwinder.
-
-gdb_test_sequence "bt"  "backtrace" {
-    "\\r\\n#0 .* ccc \\(arg=789\\) at "
-    "\\r\\n#1 .* bbb \\(arg=456\\) at "
-    "\\r\\n#2 .* aaa \\(arg=123\\) at "
-    "\\r\\n#3 .* main \\(.*\\) at"
+proc cont_and_backtrace { tst } {
+
+    with_test_prefix $tst {
+	gdb_breakpoint "ccc"
+
+	# We're testing different code paths within the unwinder's sniffer.
+	# Set the current path to be tested here.
+	gdb_test_no_output "python TestUnwinder.set_test(\"$tst\")" \
+			   "set code path within python unwinder to $tst"
+
+	# If the unwinder is active, the usage count will increment while
+	# running to the breakpoint.  Reset it prior to doing the backtrace.
+	gdb_test_no_output "python TestUnwinder.reset_count()" \
+			   "reset count"
+
+	gdb_continue_to_breakpoint "ccc"
+
+	# The python based unwinder should be called a number of times while
+	# generating the backtrace, but its sniffer always returns None.  So
+	# it doesn't really contribute to generating any of the frames below.
+	#
+	# But that's okay.  Our goal here is to make sure that GDB doesn't
+	# get hung up in potentially infinite recursion when invoking the
+	# Python-based unwinder.
+
+	gdb_test_sequence "bt"  "backtrace" {
+	    "\\r\\n#0 .* ccc \\(arg=789\\) at "
+	    "\\r\\n#1 .* bbb \\(arg=456\\) at "
+	    "\\r\\n#2 .* aaa \\(arg=123\\) at "
+	    "\\r\\n#3 .* main \\(.*\\) at"
+	}
+
+	# Test that the python-based unwinder / sniffer was actually called
+	# during generation of the backtrace.
+	gdb_test "python print(TestUnwinder.count > 0)" "True" \
+		 "python unwinder called"
+    }
 }
 
-# Test that the python-based unwinder / sniffer was actually called
-# during generation of the backtrace.
-gdb_test "python print(TestUnwinder.count > 0)" "True"
+cont_and_backtrace "check_undefined_symbol"
+cont_and_backtrace "check_user_reg_pc"
+cont_and_backtrace "check_pae_pc"
diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.py b/gdb/testsuite/gdb.python/py-recurse-unwind.py
index 1da7aca..5eb87bb 100644
--- a/gdb/testsuite/gdb.python/py-recurse-unwind.py
+++ b/gdb/testsuite/gdb.python/py-recurse-unwind.py
@@ -40,13 +40,18 @@ class TestUnwinder(Unwinder):
     def inc_count (cls):
         cls.count += 1
 
+    test = 'check_undefined_symbol'
+
+    @classmethod
+    def set_test (cls, test) :
+       cls.test = test
+
     def __init__(self):
         Unwinder.__init__(self, "test unwinder")
         self.recurse_level = 0
 
     def __call__(self, pending_frame):
 
-
         if self.recurse_level > 0:
             gdb.write("TestUnwinder: Recursion detected - returning early.\n")
             return None
@@ -54,11 +59,25 @@ class TestUnwinder(Unwinder):
         self.recurse_level += 1
         TestUnwinder.inc_count()
 
-        try:
-            val = gdb.parse_and_eval("undefined_symbol")
+        if TestUnwinder.test == 'check_user_reg_pc' :
+
+            pc = pending_frame.read_register('pc')
+            pc_as_int = int(pc.cast(gdb.lookup_type('int')))
+            # gdb.write("In unwinder: pc=%x\n" % pc_as_int)
+
+        elif TestUnwinder.test == 'check_pae_pc' :
+
+            pc = gdb.parse_and_eval('$pc')
+            pc_as_int = int(pc.cast(gdb.lookup_type('int')))
+            # gdb.write("In unwinder: pc=%x\n" % pc_as_int)
+
+        elif TestUnwinder.test == 'check_undefined_symbol' :
+
+            try:
+                val = gdb.parse_and_eval("undefined_symbol")
 
-        except Exception as arg:
-            pass
+            except Exception as arg:
+                pass
 
         self.recurse_level -= 1
 

Re: [PATCH v2 1/5] Extend test gdb.python/py-recurse-unwind.exp

[Sergio Durigan Junior]

On Wednesday, November 16 2016, Kevin Buettner wrote:

>> Otherwise, LGTM.
>
> Thanks again for the review.
>
> This is what I've pushed...

Heya,

While checking some BuildBot logs, I noticed a few new FAILures
introduced by this commit.  You can see them at:

  <https://sourceware.org/ml/gdb-testers/2016-q4/msg03062.html>

Cheers,

> commit 1a2f3d7ff1d79b1290704e48c71e905b987393a6
> Author: Kevin Buettner <kevinb@redhat.com>
> Date:   Mon Sep 26 15:00:37 2016 -0700
>
>     Extend test gdb.python/py-recurse-unwind.exp
>     
>     This patch modifies the unwinder (sniffer) defined in
>     py-recurse-unwind.py so that, depending upon the value of one of its
>     class variables, it will take different paths through the code,
>     testing different functionality.
>     
>     The original test attempted to obtain the value of an undefined
>     symbol.
>     
>     This somewhat expanded test checks to see if 'pc' can be read via
>     gdb.PendingFrame.read_register() and also via gdb.parse_and_eval().
>     
>     gdb/testsuite/ChangeLog:
>     
>     	* gdb.python/py-recurse-unwind.c (main): Add loop.
>     	* gdb.python/py-recurse-unwind.py (TestUnwinder): Add calls
>     	to read_register() and gdb.parse_and_eval().  Make each code
>     	call a separate case that can be individually tested.
>     	* gdb.python/py-recurse-unwind.exp (cont_and_backtrace): New
>     	proc. Call cont_and_backtrace for each of the code paths that
>     	we want to test in the unwinder.
> ---
>  gdb/testsuite/ChangeLog                        | 10 ++++
>  gdb/testsuite/gdb.python/py-recurse-unwind.c   |  6 ++-
>  gdb/testsuite/gdb.python/py-recurse-unwind.exp | 63 ++++++++++++++++----------
>  gdb/testsuite/gdb.python/py-recurse-unwind.py  | 29 ++++++++++--
>  4 files changed, 79 insertions(+), 29 deletions(-)
>
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index d9e61f4..82126c0 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,13 @@
> +2016-11-16  Kevin Buettner  <kevinb@redhat.com>
> +
> +	* gdb.python/py-recurse-unwind.c (main): Add loop.
> +	* gdb.python/py-recurse-unwind.py (TestUnwinder): Add calls
> +	to read_register() and gdb.parse_and_eval().  Make each code
> +	call a separate case that can be individually tested.
> +	* gdb.python/py-recurse-unwind.exp (cont_and_backtrace): New
> +	proc. Call cont_and_backtrace for each of the code paths that
> +	we want to test in the unwinder.
> +
>  2016-11-15  Andreas Arnez  <arnez@linux.vnet.ibm.com>
>  
>  	* gdb.dwarf2/bitfield-parent-optimized-out.exp: Fix DWARF code for
> diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.c b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> index 02a835a..77acec1 100644
> --- a/gdb/testsuite/gdb.python/py-recurse-unwind.c
> +++ b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> @@ -37,6 +37,10 @@ aaa (int arg)
>  int
>  main ()
>  {
> -  aaa (123);
> +  int i;
> +
> +  for (i = 0; i < 10; i++)
> +    aaa (123);
> +
>    return 0;
>  }
> diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.exp b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
> index 9629a97..97c69f7 100644
> --- a/gdb/testsuite/gdb.python/py-recurse-unwind.exp
> +++ b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
> @@ -45,29 +45,46 @@ if ![runto_main] then {
>      return 0
>  }
>  
> -gdb_breakpoint "ccc"
>  
> -gdb_continue_to_breakpoint "ccc"
> -
> -# If the unwinder is active, the usage count will increment while
> -# running to the breakpoint.  Reset it prior to doing the backtrace.
> -gdb_test_no_output "python TestUnwinder.reset_count()"
> -
> -# The python based unwinder should be called a number of times while
> -# generating the backtrace, but its sniffer always returns None.  So
> -# it doesn't really contribute to generating any of the frames below.
> -#
> -# But that's okay.  Our goal here is to make sure that GDB doesn't
> -# get hung up in potentially infinite recursion when invoking the
> -# Python-based unwinder.
> -
> -gdb_test_sequence "bt"  "backtrace" {
> -    "\\r\\n#0 .* ccc \\(arg=789\\) at "
> -    "\\r\\n#1 .* bbb \\(arg=456\\) at "
> -    "\\r\\n#2 .* aaa \\(arg=123\\) at "
> -    "\\r\\n#3 .* main \\(.*\\) at"
> +proc cont_and_backtrace { tst } {
> +
> +    with_test_prefix $tst {
> +	gdb_breakpoint "ccc"
> +
> +	# We're testing different code paths within the unwinder's sniffer.
> +	# Set the current path to be tested here.
> +	gdb_test_no_output "python TestUnwinder.set_test(\"$tst\")" \
> +			   "set code path within python unwinder to $tst"
> +
> +	# If the unwinder is active, the usage count will increment while
> +	# running to the breakpoint.  Reset it prior to doing the backtrace.
> +	gdb_test_no_output "python TestUnwinder.reset_count()" \
> +			   "reset count"
> +
> +	gdb_continue_to_breakpoint "ccc"
> +
> +	# The python based unwinder should be called a number of times while
> +	# generating the backtrace, but its sniffer always returns None.  So
> +	# it doesn't really contribute to generating any of the frames below.
> +	#
> +	# But that's okay.  Our goal here is to make sure that GDB doesn't
> +	# get hung up in potentially infinite recursion when invoking the
> +	# Python-based unwinder.
> +
> +	gdb_test_sequence "bt"  "backtrace" {
> +	    "\\r\\n#0 .* ccc \\(arg=789\\) at "
> +	    "\\r\\n#1 .* bbb \\(arg=456\\) at "
> +	    "\\r\\n#2 .* aaa \\(arg=123\\) at "
> +	    "\\r\\n#3 .* main \\(.*\\) at"
> +	}
> +
> +	# Test that the python-based unwinder / sniffer was actually called
> +	# during generation of the backtrace.
> +	gdb_test "python print(TestUnwinder.count > 0)" "True" \
> +		 "python unwinder called"
> +    }
>  }
>  
> -# Test that the python-based unwinder / sniffer was actually called
> -# during generation of the backtrace.
> -gdb_test "python print(TestUnwinder.count > 0)" "True"
> +cont_and_backtrace "check_undefined_symbol"
> +cont_and_backtrace "check_user_reg_pc"
> +cont_and_backtrace "check_pae_pc"
> diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.py b/gdb/testsuite/gdb.python/py-recurse-unwind.py
> index 1da7aca..5eb87bb 100644
> --- a/gdb/testsuite/gdb.python/py-recurse-unwind.py
> +++ b/gdb/testsuite/gdb.python/py-recurse-unwind.py
> @@ -40,13 +40,18 @@ class TestUnwinder(Unwinder):
>      def inc_count (cls):
>          cls.count += 1
>  
> +    test = 'check_undefined_symbol'
> +
> +    @classmethod
> +    def set_test (cls, test) :
> +       cls.test = test
> +
>      def __init__(self):
>          Unwinder.__init__(self, "test unwinder")
>          self.recurse_level = 0
>  
>      def __call__(self, pending_frame):
>  
> -
>          if self.recurse_level > 0:
>              gdb.write("TestUnwinder: Recursion detected - returning early.\n")
>              return None
> @@ -54,11 +59,25 @@ class TestUnwinder(Unwinder):
>          self.recurse_level += 1
>          TestUnwinder.inc_count()
>  
> -        try:
> -            val = gdb.parse_and_eval("undefined_symbol")
> +        if TestUnwinder.test == 'check_user_reg_pc' :
> +
> +            pc = pending_frame.read_register('pc')
> +            pc_as_int = int(pc.cast(gdb.lookup_type('int')))
> +            # gdb.write("In unwinder: pc=%x\n" % pc_as_int)
> +
> +        elif TestUnwinder.test == 'check_pae_pc' :
> +
> +            pc = gdb.parse_and_eval('$pc')
> +            pc_as_int = int(pc.cast(gdb.lookup_type('int')))
> +            # gdb.write("In unwinder: pc=%x\n" % pc_as_int)
> +
> +        elif TestUnwinder.test == 'check_undefined_symbol' :
> +
> +            try:
> +                val = gdb.parse_and_eval("undefined_symbol")
>  
> -        except Exception as arg:
> -            pass
> +            except Exception as arg:
> +                pass
>  
>          self.recurse_level -= 1

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH v2 1/5] Extend test gdb.python/py-recurse-unwind.exp

[Kevin Buettner]

On Wed, 16 Nov 2016 17:46:18 -0500
Sergio Durigan Junior <sergiodj@redhat.com> wrote:

> While checking some BuildBot logs, I noticed a few new FAILures
> introduced by this commit.  You can see them at:
> 
>   <https://sourceware.org/ml/gdb-testers/2016-q4/msg03062.html>
> 
> Cheers,
> 
> > commit 1a2f3d7ff1d79b1290704e48c71e905b987393a6
> > Author: Kevin Buettner <kevinb@redhat.com>
> > Date:   Mon Sep 26 15:00:37 2016 -0700

I wrote and committed the test (which introduces some new FAILures)
prior to committing the work which fixes those failures.  I didn't
think it would be a problem, since I pushed them all at the same
time.

However, I see now that I should have reordered these commits prior to
pushing them.  That's what I'll do in the future.

My apologies for introducing temporary regressions into the testing...

Kevin

Re: [PATCH v2 3/5] Distinguish sentinel frame from null frame

[Kevin Buettner]

This patch replaces the `current_frame' static global in frame.c with
`sentinel_frame'.  It also makes the sentinel frame id unique and
different from the null frame.

By itself, there is not much point to this patch, but it makes
the code cleaner for the VALUE_FRAME_ID changes in another patch.
Since we now allow "navigation" to the sentinel frame, it removes
the necessity of adding special cases to other parts of GDB.

Note that a new function, get_next_frame_sentinel_okay, is introduced
in this patch.  It will be used by the VALUE_FRAME_ID changes that
I've made.

Thanks to Pedro Alves for this suggestion.

gdb/ChangeLog:

	* frame.h (enum frame_id_stack_status): Add FID_STACK_SENTINEL.
	(struct frame_id): Increase number of bits required for storing
	stack status to 3 from 2.
	(sentinel_frame_id): New declaration.
	(get_next_frame_sentinel_okay): Declare.
	(frame_find_by_id_sentinel_okay): Declare.
	* frame.c (current_frame): Rename this static global to...
	(sentinel_frame): ...this static global, which has also been
	moved an earlier location in the file.
	(fprint_frame_id): Add case for sentinel frame id.
	(get_frame_id): Return early for sentinel frame.
	(sentinel_frame_id): Define.
	(frame_find_by_id): Add case for sentinel_frame_id.
	(create_sentinel_frame): Use sentinel_frame_id for this_id.value
	instead of null_frame_id.
	(get_current_frame): Add local declaration for `current_frame'.
	Remove local declaration for `sentinel_frame.'
	(get_next_frame_sentinel_okay): New function.
	(reinit_frame_cache): Use `sentinel_frame' in place of
	`current_frame'.
---
 gdb/frame.c | 94 +++++++++++++++++++++++++++++++++++++++----------------------
 gdb/frame.h | 12 +++++++-
 2 files changed, 71 insertions(+), 35 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index d3f3056..54dc833 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -43,6 +43,15 @@
 #include "hashtab.h"
 #include "valprint.h"
 
+/* The sentinel frame terminates the innermost end of the frame chain.
+   If unwound, it returns the information needed to construct an
+   innermost frame.
+
+   The current frame, which is the innermost frame, can be found at
+   sentinel_frame->prev.  */
+
+static struct frame_info *sentinel_frame;
+
 static struct frame_info *get_prev_frame_raw (struct frame_info *this_frame);
 static const char *frame_stop_reason_symbol_string (enum unwind_stop_reason reason);
 
@@ -65,7 +74,7 @@ enum cached_copy_status
 
 /* We keep a cache of stack frames, each of which is a "struct
    frame_info".  The innermost one gets allocated (in
-   wait_for_inferior) each time the inferior stops; current_frame
+   wait_for_inferior) each time the inferior stops; sentinel_frame
    points to it.  Additional frames get allocated (in get_prev_frame)
    as needed, and are chained through the next and prev fields.  Any
    time that the frame cache becomes invalid (most notably when we
@@ -323,6 +332,8 @@ fprint_frame_id (struct ui_file *file, struct frame_id id)
     fprintf_unfiltered (file, "!stack");
   else if (id.stack_status == FID_STACK_UNAVAILABLE)
     fprintf_unfiltered (file, "stack=<unavailable>");
+  else if (id.stack_status == FID_STACK_SENTINEL)
+    fprintf_unfiltered (file, "stack=<sentinel>");
   else
     fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
   fprintf_unfiltered (file, ",");
@@ -562,6 +573,7 @@ frame_unwind_caller_id (struct frame_info *next_frame)
 }
 
 const struct frame_id null_frame_id = { 0 }; /* All zeros.  */
+const struct frame_id sentinel_frame_id = { 0, 0, 0, FID_STACK_SENTINEL, 0, 1, 0 };
 const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_INVALID, 0, 1, 0 };
 
 struct frame_id
@@ -797,6 +809,10 @@ frame_find_by_id (struct frame_id id)
   if (!frame_id_p (id))
     return NULL;
 
+  /* Check for the sentinel frame.  */
+  if (frame_id_eq (id, sentinel_frame_id))
+    return sentinel_frame;
+
   /* Try using the frame stash first.  Finding it there removes the need
      to perform the search by looping over all frames, which can be very
      CPU-intensive if the number of frames is very high (the loop is O(n)
@@ -1474,10 +1490,9 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
   /* Link this frame back to itself.  The frame is self referential
      (the unwound PC is the same as the pc), so make it so.  */
   frame->next = frame;
-  /* Make the sentinel frame's ID valid, but invalid.  That way all
-     comparisons with it should fail.  */
+  /* The sentinel frame has a special ID.  */
   frame->this_id.p = 1;
-  frame->this_id.value = null_frame_id;
+  frame->this_id.value = sentinel_frame_id;
   if (frame_debug)
     {
       fprintf_unfiltered (gdb_stdlog, "{ create_sentinel_frame (...) -> ");
@@ -1487,10 +1502,6 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
   return frame;
 }
 
-/* Info about the innermost stack frame (contents of FP register).  */
-
-static struct frame_info *current_frame;
-
 /* Cache for frame addresses already read by gdb.  Valid only while
    inferior is stopped.  Control variables for the frame cache should
    be local to this module.  */
@@ -1511,6 +1522,8 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame
 struct frame_info *
 get_current_frame (void)
 {
+  struct frame_info *current_frame;
+
   /* First check, and report, the lack of registers.  Having GDB
      report "No stack!" or "No memory" when the target doesn't even
      have registers is very confusing.  Besides, "printcmd.exp"
@@ -1526,29 +1539,23 @@ get_current_frame (void)
   if (get_traceframe_number () < 0)
     validate_registers_access ();
 
-  if (current_frame == NULL)
-    {
-      int stashed;
-      struct frame_info *sentinel_frame =
-	create_sentinel_frame (current_program_space, get_current_regcache ());
-
-      /* Set the current frame before computing the frame id, to avoid
-	 recursion inside compute_frame_id, in case the frame's
-	 unwinder decides to do a symbol lookup (which depends on the
-	 selected frame's block).
-
-	 This call must always succeed.  In particular, nothing inside
-	 get_prev_frame_always_1 should try to unwind from the
-	 sentinel frame, because that could fail/throw, and we always
-	 want to leave with the current frame created and linked in --
-	 we should never end up with the sentinel frame as outermost
-	 frame.  */
-      current_frame = get_prev_frame_always_1 (sentinel_frame);
-      gdb_assert (current_frame != NULL);
-
-      /* No need to compute the frame id yet.  That'll be done lazily
-	 from inside get_frame_id instead.  */
-    }
+  if (sentinel_frame == NULL)
+    sentinel_frame =
+      create_sentinel_frame (current_program_space, get_current_regcache ());
+
+  /* Set the current frame before computing the frame id, to avoid
+     recursion inside compute_frame_id, in case the frame's
+     unwinder decides to do a symbol lookup (which depends on the
+     selected frame's block).
+
+     This call must always succeed.  In particular, nothing inside
+     get_prev_frame_always_1 should try to unwind from the
+     sentinel frame, because that could fail/throw, and we always
+     want to leave with the current frame created and linked in --
+     we should never end up with the sentinel frame as outermost
+     frame.  */
+  current_frame = get_prev_frame_always_1 (sentinel_frame);
+  gdb_assert (current_frame != NULL);
 
   return current_frame;
 }
@@ -1729,6 +1736,25 @@ get_next_frame (struct frame_info *this_frame)
     return NULL;
 }
 
+/* Return the frame that THIS_FRAME calls.  If THIS_FRAME is the
+   innermost (i.e. current) frame, return the sentinel frame.  Thus,
+   unlike get_next_frame(), NULL will never be returned.  */
+
+struct frame_info *
+get_next_frame_sentinel_okay (struct frame_info *this_frame)
+{
+  gdb_assert (this_frame != NULL);
+
+  /* Note that, due to the manner in which the sentinel frame is
+     constructed, this_frame->next still works even when this_frame
+     is the sentinel frame.  But we disallow it here anyway because
+     calling get_next_frame_sentinel_okay() on the sentinel frame
+     is likely a coding error.  */
+  gdb_assert (this_frame != sentinel_frame);
+
+  return this_frame->next;
+}
+
 /* Observer for the target_changed event.  */
 
 static void
@@ -1745,7 +1771,7 @@ reinit_frame_cache (void)
   struct frame_info *fi;
 
   /* Tear down all frame caches.  */
-  for (fi = current_frame; fi != NULL; fi = fi->prev)
+  for (fi = sentinel_frame; fi != NULL; fi = fi->prev)
     {
       if (fi->prologue_cache && fi->unwind->dealloc_cache)
 	fi->unwind->dealloc_cache (fi, fi->prologue_cache);
@@ -1757,10 +1783,10 @@ reinit_frame_cache (void)
   obstack_free (&frame_cache_obstack, 0);
   obstack_init (&frame_cache_obstack);
 
-  if (current_frame != NULL)
+  if (sentinel_frame != NULL)
     annotate_frames_invalid ();
 
-  current_frame = NULL;		/* Invalidate cache */
+  sentinel_frame = NULL;		/* Invalidate cache */
   select_frame (NULL);
   frame_stash_invalidate ();
   if (frame_debug)
diff --git a/gdb/frame.h b/gdb/frame.h
index a05ac82..1bb3565 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -90,6 +90,9 @@ enum frame_id_stack_status
   /* Stack address is valid, and is found in the stack_addr field.  */
   FID_STACK_VALID = 1,
 
+  /* Sentinel frame.  Stack may or may not be valid.  */
+  FID_STACK_SENTINEL = 2,
+
   /* Stack address is unavailable.  I.e., there's a valid stack, but
      we don't know where it is (because memory or registers we'd
      compute it from were not collected).  */
@@ -150,7 +153,7 @@ struct frame_id
   CORE_ADDR special_addr;
 
   /* Flags to indicate the above fields have valid contents.  */
-  ENUM_BITFIELD(frame_id_stack_status) stack_status : 2;
+  ENUM_BITFIELD(frame_id_stack_status) stack_status : 3;
   unsigned int code_addr_p : 1;
   unsigned int special_addr_p : 1;
 
@@ -166,6 +169,9 @@ struct frame_id
 /* For convenience.  All fields are zero.  This means "there is no frame".  */
 extern const struct frame_id null_frame_id;
 
+/* Sentinel frame.  */
+extern const struct frame_id sentinel_frame_id;
+
 /* This means "there is no frame ID, but there is a frame".  It should be
    replaced by best-effort frame IDs for the outermost frame, somehow.
    The implementation is only special_addr_p set.  */
@@ -310,6 +316,10 @@ extern void select_frame (struct frame_info *);
 extern struct frame_info *get_prev_frame (struct frame_info *);
 extern struct frame_info *get_next_frame (struct frame_info *);
 
+/* Like get_next_frame(), but allows return of the sentinel frame.  NULL
+   is never returned.  */
+extern struct frame_info *get_next_frame_sentinel_okay (struct frame_info *);
+
 /* Return a "struct frame_info" corresponding to the frame that called
    THIS_FRAME.  Returns NULL if there is no such frame.
 

Re: [PATCH v2 3/5] Distinguish sentinel frame from null frame

[Kevin Buettner]

Ugh.  I messed up the numbering.  It should have been 2/5 instead of 3/5.

Sorry for the confusion.

Kevin

Re: [PATCH v2 3/5] Distinguish sentinel frame from null frame

[Pedro Alves]

On 11/02/2016 10:16 PM, Kevin Buettner wrote:

> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -90,6 +90,9 @@ enum frame_id_stack_status
>    /* Stack address is valid, and is found in the stack_addr field.  */
>    FID_STACK_VALID = 1,
>  
> +  /* Sentinel frame.  Stack may or may not be valid.  */
> +  FID_STACK_SENTINEL = 2,

What does this "Stack may or may not be valid" comment mean?
"sentinel_frame_id" is a constant, how can the "stack_addr" field
vary?

Otherwise LGTM.

Thanks,
Pedro Alves

Re: [PATCH v2 3/5] Distinguish sentinel frame from null frame

[Kevin Buettner]

On Wed, 9 Nov 2016 14:48:09 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 11/02/2016 10:16 PM, Kevin Buettner wrote:
> 
> > --- a/gdb/frame.h
> > +++ b/gdb/frame.h
> > @@ -90,6 +90,9 @@ enum frame_id_stack_status
> >    /* Stack address is valid, and is found in the stack_addr field.  */
> >    FID_STACK_VALID = 1,
> >  
> > +  /* Sentinel frame.  Stack may or may not be valid.  */
> > +  FID_STACK_SENTINEL = 2,  
> 
> What does this "Stack may or may not be valid" comment mean?
> "sentinel_frame_id" is a constant, how can the "stack_addr" field
> vary?

I've adjusted the comment as follows:

+  /* Sentinel frame.  */
+  FID_STACK_SENTINEL = 2,

> Otherwise LGTM.

Pushed.

Thanks for the review.

Kevin

Re: [PATCH v2 3/5] Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID

[Kevin Buettner]

The VALUE_FRAME_ID macro provides access to a member in struct value
that's used to hold the frame id that's used when determining a
register's value or when assigning to a register.  The underlying
member has a long and obscure name.  I won't refer to it here, but
will simply refer to VALUE_FRAME_ID as if it's the struct value member
instead of being a convenient macro.

At the moment, without this patch in place, VALUE_FRAME_ID is set in
value_of_register_lazy() and several other locations to hold the frame
id of the frame passed to those functions.

VALUE_FRAME_ID is used in the lval_register case of
value_fetch_lazy().  To fetch the register's value, it calls
get_frame_register_value() which, in turn, calls
frame_unwind_register_value() with frame->next.

A python based unwinder may wish to determine the value of a register
or evaluate an expression containing a register.  When it does this,
value_fetch_lazy() will be called under some circumstances.  It will
attempt to determine the frame id associated with the frame passed to
it.  In so doing, it will end up back in the frame sniffer of the very
same python unwinder that's attempting to learn the value of a
register as part of the sniffing operation.  This recursion is not
desirable.

As noted above, when value_fetch_lazy() wants to fetch a register's
value, it does so (indirectly) by unwinding from frame->next.

With this in mind, a solution suggests itself:  Change VALUE_FRAME_ID
to hold the frame id associated with the next frame.  Then, when it
comes time to obtain the value associated with the register, we can
simply unwind from the frame corresponding to the frame id stored in
VALUE_FRAME_ID.  This neatly avoids the python unwinder recursion
problem by changing when the "next" operation occurs.  Instead of the
"next" operation occuring when the register value is fetched, it
occurs earlier on when assigning a frame id to VALUE_FRAME_ID.
(Thanks to Pedro for this suggestion.)

This patch implements this idea.

It builds on the patch "Distinguish sentinel frame from null frame".
Without that work in place, it's necessary to check for null_id at
several places and then obtain the sentinel frame.

It also renames most occurences of VALUE_FRAME_ID to
VALUE_NEXT_FRAME_ID to reflect the new meaning of this field.

There are several uses of VALUE_FRAME_ID which were not changed.  In
each case, the original meaning of VALUE_FRAME_ID is required to get
correct results.  In all but one of these uses, either
put_frame_register_bytes() or get_frame_register_bytes() is being
called with the frame value obtained from VALUE_FRAME_ID.  Both of
these functions perform some unwinding by performing a "->next"
operation on the frame passed to it.  If we were to use the new
VALUE_NEXT_FRAME_ID macro, this would effectively do two "->next"
operations, which is not what we want.

The VALUE_FRAME_ID macro has been redefined in terms of
VALUE_NEXT_FRAME_ID.  It simply fetches the previous frame's id,
providing this id as the value of the macro.
    
gdb/ChangeLog:
    
    	* value.h (VALUE_FRAME_ID): Rename to VALUE_NEXT_FRAME_ID. Update
    	comment.  Create new VALUE_FRAME_ID which is defined in terms of
    	VALUE_NEXT_FRAME_ID.
    	(deprecated_value_frame_id_hack): Rename to
    	deprecated_value_next_frame_id_hack.
    	* dwarf2loc.c, findvar.c, frame-unwind.c, sentinel-frame.c,
    	valarith.c, valops.c, value.c: Adjust nearly all occurences of
    	VALUE_FRAME_ID to VALUE_NEXT_FRAME_ID.  Add comments for those
    	which did not change.
    	* value.c (struct value): Rename frame_id field to next_frame_id.
    	Update comment.
    	(deprecated_value_frame_id_hack): Rename to
    	deprecated_value_next_frame_id_hack.
    	(value_fetch_lazy): Call frame_unwind_register_value()
	instead of get_frame_register_value().
    	* frame.c (get_prev_frame_id_by_id): New function.
    	* frame.h (get_prev_frame_id_by_id): Declare.
    	* dwarf2loc.c (dwarf2_evaluate_loc_desc_full): Make
    	VALUE_NEXT_FRAME_ID refer to the next frame.
	* findvar.c (value_of_register_lazy): Likewise.
	(default_value_from_register): Likewise.
	(value_from_register): Likewise.
	* frame_unwind.c (frame_unwind_got_optimized): Likewise.
	* sentinel-frame.c (sentinel_frame_prev_register): Likewise.
    	* value.h (VALUE_FRAME_ID): Update comment describing this macro.
---
 gdb/dwarf2loc.c      | 21 +++++++++++++++++----
 gdb/findvar.c        | 26 ++++++++++++++++++++------
 gdb/frame-unwind.c   |  3 ++-
 gdb/frame.c          | 16 ++++++++++++++++
 gdb/frame.h          |  4 ++++
 gdb/sentinel-frame.c |  2 +-
 gdb/valarith.c       |  2 +-
 gdb/valops.c         | 13 ++++++++++---
 gdb/value.c          | 32 +++++++++++++++++++++-----------
 gdb/value.h          | 16 ++++++++++++----
 10 files changed, 104 insertions(+), 31 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 6f25314..9bf80af 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1666,13 +1666,18 @@ read_pieced_value (struct value *v)
   gdb_byte *contents;
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (v);
-  struct frame_info *frame = frame_find_by_id (VALUE_FRAME_ID (v));
+  struct frame_info *frame;
   size_t type_len;
   size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (v)));
 
+  /* VALUE_FRAME_ID is used instead of VALUE_NEXT_FRAME_ID here
+     because FRAME is passed to get_frame_register_bytes(), which
+     does its own "->next" operation.  */
+  frame = frame_find_by_id (VALUE_FRAME_ID (v));
+
   if (value_type (v) != value_enclosing_type (v))
     internal_error (__FILE__, __LINE__,
 		    _("Should not be able to create a lazy value with "
@@ -1835,13 +1840,18 @@ write_pieced_value (struct value *to, struct value *from)
   const gdb_byte *contents;
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (to);
-  struct frame_info *frame = frame_find_by_id (VALUE_FRAME_ID (to));
+  struct frame_info *frame;
   size_t type_len;
   size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (to)));
 
+  /* VALUE_FRAME_ID is used instead of VALUE_NEXT_FRAME_ID here
+     because FRAME is passed to get_frame_register_bytes() and
+     put_frame_register_bytes(), both of which do their own "->next"
+     operations.  */
+  frame = frame_find_by_id (VALUE_FRAME_ID (to));
   if (frame == NULL)
     {
       mark_value_bytes_optimized_out (to, 0, TYPE_LENGTH (value_type (to)));
@@ -2295,7 +2305,10 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
   if (ctx.num_pieces > 0)
     {
       struct piece_closure *c;
-      struct frame_id frame_id = get_frame_id (frame);
+      struct frame_id frame_id
+        = ((frame == NULL) 
+	   ? null_frame_id
+	   : get_frame_id (get_next_frame_sentinel_okay (frame)));
       ULONGEST bit_size = 0;
       int i;
 
@@ -2310,7 +2323,7 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
 	 closure but before allocating the result.  */
       do_cleanups (value_chain);
       retval = allocate_computed_value (type, &pieced_value_funcs, c);
-      VALUE_FRAME_ID (retval) = frame_id;
+      VALUE_NEXT_FRAME_ID (retval) = frame_id;
       set_value_offset (retval, byte_offset);
     }
   else
diff --git a/gdb/findvar.c b/gdb/findvar.c
index 6e28a29..bab717a 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -283,17 +283,23 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct value *reg_val;
+  struct frame_info *next_frame;
 
   gdb_assert (regnum < (gdbarch_num_regs (gdbarch)
 			+ gdbarch_num_pseudo_regs (gdbarch)));
 
-  /* We should have a valid (i.e. non-sentinel) frame.  */
-  gdb_assert (frame_id_p (get_frame_id (frame)));
+  gdb_assert (frame != NULL);
+
+  next_frame = get_next_frame_sentinel_okay (frame);
+
+  /* We should have a valid next frame.  */
+  gdb_assert (frame_id_p (get_frame_id (next_frame)));
 
   reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
   VALUE_LVAL (reg_val) = lval_register;
   VALUE_REGNUM (reg_val) = regnum;
-  VALUE_FRAME_ID (reg_val) = get_frame_id (frame);
+  VALUE_NEXT_FRAME_ID (reg_val) = get_frame_id (next_frame);
+
   return reg_val;
 }
 
@@ -815,9 +821,17 @@ default_value_from_register (struct gdbarch *gdbarch, struct type *type,
 {
   int len = TYPE_LENGTH (type);
   struct value *value = allocate_value (type);
+  struct frame_info *frame;
 
   VALUE_LVAL (value) = lval_register;
-  VALUE_FRAME_ID (value) = frame_id;
+  frame = frame_find_by_id (frame_id);
+
+  if (frame == NULL)
+    frame_id = null_frame_id;
+  else
+    frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
+
+  VALUE_NEXT_FRAME_ID (value) = frame_id;
   VALUE_REGNUM (value) = regnum;
 
   /* Any structure stored in more than one register will always be
@@ -902,7 +916,7 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
          including the location.  */
       v = allocate_value (type);
       VALUE_LVAL (v) = lval_register;
-      VALUE_FRAME_ID (v) = get_frame_id (frame);
+      VALUE_NEXT_FRAME_ID (v) = get_frame_id (get_next_frame_sentinel_okay (frame));
       VALUE_REGNUM (v) = regnum;
       ok = gdbarch_register_to_value (gdbarch, frame, regnum, type1,
 				      value_contents_raw (v), &optim,
@@ -950,7 +964,7 @@ address_from_register (int regnum, struct frame_info *frame)
      where the ID of FRAME is not yet known.  Calling value_from_register
      would therefore abort in get_frame_id.  However, since we only need
      a temporary value that is never used as lvalue, we actually do not
-     really need to set its VALUE_FRAME_ID.  Therefore, we re-implement
+     really need to set its VALUE_NEXT_FRAME_ID.  Therefore, we re-implement
      the core of value_from_register, but use the null_frame_id.  */
 
   /* Some targets require a special conversion routine even for plain
diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 187e6c2..974a236 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -210,7 +210,8 @@ frame_unwind_got_optimized (struct frame_info *frame, int regnum)
   mark_value_bytes_optimized_out (val, 0, TYPE_LENGTH (type));
   VALUE_LVAL (val) = lval_register;
   VALUE_REGNUM (val) = regnum;
-  VALUE_FRAME_ID (val) = get_frame_id (frame);
+  VALUE_NEXT_FRAME_ID (val)
+    = get_frame_id (get_next_frame_sentinel_okay (frame));
   return val;
 }
 
diff --git a/gdb/frame.c b/gdb/frame.c
index 54dc833..02635e6 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2305,6 +2305,22 @@ get_prev_frame (struct frame_info *this_frame)
   return get_prev_frame_always (this_frame);
 }
 
+struct frame_id
+get_prev_frame_id_by_id (struct frame_id id)
+{
+  struct frame_id prev_id;
+  struct frame_info *frame;
+  
+  frame = frame_find_by_id (id);
+
+  if (frame != NULL)
+    prev_id = get_frame_id (get_prev_frame (frame));
+  else
+    prev_id = null_frame_id;
+
+  return prev_id;
+}
+
 CORE_ADDR
 get_frame_pc (struct frame_info *frame)
 {
diff --git a/gdb/frame.h b/gdb/frame.h
index 1bb3565..a128a86 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -331,6 +331,10 @@ extern struct frame_info *get_prev_frame_always (struct frame_info *);
    is not found.  */
 extern struct frame_info *frame_find_by_id (struct frame_id id);
 
+/* Given a frame's ID, find the previous frame's ID.  Returns null_frame_id
+   if the frame is not found.  */
+extern struct frame_id get_prev_frame_id_by_id (struct frame_id id);
+
 /* Base attributes of a frame: */
 
 /* The frame's `resume' address.  Where the program will resume in
diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c
index 6cd1bc3..eb827eb 100644
--- a/gdb/sentinel-frame.c
+++ b/gdb/sentinel-frame.c
@@ -51,7 +51,7 @@ sentinel_frame_prev_register (struct frame_info *this_frame,
   struct value *value;
 
   value = regcache_cooked_read_value (cache->regcache, regnum);
-  VALUE_FRAME_ID (value) = get_frame_id (this_frame);
+  VALUE_NEXT_FRAME_ID (value) = sentinel_frame_id;
 
   return value;
 }
diff --git a/gdb/valarith.c b/gdb/valarith.c
index de6fcfd..2c56110 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -227,7 +227,7 @@ value_subscripted_rvalue (struct value *array, LONGEST index, int lowerbound)
 
   set_value_component_location (v, array);
   VALUE_REGNUM (v) = VALUE_REGNUM (array);
-  VALUE_FRAME_ID (v) = VALUE_FRAME_ID (array);
+  VALUE_NEXT_FRAME_ID (v) = VALUE_NEXT_FRAME_ID (array);
   set_value_offset (v, value_offset (array) + elt_offs);
   return v;
 }
diff --git a/gdb/valops.c b/gdb/valops.c
index 40392e8..f96016f 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1112,8 +1112,15 @@ value_assign (struct value *toval, struct value *fromval)
 	struct gdbarch *gdbarch;
 	int value_reg;
 
-	/* Figure out which frame this is in currently.  */
+	/* Figure out which frame this is in currently.
+	
+	   We use VALUE_FRAME_ID for obtaining the value's frame id instead of
+	   VALUE_NEXT_FRAME_ID due to requiring a frame which may be passed  to
+	   put_frame_register_bytes() below.  That function will (eventually)
+	   perform the any necessary unwind operation by first obtaining the next
+	   frame.  */
 	frame = frame_find_by_id (VALUE_FRAME_ID (toval));
+
 	value_reg = VALUE_REGNUM (toval);
 
 	if (!frame)
@@ -1333,7 +1340,7 @@ address_of_variable (struct symbol *var, const struct block *b)
 	struct frame_info *frame;
 	const char *regname;
 
-	frame = frame_find_by_id (VALUE_FRAME_ID (val));
+	frame = frame_find_by_id (VALUE_NEXT_FRAME_ID (val));
 	gdb_assert (frame);
 
 	regname = gdbarch_register_name (get_frame_arch (frame),
@@ -3820,7 +3827,7 @@ value_slice (struct value *array, int lowbound, int length)
       }
 
     set_value_component_location (slice, array);
-    VALUE_FRAME_ID (slice) = VALUE_FRAME_ID (array);
+    VALUE_NEXT_FRAME_ID (slice) = VALUE_NEXT_FRAME_ID (array);
     set_value_offset (slice, value_offset (array) + offset);
   }
 
diff --git a/gdb/value.c b/gdb/value.c
index b825aec..56012bc 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -262,9 +262,11 @@ struct value
      bitfields.  */
   struct value *parent;
 
-  /* Frame register value is relative to.  This will be described in
-     the lval enum above as "lval_register".  */
-  struct frame_id frame_id;
+  /* Frame ID of "next" frame to which a register value is relative.  A
+     register value is indicated when the lval enum (above) is set to
+     lval_register.  So, if the register value is found relative to frame F,
+     then the frame id of F->next will be stored in next_frame_id.  */
+  struct frame_id next_frame_id;
 
   /* Type of the value.  */
   struct type *type;
@@ -943,7 +945,7 @@ allocate_value_lazy (struct type *type)
   val->enclosing_type = type;
   VALUE_LVAL (val) = not_lval;
   val->location.address = 0;
-  VALUE_FRAME_ID (val) = null_frame_id;
+  VALUE_NEXT_FRAME_ID (val) = null_frame_id;
   val->offset = 0;
   val->bitpos = 0;
   val->bitsize = 0;
@@ -1582,9 +1584,9 @@ deprecated_value_internalvar_hack (struct value *value)
 }
 
 struct frame_id *
-deprecated_value_frame_id_hack (struct value *value)
+deprecated_value_next_frame_id_hack (struct value *value)
 {
-  return &value->frame_id;
+  return &value->next_frame_id;
 }
 
 short *
@@ -1786,7 +1788,7 @@ value_copy (struct value *arg)
   val->offset = arg->offset;
   val->bitpos = arg->bitpos;
   val->bitsize = arg->bitsize;
-  VALUE_FRAME_ID (val) = VALUE_FRAME_ID (arg);
+  VALUE_NEXT_FRAME_ID (val) = VALUE_NEXT_FRAME_ID (arg);
   VALUE_REGNUM (val) = VALUE_REGNUM (arg);
   val->lazy = arg->lazy;
   val->embedded_offset = value_embedded_offset (arg);
@@ -3232,7 +3234,7 @@ value_primitive_field (struct value *arg1, LONGEST offset,
     }
   set_value_component_location (v, arg1);
   VALUE_REGNUM (v) = VALUE_REGNUM (arg1);
-  VALUE_FRAME_ID (v) = VALUE_FRAME_ID (arg1);
+  VALUE_NEXT_FRAME_ID (v) = VALUE_NEXT_FRAME_ID (arg1);
   return v;
 }
 
@@ -3989,7 +3991,7 @@ value_fetch_lazy (struct value *val)
 
       while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
 	{
-	  struct frame_id frame_id = VALUE_FRAME_ID (new_val);
+	  struct frame_id frame_id = VALUE_NEXT_FRAME_ID (new_val);
 
 	  frame = frame_find_by_id (frame_id);
 	  regnum = VALUE_REGNUM (new_val);
@@ -4004,7 +4006,13 @@ value_fetch_lazy (struct value *val)
 	  gdb_assert (!gdbarch_convert_register_p (get_frame_arch (frame),
 						   regnum, type));
 
-	  new_val = get_frame_register_value (frame, regnum);
+	  /* FRAME was obtained, above, via VALUE_NEXT_FRAME_ID. 
+	     Since a "->next" operation was performed when setting
+	     this field, we do not need to perform a "next" operation
+	     again when unwinding the register.  That's why
+	     frame_unwind_register_value() is called here instead of
+	     get_frame_register_value().  */
+	  new_val = frame_unwind_register_value (frame, regnum);
 
 	  /* If we get another lazy lval_register value, it means the
 	     register is found by reading it from the next frame.
@@ -4018,7 +4026,7 @@ value_fetch_lazy (struct value *val)
 	     in this situation.  */
 	  if (VALUE_LVAL (new_val) == lval_register
 	      && value_lazy (new_val)
-	      && frame_id_eq (VALUE_FRAME_ID (new_val), frame_id))
+	      && frame_id_eq (VALUE_NEXT_FRAME_ID (new_val), frame_id))
 	    internal_error (__FILE__, __LINE__,
 			    _("infinite loop while fetching a register"));
 	}
@@ -4038,6 +4046,8 @@ value_fetch_lazy (struct value *val)
       if (frame_debug)
 	{
 	  struct gdbarch *gdbarch;
+	  /* VALUE_FRAME_ID is used here, instead of VALUE_NEXT_FRAME_ID,
+	     so that the frame level will be shown correctly.  */
 	  frame = frame_find_by_id (VALUE_FRAME_ID (val));
 	  regnum = VALUE_REGNUM (val);
 	  gdbarch = get_frame_arch (frame);
diff --git a/gdb/value.h b/gdb/value.h
index 3299e87..e01ad89 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -434,10 +434,18 @@ extern void set_value_address (struct value *, CORE_ADDR);
 extern struct internalvar **deprecated_value_internalvar_hack (struct value *);
 #define VALUE_INTERNALVAR(val) (*deprecated_value_internalvar_hack (val))
 
-/* Frame register value is relative to.  This will be described in the
-   lval enum above as "lval_register".  */
-extern struct frame_id *deprecated_value_frame_id_hack (struct value *);
-#define VALUE_FRAME_ID(val) (*deprecated_value_frame_id_hack (val))
+/* Frame ID of "next" frame to which a register value is relative.  A
+   register value is indicated by VALUE_LVAL being set to lval_register.
+   So, if the register value is found relative to frame F, then the
+   frame id of F->next will be stored in VALUE_NEXT_FRAME_ID.  */
+extern struct frame_id *deprecated_value_next_frame_id_hack (struct value *);
+#define VALUE_NEXT_FRAME_ID(val) (*deprecated_value_next_frame_id_hack (val))
+
+/* Frame ID of frame to which a register value is relative.  This is
+   similar to VALUE_NEXT_FRAME_ID, above, but may not be assigned to. 
+   Note that VALUE_FRAME_ID effectively undoes the "next" operation
+   that was performed during the assignment to VALUE_NEXT_FRAME_ID.  */
+#define VALUE_FRAME_ID(val) (get_prev_frame_id_by_id (VALUE_NEXT_FRAME_ID (val)))
 
 /* Register number if the value is from a register.  */
 extern short *deprecated_value_regnum_hack (struct value *);

Re: [PATCH v2 3/5] Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID

[Pedro Alves]

On 11/02/2016 10:19 PM, Kevin Buettner wrote:

> @@ -2295,7 +2305,10 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
>    if (ctx.num_pieces > 0)
>      {
>        struct piece_closure *c;
> -      struct frame_id frame_id = get_frame_id (frame);
> +      struct frame_id frame_id
> +        = ((frame == NULL) 

Redundant parens.

> +	   ? null_frame_id
> +	   : get_frame_id (get_next_frame_sentinel_okay (frame)));
>        ULONGEST bit_size = 0;
>        int i;
>  


> diff --git a/gdb/valops.c b/gdb/valops.c
> index 40392e8..f96016f 100644
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -1112,8 +1112,15 @@ value_assign (struct value *toval, struct value *fromval)
>  	struct gdbarch *gdbarch;
>  	int value_reg;
>  
> -	/* Figure out which frame this is in currently.  */
> +	/* Figure out which frame this is in currently.
> +	
> +	   We use VALUE_FRAME_ID for obtaining the value's frame id instead of
> +	   VALUE_NEXT_FRAME_ID due to requiring a frame which may be passed  to

Spurious space in "passed  to".

> +	   put_frame_register_bytes() below.  That function will (eventually)
> +	   perform the any necessary unwind operation by first obtaining the next
> +	   frame.  */

"the any necessary" looks like a typo?

>  	frame = frame_find_by_id (VALUE_FRAME_ID (toval));
> +
>  	value_reg = VALUE_REGNUM (toval);
>  
>  	if (!frame)



> @@ -3989,7 +3991,7 @@ value_fetch_lazy (struct value *val)
>  
>        while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
>  	{
> -	  struct frame_id frame_id = VALUE_FRAME_ID (new_val);
> +	  struct frame_id frame_id = VALUE_NEXT_FRAME_ID (new_val);
>  
>  	  frame = frame_find_by_id (frame_id);
>  	  regnum = VALUE_REGNUM (new_val);
> @@ -4004,7 +4006,13 @@ value_fetch_lazy (struct value *val)
>  	  gdb_assert (!gdbarch_convert_register_p (get_frame_arch (frame),
>  						   regnum, type));
>  
> -	  new_val = get_frame_register_value (frame, regnum);
> +	  /* FRAME was obtained, above, via VALUE_NEXT_FRAME_ID. 
> +	     Since a "->next" operation was performed when setting
> +	     this field, we do not need to perform a "next" operation
> +	     again when unwinding the register.  That's why
> +	     frame_unwind_register_value() is called here instead of
> +	     get_frame_register_value().  */
> +	  new_val = frame_unwind_register_value (frame, regnum);

The comment just below needs updating: it's still phrased in terms
of get_frame_register_value.  Also, I suspect that renaming the "frame" and
"frame_id" locals to "next_frame" and "next_frame_id" would allow simplifying
the new comment.

>  
>  	  /* If we get another lazy lval_register value, it means the
>  	     register is found by reading it from the next frame.
> @@ -4018,7 +4026,7 @@ value_fetch_lazy (struct value *val)
>  	     in this situation.  */
>  	  if (VALUE_LVAL (new_val) == lval_register
>  	      && value_lazy (new_val)
> -	      && frame_id_eq (VALUE_FRAME_ID (new_val), frame_id))
> +	      && frame_id_eq (VALUE_NEXT_FRAME_ID (new_val), frame_id))
>  	    internal_error (__FILE__, __LINE__,
>  			    _("infinite loop while fetching a register"));
>  	}


Otherwise LGTM.

Thanks,
Pedro Alves

Re: [PATCH v2 3/5] Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID

[Kevin Buettner]

On Wed, 9 Nov 2016 14:48:35 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 11/02/2016 10:19 PM, Kevin Buettner wrote:
> 
> > @@ -2295,7 +2305,10 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
> >    if (ctx.num_pieces > 0)
> >      {
> >        struct piece_closure *c;
> > -      struct frame_id frame_id = get_frame_id (frame);
> > +      struct frame_id frame_id
> > +        = ((frame == NULL)   
> 
> Redundant parens.

Fixed.

> > +	   We use VALUE_FRAME_ID for obtaining the value's frame id instead of
> > +	   VALUE_NEXT_FRAME_ID due to requiring a frame which may be passed  to  
> 
> Spurious space in "passed  to".

Fixed.

> > +	   put_frame_register_bytes() below.  That function will (eventually)
> > +	   perform the any necessary unwind operation by first obtaining the next
> > +	   frame.  */  
> 
> "the any necessary" looks like a typo?

Agreed.  I removed "any " from the comment.

> The comment just below needs updating: it's still phrased in terms
> of get_frame_register_value.  Also, I suspect that renaming the "frame" and
> "frame_id" locals to "next_frame" and "next_frame_id" would allow simplifying
> the new comment.
> 
> >  
> >  	  /* If we get another lazy lval_register value, it means the
> >  	     register is found by reading it from the next frame.

I made the next_frame and next_frame_id changes that you suggested.
Here's the change for the comment:

 	  /* If we get another lazy lval_register value, it means the
-	     register is found by reading it from the next frame.
-	     get_frame_register_value should never return a value with
-	     the frame id pointing to FRAME.  If it does, it means we
+	     register is found by reading it from NEXT_FRAME's next frame.
+	     frame_unwind_register_value should never return a value with
+	     the frame id pointing to NEXT_FRAME.  If it does, it means we
 	     either have two consecutive frames with the same frame id
 	     in the frame chain, or some code is trying to unwind
 	     behind get_prev_frame's back (e.g., a frame unwind

> Otherwise LGTM.

Thanks again for the review.

Pushed.

Kevin

Re: [PATCH v2 4/5] Make gdb.PendingFrame.read_register handle "user" registers

[Kevin Buettner]

[ FYI, Pedro has already OK'd this one.  It's unchanged from the version
  in the earlier series, but is included here for completeness. ]

The C function, pending_framepy_read_register(), which implements
the python interface gdb.PendingFrame.read_register does not handle
the so called "user" registers like "pc".  An assertion error is
triggered due to the user registers having numbers larger than or
equal to gdbarch_num_regs(gdbarch).

With the VALUE_FRAME_ID tweak in place, the call to
get_frame_register_value() can simply be replaced by a call to
value_of_register(), which handles both real registers as well as the
user registers.

gdb/ChangeLog:
    
    	* python/py-unwind.c (pending_framepy_read_register): Use
    	value_of_register() instead of get_frame_register_value().
---
 gdb/python/py-unwind.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index cc685ae..6740034 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -412,7 +412,12 @@ pending_framepy_read_register (PyObject *self, PyObject *args)
 
   TRY
     {
-      val = get_frame_register_value (pending_frame->frame_info, regnum);
+      /* Fetch the value associated with a register, whether it's
+	 a real register or a so called "user" register, like "pc",
+	 which maps to a real register.  In the past,
+	 get_frame_register_value() was used here, which did not
+	 handle the user register case.  */
+      val = value_of_register (regnum, pending_frame->frame_info);
       if (val == NULL)
         PyErr_Format (PyExc_ValueError,
                       "Cannot read register %d from frame.",

Re: [PATCH v2 4/5] Make gdb.PendingFrame.read_register handle "user" registers

[Kevin Buettner]

On Wed, 2 Nov 2016 15:23:32 -0700
Kevin Buettner <kevin@buettner.to> wrote:

> [ FYI, Pedro has already OK'd this one.  It's unchanged from the version
>   in the earlier series, but is included here for completeness. ]
> 
> The C function, pending_framepy_read_register(), which implements
> the python interface gdb.PendingFrame.read_register does not handle
> the so called "user" registers like "pc".  An assertion error is
> triggered due to the user registers having numbers larger than or
> equal to gdbarch_num_regs(gdbarch).
> 
> With the VALUE_FRAME_ID tweak in place, the call to
> get_frame_register_value() can simply be replaced by a call to
> value_of_register(), which handles both real registers as well as the
> user registers.
> 
> gdb/ChangeLog:
>     
>     	* python/py-unwind.c (pending_framepy_read_register): Use
>     	value_of_register() instead of get_frame_register_value().

I've pushed this change.

Kevin

Re: [PATCH v2 5/5] Stash frame id of current frame before stashing frame id for previous frame

[Kevin Buettner]

This patch ensures that the frame id for the current frame is stashed
before that of the previous frame (to the current frame).

First, it should be noted that the frame id for the current frame is
not stashed by get_current_frame().  The current frame's frame id is
lazily computed and stashed via calls to get_frame_id().  However,
it's possible for get_prev_frame() to be called without first stashing
the current frame.

The frame stash is used not only to speed up frame lookups, but
also to detect cycles.  When attempting to compute the frame id
for a "previous" frame (in get_prev_frame_if_no_cycle), a cycle
is detected if the computed frame id is already in the stash.

If it should happen that a previous frame id is stashed which should
represent a cycle for the current frame, then an assertion failure
will trigger should get_frame_id() be later called to determine
the frame id for the current frame.

As of late 2016, with the "Tweak meaning of VALUE_FRAME_ID" patch in
place, this actually occurs when running the
gdb.dwarf2/dw2-dup-frame.exp test.  While attempting to generate a
backtrace, the python frame filter code is invoked, leading to
frame_info_to_frame_object() (in python/py-frame.c) being called.
That function will potentially call get_prev_frame() before
get_frame_id() is called.  The call to get_prev_frame() can eventually
end up in get_prev_frame_if_no_cycle() which, in turn, calls
compute_frame_id(), after which the frame id is stashed for the
previous frame.

If the frame id for the current frame is stashed, the cycle detection
code (which relies on the frame stash) in get_prev_frame_if_no_cycle()
will be triggered for a cycle starting with the current frame.  If the
current frame's id is not stashed, the cycle detecting code can't
operate as designed.  Instead, when get_frame_id() is called on the
current frame at some later point, the current frame's id will found
to be already in the stash, triggering an assertion failure.

Below is an in depth examination of the failure which lead to this change.
I've shortened pathnames for brevity and readability.

Here's the portion of the log file showing the failure/internal error:

(gdb) break stop_frame
Breakpoint 1 at 0x40059a: file dw2-dup-frame.c, line 22.
(gdb) run
Starting program: testsuite/outputs/gdb.dwarf2/dw2-dup-frame/dw2-dup-frame

Breakpoint 1, stop_frame () at dw2-dup-frame.c:22
22	}
(gdb) bt
gdb/frame.c:544: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)
FAIL: gdb.dwarf2/dw2-dup-frame.exp: backtrace from stop_frame (GDB internal error)

Here's a partial backtrace from the internal error, showing the frames
which I think are relevant, plus several extra to provide context:

    file=0x932b98 "gdb/frame.c", line=544,
    fmt=0x932b20 "%s: Assertion `%s' failed.")
    at gdb/common/errors.c:54
    at gdb/frame.c:544
    at gdb/python/py-frame.c:390
    frame_low=0, frame_high=-1)
    at gdb/python/py-framefilter.c:1453
    extlang=0x8857e0 <extension_language_python>, frame=0xe5a760, flags=7,
    args_type=CLI_SCALAR_VALUES, out=0xf6def0, frame_low=0, frame_high=-1)
    at gdb/python/py-framefilter.c:1548
    flags=7, args_type=CLI_SCALAR_VALUES, out=0xf6def0, frame_low=0,
    frame_high=-1)
    at gdb/extension.c:572
    no_filters=0, from_tty=1)
    at gdb/stack.c:1834

Examination of the code in frame_info_to_frame_object(), which is in
python/py-frame.c, is key to understanding this problem:

      if (get_prev_frame (frame) == NULL
      && get_frame_unwind_stop_reason (frame) != UNWIND_NO_REASON
      && get_next_frame (frame) != NULL)
    {
      frame_obj->frame_id = get_frame_id (get_next_frame (frame));
      frame_obj->frame_id_is_next = 1;
    }
      else
    {
      frame_obj->frame_id = get_frame_id (frame);
      frame_obj->frame_id_is_next = 0;
    }

I will first note that the frame id for frame has not been computed yet.  (This
was verified by placing a breakpoint on compute_frame_id().)

The call to get_prev_frame() causes the the frame id to (eventually) be
computed for the previous frame.  Here's a backtrace showing how we
get there:

    at gdb/frame.c:496
    at gdb/frame.c:1871
    at gdb/frame.c:2045
    at gdb/frame.c:2061
    at gdb/frame.c:2303
    at gdb/python/py-frame.c:381

For this particular case, we end up in the else clause of the code above
which calls get_frame_id (frame).  It's at this point that the frame id
for frame is computed.  Again, here's a backtrace:

    at gdb/frame.c:496
    at gdb/frame.c:539
    at gdb/python/py-frame.c:390

The test in question, dw2-dup-frame.exp, deliberately creates a broken
(cyclic) stack.  So, in this instance, the frame id for the prev
`frame' will be the same as that for `frame'.  But that particular
frame id ended up in the stash during the previous frame operation.
When, just a few lines later, we compute the frame id for `frame', the
id in question is already in the stash, thus triggering the assertion
failure.

I considered two other solutions to solving this problem:

We could prevent get_prev_frame() from being called before
get_frame_id() in frame_info_to_frame_object().  (See above for the
snippet of code where this happens.) A call to get_frame_id (frame)
could be placed ahead of that code snippet above.  I have tested this
approach and, while it does work, I can't be certain that
get_prev_frame() isn't called ahead of stashing the current frame
somewhere else in GDB, but in a less obvious way.

Another approach is to stash the current frame's id by calling
get_frame_id() in get_current_frame().  This approach is conceptually
simpler, but when importing a python unwinder, has the unwelcome side
effect of causing the unwinder to be called during import.

A cleaner looking fix would be to place this code after code
corresponding to the "Don't compute the frame id of the current frame
yet..." comment in get_prev_frame_if_no_cycle().  Sadly, this does not
work though; by the time we get to this point, the frame state for the
prev frame has been modified just enough to cause an internal error to
occur when attempting to compute the (current) frame id for inline
frames.  (The unexpected failure count increases by roughly 130
failures.)  Therefore, I decided to place it as early as possible
in get_prev_frame().

gdb/ChangeLog:
    
    	* frame.c (get_prev_frame): Stash frame id for current frame
    	prior to computing frame id for previous frame.
---
 gdb/frame.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gdb/frame.c b/gdb/frame.c
index 02635e6..5414cb3 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2220,6 +2220,17 @@ get_prev_frame (struct frame_info *this_frame)
      something should be calling get_selected_frame() or
      get_current_frame().  */
   gdb_assert (this_frame != NULL);
+  
+  /* If this_frame is the current frame, then compute and stash
+     its frame id prior to fetching and computing the frame id of the
+     previous frame.  Otherwise, the cycle detection code in
+     get_prev_frame_if_no_cycle() will not work correctly.  When
+     get_frame_id() is called later on, an assertion error will
+     be triggered in the event of a cycle between the current
+     frame and its previous frame.  */
+  if (this_frame->level == 0)
+    get_frame_id (this_frame);
+
   frame_pc_p = get_frame_pc_if_available (this_frame, &frame_pc);
 
   /* tausq/2004-12-07: Dummy frames are skipped because it doesn't make much

Re: [PATCH v2 5/5] Stash frame id of current frame before stashing frame id for previous frame

[Pedro Alves]

On 11/02/2016 10:26 PM, Kevin Buettner wrote:

> I will first note that the frame id for frame has not been computed yet.  (This
> was verified by placing a breakpoint on compute_frame_id().)
> 
> The call to get_prev_frame() causes the the frame id to (eventually) be
> computed for the previous frame.  Here's a backtrace showing how we
> get there:
> 
>     at gdb/frame.c:496
>     at gdb/frame.c:1871
>     at gdb/frame.c:2045
>     at gdb/frame.c:2061
>     at gdb/frame.c:2303
>     at gdb/python/py-frame.c:381

Function names would make that backtrace soooo much easier to read.  :-)

> gdb/ChangeLog:
>     
>     	* frame.c (get_prev_frame): Stash frame id for current frame
>     	prior to computing frame id for previous frame.


I'm fine with this solution.  LGTM.

Thanks,
Pedro Alves

Re: [PATCH v2 5/5] Stash frame id of current frame before stashing frame id for previous frame

[Kevin Buettner]

On Wed, 9 Nov 2016 14:48:46 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 11/02/2016 10:26 PM, Kevin Buettner wrote:
> 
> > I will first note that the frame id for frame has not been computed yet.  (This
> > was verified by placing a breakpoint on compute_frame_id().)
> > 
> > The call to get_prev_frame() causes the the frame id to (eventually) be
> > computed for the previous frame.  Here's a backtrace showing how we
> > get there:
> > 
> >     at gdb/frame.c:496
> >     at gdb/frame.c:1871
> >     at gdb/frame.c:2045
> >     at gdb/frame.c:2061
> >     at gdb/frame.c:2303
> >     at gdb/python/py-frame.c:381  
> 
> Function names would make that backtrace soooo much easier to read.  :-)

Thanks for catching this.  It had me puzzled for a while.

Since some of the backtrace lines were long, I pasted the backtrace
into my commit message without any leading whitespace.  As a consequence,
the # for the backtrace ended up in column 0 and was interpreted by git
as a comment.

I added the backtraces back into the commit message with leading whitespace.

After reading the documentation, I see that "git commit --cleanup=verbatim"
could be used to prevent git from removing lines starting with #.  Though
that means that the committer needs to manually remove other comment lines
added by git.

> 
> > gdb/ChangeLog:
> >     
> >     	* frame.c (get_prev_frame): Stash frame id for current frame
> >     	prior to computing frame id for previous frame.  
> 
> 
> I'm fine with this solution.  LGTM.

Pushed.

Kevin