[armedbear-devel] Serious bug involving UWP
Steps to reproduce: * ABCL from svn, updated a few minutes ago. * Slime from CVS. * M-x slime-scratch * Put (defun test (x) ,bar) in the buffer * Press C-c C-c You'll see "Evaluation aborted." in the minibuffer even though the debugger should have popped up complaining about comma not being inside backquote. C-c C-c is SWANK-COMPILE-STRING in swank-abcl.lisp. If you follow the control flow, you'll see that in SLDB-LOOP (we come here through *DEBUGGER-HOOK*) in swank.lisp, __the UNWINDED-PROTECT's protected form is *never* executed and it goes straight to the cleanup forms.__ -T.
On 9/17/09 10:56 AM, Tobias C. Rittweiler wrote:
C-c C-c is SWANK-COMPILE-STRING in swank-abcl.lisp.
If you follow the control flow, you'll see that in SLDB-LOOP (we come here through *DEBUGGER-HOOK*) in swank.lisp,
__the UNWINDED-PROTECT's protected form is *never* executed and it goes straight to the cleanup forms.__
Hmmm, I don't come up with the same analysis of the control flow. I don't see the code going through SLDB-LOOP at all, but rather bombing out in swank-abcl.lisp's implementation of SWANK-COMPILE-STRING at line 396 because in ABCL the comma not inside a backquote is a READER-ERROR which is not a subtype of the WARNING referenced in the enclosing HANDLER-BIND. The actual error is coming from READ-FROM-STRING: (defimplementation swank-compile-string (string &key buffer position filename policy) (declare (ignore filename policy)) (let ((jvm::*resignal-compiler-warnings* t) (*abcl-signaled-conditions* nil)) (handler-bind ((warning #'handle-compiler-warning)) (let ((*buffer-name* buffer) (*buffer-start-position* position) (*buffer-string* string)) (funcall (compile nil (read-from-string (format nil "(~S () ~A)" 'lambda string)))) t)))) One can reproduce the error from the SLIME REPL by evaluating (SWANK-BACKEND:SWANK-COMPILE-STRING "(defun test (x) ,bar)" :BUFFER "*slime-scratch*" :POSITION 1 :FILENAME NIL :POLICY NIL) If I bind a handler for ERROR in the enclosing HANDLER-BIND, I do trap the error. Tobias: are you relying on visual inspection of the problem here, or are you getting a backtrace somehow? I didn't understand your comment "(we come here through *DEBUGGER-HOOK*)". SBCL writes the string to a temporary file, then compiles that file. Since the ABCL code for compiling a file works ok, I would maybe pursue that route. -- "A screaming comes across the sky. It has happened before, but there is nothing to compare to it now."
Mark Evenson writes:
On 9/17/09 10:56 AM, Tobias C. Rittweiler wrote:
C-c C-c is SWANK-COMPILE-STRING in swank-abcl.lisp.
If you follow the control flow, you'll see that in SLDB-LOOP (we come here through *DEBUGGER-HOOK*) in swank.lisp,
__the UNWINDED-PROTECT's protected form is *never* executed and it goes straight to the cleanup forms.__
Hmmm, I don't come up with the same analysis of the control flow. I don't see the code going through SLDB-LOOP at all, but rather bombing out in swank-abcl.lisp's implementation of SWANK-COMPILE-STRING at line 396 because in ABCL the comma not inside a backquote is a READER-ERROR which is not a subtype of the WARNING referenced in the enclosing HANDLER-BIND. The actual error is coming from READ-FROM-STRING:
One can reproduce the error from the SLIME REPL by evaluating
(SWANK-BACKEND:SWANK-COMPILE-STRING "(defun test (x) ,bar)" :BUFFER "*slime-scratch*" :POSITION 1 :FILENAME NIL :POLICY NIL)
If I bind a handler for ERROR in the enclosing HANDLER-BIND, I do trap the error.
Tobias: are you relying on visual inspection of the problem here, or are you getting a backtrace somehow? I didn't understand your comment "(we come here through *DEBUGGER-HOOK*)".
*DEBUGGER-HOOK* is set to SWANK-DEBUGGER-HOOK which calls DEBUG-IN-EMACS which calls SLDB-LOOP. READ-FROM-STRING calls ERROR which calls INVOKE-DEBUGGER which calls the function in *DEBUGGER-HOOK* Does that make more sense? Just insert some FORMAT calls to SLDB-LOOP, in particular one before the UWP, and one as the first form of the protected form.
SBCL writes the string to a temporary file, then compiles that file. Since the ABCL code for compiling a file works ok, I would maybe pursue that route.
Yes, that's the way it should be implemented for other reasons: COMPILE does not process source as file compiler, think of EVAL-WHEN. See my comment at http://trac.common-lisp.net/armedbear/ticket/56 for the proper way of how to implement SWANK-COMPILE-STRING. I'll make it go the route over a temp file, but the UWP issue should be resolved first otherwise I'd remove the way to reproduce this issue. -T.
On 9/17/09 2:33 PM, Tobias C. Rittweiler wrote:
Tobias: are you relying on visual inspection of the problem here, or are you getting a backtrace somehow? I didn't understand your comment "(we come here through *DEBUGGER-HOOK*)".
*DEBUGGER-HOOK* is set to SWANK-DEBUGGER-HOOK which calls DEBUG-IN-EMACS which calls SLDB-LOOP.
READ-FROM-STRING calls ERROR which calls INVOKE-DEBUGGER which calls the function in *DEBUGGER-HOOK*
Does that make more sense?
Yep: thanks for helping ol' thick-in-the-head here.
Just insert some FORMAT calls to SLDB-LOOP, in particular one before the UWP, and one as the first form of the protected form.
If I patch swank.lisp as attached I see SLDB-LOOP-1 SLDB-LOOP-2 SLDB-LOOP-3 SLDB-LOOP-6 SLDB-LOOP-7 SLDB-LOOP-8 indicating that the SLDB-LOOP UNWIND-PROTECT is executing part of the protected forms, and then going to the cleanup forms. Or am I misinterpreting something again?
SBCL writes the string to a temporary file, then compiles that file. Since the ABCL code for compiling a file works ok, I would maybe pursue that route.
Yes, that's the way it should be implemented for other reasons: COMPILE does not process source as file compiler, think of EVAL-WHEN.
See my comment at http://trac.common-lisp.net/armedbear/ticket/56 for the proper way of how to implement SWANK-COMPILE-STRING.
Eliminating the use of temporary files in the compiler as you suggest in your note to ticket #56 is needed in the near future anyways so ABCL can operate in the absence of a filesystem, so implementing COMPILE-FROM-STREAM is a good idea anyways.
I'll make it go the route over a temp file, but the UWP issue should be resolved first otherwise I'd remove the way to reproduce this issue.
Can you refactor the UNWIND-PROTECT problem into non-SLIME code somehow? -- "A screaming comes across the sky. It has happened before, but there is nothing to compare to it now."
Mark Evenson writes:
Just insert some FORMAT calls to SLDB-LOOP, in particular one before the UWP, and one as the first form of the protected form.
If I patch swank.lisp as attached I see
SLDB-LOOP-1 SLDB-LOOP-2 SLDB-LOOP-3 SLDB-LOOP-6 SLDB-LOOP-7 SLDB-LOOP-8
indicating that the SLDB-LOOP UNWIND-PROTECT is executing part of the protected forms, and then going to the cleanup forms. Or am I misinterpreting something again?
So you see an *sldb ...* buffer if you C-c C-c the original form? What platform / Java version do you use? I'm on Ubuntu Linux x86-32, with Java 1.6.0_16.
I'll make it go the route over a temp file, but the UWP issue should be resolved first otherwise I'd remove the way to reproduce this issue.
Can you refactor the UNWIND-PROTECT problem into non-SLIME code somehow?
I wouldn't know how. It's strange because C-c C-k _does_ pop up an SLDB. -T.
On 9/17/09 4:03 PM, Tobias C. Rittweiler wrote: […]
So you see an *sldb ...* buffer if you C-c C-c the original form?
Nope. I see "Evaluation aborted." flash in the mini-buffer
What platform / Java version do you use?
Mac OS X 10.6.1 with Java 1.6.0_15 under Emacs 23.1. Hmmm. Anyone else out there able to report what sort of behavior they see with 'swank.lisp' patched as in my previous post? I'm firing up Ubuntu under VirtualBox to see if I see different behavior, but I have to set up ABCL/SLIME plus Java so it could take a bit. -- "A screaming comes across the sky. It has happened before, but there is nothing to compare to it now."
Mark Evenson <evenson@panix.com> writes:
On 9/17/09 4:03 PM, Tobias C. Rittweiler wrote: […]
So you see an *sldb ...* buffer if you C-c C-c the original form?
Nope. I see "Evaluation aborted." flash in the mini-buffer
Huh? That doesn't match with the output you got. Will you be on IRC later? -T.
"Tobias C. Rittweiler" writes:
Steps to reproduce:
* ABCL from svn, updated a few minutes ago.
* Slime from CVS.
* M-x slime-scratch
* Put (defun test (x) ,bar) in the buffer
* Press C-c C-c
You'll see "Evaluation aborted." in the minibuffer even though the debugger should have popped up complaining about comma not being inside backquote.
C-c C-c is SWANK-COMPILE-STRING in swank-abcl.lisp.
If you follow the control flow, you'll see that in SLDB-LOOP (we come here through *DEBUGGER-HOOK*) in swank.lisp,
__the UNWINDED-PROTECT's protected form is *never* executed and it goes straight to the cleanup forms.__
Ok, I found the root of the issue: The java code behind SYS:FRAME-TO-STRING created conses whose CAR ptr is NULL. The printer will barf on such things, of course. A patch which fixes this issue is attached. There is another issue I'd like to raise. The reason for the funny behaviour, and why it consumed a lot of time to track this bug down is because there's no global handler which prints a Stack Trace for uncaught Exceptions. Instead of such a global handler, local "catch (...) { Debug.Trace ... }" are sprinkled all over the code base. And, obviously, they're easy to miss---as it has been the case here. So the exception is swept under the carpet and ABCL seems to unwind the stack to some safe point, probably the toplevel, and proceed from there. -T.
On 9/17/09 10:36 PM, Tobias C. Rittweiler wrote:
Ok, I found the root of the issue: The java code behind SYS:FRAME-TO-STRING created conses whose CAR ptr is NULL. The printer will barf on such things, of course.
A patch which fixes this issue is attached.
Applied to trunk as [svn 12149][1], and backported to 0.16 as [svn 12150][2]. Thanks for tracking this down! We should probably start thinking about a 0.16.1 release. [1]: http://trac.common-lisp.net/armedbear/changeset/12149 [2]: http://trac.common-lisp.net/armedbear/changeset/12150 -- "A screaming comes across the sky. It has happened before, but there is nothing to compare to it now."
On 9/17/09 10:36 PM, Tobias C. Rittweiler wrote:
There is another issue I'd like to raise.
The reason for the funny behaviour, and why it consumed a lot of time to track this bug down is because there's no global handler which prints a Stack Trace for uncaught Exceptions.
Instead of such a global handler, local "catch (...) { Debug.Trace ... }" are sprinkled all over the code base. And, obviously, they're easy to miss---as it has been the case here.
So the exception is swept under the carpet and ABCL seems to unwind the stack to some safe point, probably the toplevel, and proceed from there.
Acknowledged. The current codebase seemingly suffers from a lack of a coherent plan for how to deal with exceptions, at least to my understanding of it. I think the use of 'org.armedbear.lisp.ConditionThrowable' subclassing 'java.lang.Throwable' should be reconsidered, to make more consistent use of the [exception chaining mechanism introduced in Java 1.4][1]. But going through the code to remove all the instances of swallowing exceptions to at least print diagnostic information should be rather easily gathered low hanging fruit. [1]: http://java.sun.com/j2se/1.5.0/docs/api/java/lang/Throwable.html -- "A screaming comes across the sky. It has happened before, but there is nothing to compare to it now."
Hi Mark, On Fri, Sep 18, 2009 at 8:50 AM, Mark Evenson <evenson@panix.com> wrote:
On 9/17/09 10:36 PM, Tobias C. Rittweiler wrote:
There is another issue I'd like to raise.
The reason for the funny behaviour, and why it consumed a lot of time to track this bug down is because there's no global handler which prints a Stack Trace for uncaught Exceptions.
Instead of such a global handler, local "catch (...) { Debug.Trace ... }" are sprinkled all over the code base. And, obviously, they're easy to miss---as it has been the case here.
So the exception is swept under the carpet and ABCL seems to unwind the stack to some safe point, probably the toplevel, and proceed from there.
Acknowledged.
The current codebase seemingly suffers from a lack of a coherent plan for how to deal with exceptions, at least to my understanding of it. I think the use of 'org.armedbear.lisp.ConditionThrowable' subclassing 'java.lang.Throwable' should be reconsidered, to make more consistent use of the [exception chaining mechanism introduced in Java 1.4][1].
I've thought about that for a bit, but I don't understand what you mean. Do we agree that ConditionThrowable is the superclass to Go, Return, Throw and ThreadDestroyed only? (And not to any of the Lisp exceptions.)
But going through the code to remove all the instances of swallowing exceptions to at least print diagnostic information should be rather easily gathered low hanging fruit.
The ConditionThrowable should probably be named TransferOfControl instead, is what I'm now thinking. That would add to the "self-explanatory" level in the sources, I'd say. Bye, Erik.
I created ticket #64 to track the general issue pointed out by Mark. Bye, Erik. On Sun, Oct 18, 2009 at 12:24 PM, Erik Huelsmann <ehuels@gmail.com> wrote:
Hi Mark,
On Fri, Sep 18, 2009 at 8:50 AM, Mark Evenson <evenson@panix.com> wrote:
On 9/17/09 10:36 PM, Tobias C. Rittweiler wrote:
There is another issue I'd like to raise.
The reason for the funny behaviour, and why it consumed a lot of time to track this bug down is because there's no global handler which prints a Stack Trace for uncaught Exceptions.
Instead of such a global handler, local "catch (...) { Debug.Trace ... }" are sprinkled all over the code base. And, obviously, they're easy to miss---as it has been the case here.
So the exception is swept under the carpet and ABCL seems to unwind the stack to some safe point, probably the toplevel, and proceed from there.
Acknowledged.
The current codebase seemingly suffers from a lack of a coherent plan for how to deal with exceptions, at least to my understanding of it. I think the use of 'org.armedbear.lisp.ConditionThrowable' subclassing 'java.lang.Throwable' should be reconsidered, to make more consistent use of the [exception chaining mechanism introduced in Java 1.4][1].
I've thought about that for a bit, but I don't understand what you mean. Do we agree that ConditionThrowable is the superclass to Go, Return, Throw and ThreadDestroyed only? (And not to any of the Lisp exceptions.)
But going through the code to remove all the instances of swallowing exceptions to at least print diagnostic information should be rather easily gathered low hanging fruit.
The ConditionThrowable should probably be named TransferOfControl instead, is what I'm now thinking. That would add to the "self-explanatory" level in the sources, I'd say.
Bye,
Erik.
2009/10/18 Erik Huelsmann <ehuels@gmail.com>:
The ConditionThrowable should probably be named TransferOfControl instead, is what I'm now thinking. That would add to the "self-explanatory" level in the sources, I'd say.
For the superclass of GO/RETURN/etc. yes, but I don't think we should get rid of ConditionThrowable. ConditionThrowable is actually not a good superclass for GO/RETURN/etc., because I'd expect Java code to try and catch conditions, but such catches should IMO not be mixed with catching GO/etc. To me it looks like we need two superclasses, ConditionThrowable for conditions, TransferOfControl for others.
On Sun, Oct 18, 2009 at 7:35 PM, Ville Voutilainen <ville.voutilainen@gmail.com> wrote:
2009/10/18 Erik Huelsmann <ehuels@gmail.com>:
The ConditionThrowable should probably be named TransferOfControl instead, is what I'm now thinking. That would add to the "self-explanatory" level in the sources, I'd say.
For the superclass of GO/RETURN/etc. yes, but I don't think we should get rid of ConditionThrowable. ConditionThrowable is actually not a good superclass for GO/RETURN/etc., because I'd expect Java code to try and catch conditions, but such catches should IMO not be mixed with catching GO/etc. To me it looks like we need two superclasses, ConditionThrowable for conditions, TransferOfControl for others.
This is exactly where ConditionThrowable is confusing. It's also where I thought your thoughts may take the wrong turn: Java code can't actually catch Lisp conditions: Condition (and all its decendants, apart from user defined ones) descend from StandardObject, which descends from LispObject. They are not Throwable at all! So, since there are no lisp conditions going to be caught in the catch statements, can we agree that ConditionThrowable is just a confusing misnomer? Bye, Erik.
participants (4)
-
Erik Huelsmann -
Mark Evenson -
Tobias C. Rittweiler -
Ville Voutilainen