Broken Backtraces, and Part of a Solution

Broken Backtraces, and Part of a Solution

[Noah Lavine]

Hello all,

I recently realized that backtraces weren't working for me in the most
recent build of Guile master. Specifically, I could get to a debug
prompt fine, but when I tried to get a backtrace, it always came up
empty. The problem happens in this code in
module/system/repl/error-handling.scm:

               (let* ((tag (and (pair? (fluid-ref %stacks))
                                (cdar (fluid-ref %stacks))))
                      (stack (narrow-stack->vector
                              (make-stack #t)
                              ;; Cut three frames from the top of the stack:
                              ;; make-stack, this one, and the throw handler.
                              3
                              ;; Narrow the end of the stack to the most recent
                              ;; start-stack.
                              tag
                              ;; And one more frame, because
%start-stack invoking
                              ;; the start-stack thunk has its own frame too.
                              0 (and tag 1)))
                      (error-msg (error-string stack key args))
                      (debug (make-debug stack 0 error-msg #f)))

(note: there are two instances of almost exactly the same code. the
problem I see happens at the second instance, but the first would
probably be the same)

The problem is that narrow-stack->vector returns #(). It does this
because the stack is narrowed to nothing. The narrowing really happens
in the functions scm_make_stack and narrow_stack, in stacks.c.

The reason it narrows to nothing is the third argument to
narrow-stack->vector, tag. On my Guile build, tag evaluates to
'("start-stack"). The code is trying to use the tag to trim extra
frames off of the stack, but we can only trim with procedures,
symbols, and integers. The fallback behavior is to eliminate the
entire stack, which is what we see here.

It's possible to solve this problem by using %start-stack instead of
'("start-stack"), but that doesn't seem to be as general as the
solution in this function. Instead, here's a question - why are we
using (cdar (fluid-ref %stacks)) to get the stack tag, and what was
someone expecting that to return when they wrote it?

Thanks,
Noah

Re: Broken Backtraces, and Part of a Solution

[Noah Lavine]

Sorry for the quick update, but this seems to be a result of commit
283ab48d3f20a5c5281cafc29f0c30c8d8ace9ee, on March 7th.

The fluid %stacks is set in %start-stack, in boot-9.scm. %start-stack
calls make-prompt-tag, also in boot-9.scm. The commit above switched
prompt tags from using gensyms to using lists. This is much faster,
but it also made backtraces break, because narrow_stack has the
ability to search for symbols but not for lists.

The fix I'd like to implement is to change the make-stack interface.
It trims stacks according to its arguments, but it tries to be too
clever - if it's passed an integer, it trims that many frames. If it's
passed a procedure, it trims that many procedures. If it's passed a
symbol, it trims until it sees the corresponding prompt. That worked
fine as long as prompt tags are always symbols, but breaks when they
are not.

We could fix it quickly by letting make-stack trim on pairs the same
as symbols, but that seems likely to break again. Instead, what if the
user could specify the sort of trimming they wanted? Something like
this:

  (make-stack #t '(inner prompt-tag ("start-stack"))) would trim the
stack from the innermost frame looking for prompt tag
'("start-stack"). (make-stack #t (outer frames 3)) would trim the
outermost 3 frames from the stack.

The make-stack interface isn't used very much, according to grep, so
it wouldn't be hard to change all of its uses over to the new one.

Noah

On Wed, Apr 18, 2012 at 8:02 PM, Noah Lavine <noah.b.lavine@gmail.com> wrote:
> Hello all,
>
> I recently realized that backtraces weren't working for me in the most
> recent build of Guile master. Specifically, I could get to a debug
> prompt fine, but when I tried to get a backtrace, it always came up
> empty. The problem happens in this code in
> module/system/repl/error-handling.scm:
>
>               (let* ((tag (and (pair? (fluid-ref %stacks))
>                                (cdar (fluid-ref %stacks))))
>                      (stack (narrow-stack->vector
>                              (make-stack #t)
>                              ;; Cut three frames from the top of the stack:
>                              ;; make-stack, this one, and the throw handler.
>                              3
>                              ;; Narrow the end of the stack to the most recent
>                              ;; start-stack.
>                              tag
>                              ;; And one more frame, because
> %start-stack invoking
>                              ;; the start-stack thunk has its own frame too.
>                              0 (and tag 1)))
>                      (error-msg (error-string stack key args))
>                      (debug (make-debug stack 0 error-msg #f)))
>
> (note: there are two instances of almost exactly the same code. the
> problem I see happens at the second instance, but the first would
> probably be the same)
>
> The problem is that narrow-stack->vector returns #(). It does this
> because the stack is narrowed to nothing. The narrowing really happens
> in the functions scm_make_stack and narrow_stack, in stacks.c.
>
> The reason it narrows to nothing is the third argument to
> narrow-stack->vector, tag. On my Guile build, tag evaluates to
> '("start-stack"). The code is trying to use the tag to trim extra
> frames off of the stack, but we can only trim with procedures,
> symbols, and integers. The fallback behavior is to eliminate the
> entire stack, which is what we see here.
>
> It's possible to solve this problem by using %start-stack instead of
> '("start-stack"), but that doesn't seem to be as general as the
> solution in this function. Instead, here's a question - why are we
> using (cdar (fluid-ref %stacks)) to get the stack tag, and what was
> someone expecting that to return when they wrote it?
>
> Thanks,
> Noah

Re: Broken Backtraces, and Part of a Solution

[Andy Wingo]

On Wed 18 Apr 2012 17:02, Noah Lavine <noah.b.lavine@gmail.com> writes:

> The problem is that narrow-stack->vector returns #(). It does this
> because the stack is narrowed to nothing. The narrowing really happens
> in the functions scm_make_stack and narrow_stack, in stacks.c.
>
> The reason it narrows to nothing is the third argument to
> narrow-stack->vector, tag. On my Guile build, tag evaluates to
> '("start-stack").

Aaaaah.  I was seeing something like this as well but I didn't figure
out why.  Thanks for tracking this down!  The reason is that the type of
make-prompt-tag changed, and the stack narrowing code didn't adapt
accordingly.  We need to change to default to consider generic objects
as eq?-compared prompt tags.

Andy
-- 
http://wingolog.org/

Re: Broken Backtraces, and Part of a Solution

[Noah Lavine]

On Wed, Apr 18, 2012 at 8:56 PM, Andy Wingo <wingo@pobox.com> wrote:
> On Wed 18 Apr 2012 17:02, Noah Lavine <noah.b.lavine@gmail.com> writes:
>
>> The problem is that narrow-stack->vector returns #(). It does this
>> because the stack is narrowed to nothing. The narrowing really happens
>> in the functions scm_make_stack and narrow_stack, in stacks.c.
>>
>> The reason it narrows to nothing is the third argument to
>> narrow-stack->vector, tag. On my Guile build, tag evaluates to
>> '("start-stack").
>
> Aaaaah.  I was seeing something like this as well but I didn't figure
> out why.  Thanks for tracking this down!  The reason is that the type of
> make-prompt-tag changed, and the stack narrowing code didn't adapt
> accordingly.  We need to change to default to consider generic objects
> as eq?-compared prompt tags.

I agree, but you still couldn't use procedures or integers as prompt
tags if you wanted make-stack to work, because those are special
cases. That's why I thought of just changing the interface to
make-stack to specify what you want - it's such a weird restriction
that someone could be bitten by it and have a lot of trouble tracking
it down. And because an argument can mean three different things, code
that uses make-stack is hard to understand (or at least it was for
me).

What do you think?

Noah

Re: Broken Backtraces, and Part of a Solution

[Andy Wingo]

On Wed 18 Apr 2012 18:08, Noah Lavine <noah.b.lavine@gmail.com> writes:

>> We need to change to default to consider generic objects as
>> eq?-compared prompt tags.
>
> I agree, but you still couldn't use procedures or integers as prompt
> tags if you wanted make-stack to work, because those are special
> cases.

Yeah, but the whole point of prompt tags is that you can make a new one
and know that it is eq?-unique, which is not the case for integers.  So
integers are not in the general case.  It seems useful to add procedures
as a special case too, no?

> That's why I thought of just changing the interface to make-stack to
> specify what you want - it's such a weird restriction that someone
> could be bitten by it and have a lot of trouble tracking it down. And
> because an argument can mean three different things, code that uses
> make-stack is hard to understand (or at least it was for me).

It's something of a nasty interface, I agree.  But it's been around for
a long time; if we can make a minimal change, we should, it seems to me.

Want to make a patch?

Regards,

Andy
-- 
http://wingolog.org/

Re: Broken Backtraces, and Part of a Solution

[Noah Lavine]

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

Here's a patch that fixes the bug for me. I'd also like to add a test
suite for the stack functions, to make sure this doesn't happen again,
but I'll look at that later.

Noah

On Wed, Apr 18, 2012 at 9:36 PM, Andy Wingo <wingo@pobox.com> wrote:
> On Wed 18 Apr 2012 18:08, Noah Lavine <noah.b.lavine@gmail.com> writes:
>
>>> We need to change to default to consider generic objects as
>>> eq?-compared prompt tags.
>>
>> I agree, but you still couldn't use procedures or integers as prompt
>> tags if you wanted make-stack to work, because those are special
>> cases.
>
> Yeah, but the whole point of prompt tags is that you can make a new one
> and know that it is eq?-unique, which is not the case for integers.  So
> integers are not in the general case.  It seems useful to add procedures
> as a special case too, no?
>
>> That's why I thought of just changing the interface to make-stack to
>> specify what you want - it's such a weird restriction that someone
>> could be bitten by it and have a lot of trouble tracking it down. And
>> because an argument can mean three different things, code that uses
>> make-stack is hard to understand (or at least it was for me).
>
> It's something of a nasty interface, I agree.  But it's been around for
> a long time; if we can make a minimal change, we should, it seems to me.
>
> Want to make a patch?
>
> Regards,
>
> Andy
> --
> http://wingolog.org/

[-- Attachment #2: 0001-make-stack-handles-prompt-tags-better.patch --]
[-- Type: application/octet-stream, Size: 4375 bytes --]

From d1beca7eac1958ee786f01af13df732c212cfd07 Mon Sep 17 00:00:00 2001
From: Noah Lavine <noah.b.lavine@gmail.com>
Date: Wed, 18 Apr 2012 22:10:21 -0400
Subject: [PATCH] make-stack handles prompt tags better

* libguile/stacks.c: update make-stack and narrow_stack to handle
  prompt tags that are not symbols.
---
 libguile/stacks.c |   66 ++++++++++++++++++++++++++--------------------------
 1 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/libguile/stacks.c b/libguile/stacks.c
index 13d347a..3f3f132 100644
--- a/libguile/stacks.c
+++ b/libguile/stacks.c
@@ -109,7 +109,7 @@ find_prompt (SCM key)
 }
 
 static void
-narrow_stack (SCM stack, long inner, SCM inner_key, long outer, SCM outer_key)
+narrow_stack (SCM stack, SCM inner_cut, SCM outer_cut)
 {
   unsigned long int len;
   SCM frame;
@@ -118,57 +118,67 @@ narrow_stack (SCM stack, long inner, SCM inner_key, long outer, SCM outer_key)
   frame = SCM_STACK_FRAME (stack);
 
   /* Cut inner part. */
-  if (scm_is_true (scm_procedure_p (inner_key)))
+  if (scm_is_true (scm_procedure_p (inner_cut)))
     {
       /* Cut until the given procedure is seen. */
-      for (; inner && len ; --inner)
+      for (; len ;)
         {
           SCM proc = scm_frame_procedure (frame);
           len--;
           frame = scm_frame_previous (frame);
-          if (scm_is_eq (proc, inner_key))
+          if (scm_is_eq (proc, inner_cut))
             break;
         }
     }
-  else if (scm_is_symbol (inner_key))
-    {
-      /* Cut until the given prompt tag is seen. FIXME, assumes prompt tags are
-         symbols. */
-      SCM *fp = find_prompt (inner_key);
-      for (; len; len--, frame = scm_frame_previous (frame))
-        if (fp == SCM_VM_FRAME_FP (frame) - SCM_VM_FRAME_OFFSET (frame))
-          break;
-    }
-  else
+  else if (scm_is_integer (inner_cut))
     {
       /* Cut specified number of frames. */
+      long inner = scm_to_int (inner_cut);
+      
       for (; inner && len; --inner)
         {
           len--;
           frame = scm_frame_previous (frame);
         }
     }
+  else
+    {
+      /* Cut until the given prompt tag is seen. */
+      SCM *fp = find_prompt (inner_cut);
+      for (; len; len--, frame = scm_frame_previous (frame))
+        if (fp == SCM_VM_FRAME_FP (frame) - SCM_VM_FRAME_OFFSET (frame))
+          break;
+    }
 
   SCM_SET_STACK_LENGTH (stack, len);
   SCM_SET_STACK_FRAME (stack, frame);
 
   /* Cut outer part. */
-  if (scm_is_true (scm_procedure_p (outer_key)))
+  if (scm_is_true (scm_procedure_p (outer_cut)))
     {
       /* Cut until the given procedure is seen. */
-      for (; outer && len ; --outer)
+      for (; len ;)
         {
           frame = scm_stack_ref (stack, scm_from_long (len - 1));
           len--;
-          if (scm_is_eq (scm_frame_procedure (frame), outer_key))
+          if (scm_is_eq (scm_frame_procedure (frame), outer_cut))
             break;
         }
     }
-  else if (scm_is_symbol (outer_key))
+  else if (scm_is_integer (outer_cut))
+    {
+      /* Cut specified number of frames. */
+      long outer = scm_to_int (outer_cut);
+      
+      if (outer < len)
+        len -= outer;
+      else
+        len = 0;
+    }
+  else
     {
-      /* Cut until the given prompt tag is seen. FIXME, assumes prompt tags are
-         symbols. */
-      SCM *fp = find_prompt (outer_key);
+      /* Cut until the given prompt tag is seen. */
+      SCM *fp = find_prompt (outer_cut);
       while (len)
         {
           frame = scm_stack_ref (stack, scm_from_long (len - 1));
@@ -177,14 +187,6 @@ narrow_stack (SCM stack, long inner, SCM inner_key, long outer, SCM outer_key)
             break;
         }
     }
-  else
-    {
-      /* Cut specified number of frames. */
-      if (outer < len)
-        len -= outer;
-      else
-        len = 0;
-    }
 
   SCM_SET_STACK_LENGTH (stack, len);
 }
@@ -308,10 +310,8 @@ SCM_DEFINE (scm_make_stack, "make-stack", 1, 0, 1,
 	}
       
       narrow_stack (stack,
-		    scm_is_integer (inner_cut) ? scm_to_int (inner_cut) : n,
-		    scm_is_integer (inner_cut) ? SCM_BOOL_T : inner_cut,
-		    scm_is_integer (outer_cut) ? scm_to_int (outer_cut) : n,
-		    scm_is_integer (outer_cut) ? SCM_BOOL_T : outer_cut);
+                    inner_cut,
+                    outer_cut);
 
       n = SCM_STACK_LENGTH (stack);
     }
-- 
1.7.6

Re: Broken Backtraces, and Part of a Solution

[Noah Lavine]

After looking at it more, there aren't really enough stack functions
to warrant a test suite. Any objections if I push this to master?

Noah

On Wed, Apr 18, 2012 at 10:13 PM, Noah Lavine <noah.b.lavine@gmail.com> wrote:
> Here's a patch that fixes the bug for me. I'd also like to add a test
> suite for the stack functions, to make sure this doesn't happen again,
> but I'll look at that later.
>
> Noah
>
> On Wed, Apr 18, 2012 at 9:36 PM, Andy Wingo <wingo@pobox.com> wrote:
>> On Wed 18 Apr 2012 18:08, Noah Lavine <noah.b.lavine@gmail.com> writes:
>>
>>>> We need to change to default to consider generic objects as
>>>> eq?-compared prompt tags.
>>>
>>> I agree, but you still couldn't use procedures or integers as prompt
>>> tags if you wanted make-stack to work, because those are special
>>> cases.
>>
>> Yeah, but the whole point of prompt tags is that you can make a new one
>> and know that it is eq?-unique, which is not the case for integers.  So
>> integers are not in the general case.  It seems useful to add procedures
>> as a special case too, no?
>>
>>> That's why I thought of just changing the interface to make-stack to
>>> specify what you want - it's such a weird restriction that someone
>>> could be bitten by it and have a lot of trouble tracking it down. And
>>> because an argument can mean three different things, code that uses
>>> make-stack is hard to understand (or at least it was for me).
>>
>> It's something of a nasty interface, I agree.  But it's been around for
>> a long time; if we can make a minimal change, we should, it seems to me.
>>
>> Want to make a patch?
>>
>> Regards,
>>
>> Andy
>> --
>> http://wingolog.org/

Re: Broken Backtraces, and Part of a Solution

[Andy Wingo]

On Thu 19 Apr 2012 19:47, Noah Lavine <noah.b.lavine@gmail.com> writes:

> After looking at it more, there aren't really enough stack functions
> to warrant a test suite. Any objections if I push this to master?

I haven't had time to look at it, but if you think it's right, please go
ahead.  :)

Cheers,

Andy
-- 
http://wingolog.org/

Re: Broken Backtraces, and Part of a Solution

[Ludovic Courtès]

Hi Noah,

Noah Lavine <noah.b.lavine@gmail.com> skribis:

> After looking at it more, there aren't really enough stack functions
> to warrant a test suite.

Sounds like a fallacious argument to me.  ;-)

Would it be possible for you to add test cases?  There are a few tests
under “stacks” in eval.test; perhaps something along these lines would
work?

Thanks,
Ludo’.

Re: Broken Backtraces, and Part of a Solution

[Noah Lavine]

Hello,

>> After looking at it more, there aren't really enough stack functions
>> to warrant a test suite.
>
> Sounds like a fallacious argument to me.  ;-)
>
> Would it be possible for you to add test cases?  There are a few tests
> under “stacks” in eval.test; perhaps something along these lines would
> work?

Yes, you're right. Those tests showed me how to get a stack. I pushed
the fix with two new test cases, and then cleaned up the stack-test
code a bit.

Thanks a lot,
Noah

Re: Broken Backtraces, and Part of a Solution

[Ludovic Courtès]

Noah Lavine <noah.b.lavine@gmail.com> skribis:

> Yes, you're right. Those tests showed me how to get a stack. I pushed
> the fix with two new test cases, and then cleaned up the stack-test
> code a bit.

Excellent, thank you!

Ludo’.