From e41ee54d211c03e003abda2678e4bffa8866f897 Mon Sep 17 00:00:00 2001
From: Bernd Wachter <bernd-github@wachter.fi>
Date: Thu, 12 Nov 2020 13:54:53 +0200
Subject: [PATCH] Add optional error handling to si_safe_eval

Currently eql5 has a tendency to deadlock when encountering errors,
especially when executing scripts. The problem is that si_safe_eval
when called without a return flag expects to have errors handled in a
debugger, which does not exist in this case. As a result the lisp
runtime dies, while the code moves on to starting the Qt event loop
with nothing to control it.

This change introduces three flags to control the error handling
behaviour for eval:

- DebugOnError expects a debugger to be there, which is the old
  behaviour. It can be restored by passing -debug-on-error to eql5
- LogOnError will log about the error, but does not handle it. This is
  mainly a placeholder for better error handling in the future -
  without error handling this also results in a deadlock.
- DieOnError is the new default, which logs the code leading to the
  error, and exits with an error code.

Another patch should expose those flags to the Lisp runtime, and make
the error behaviour configurable both at initializing EQL from C++ and
later on during runtime.

A drawback of this change that we only get to see the the expression
triggering the error, not the actual error - that one is thrown away
by safe-eval:
                             #'(lambda (condition)
                                 (declare (ignore condition))
                                 (return-from safe-eval err-value))

I still feel for working with eql5 this is an overall
improvement. Next steps on this issue should include:

- either provide debugger hooks, or override invoke-debugger to behave
  more sensibly
- possibly override/provide a custom variant of safe-eval to print the
  error before returning
- check a few locations to set more sensible defaults, i.e. if
  somebody just runl eql5 without a script they'd probably want to end
  up in the debugger on errors.
---
 src/eql.cpp    | 22 ++++++++++++++++++++--
 src/eql5/eql.h | 10 +++++++++-
 src/main.cpp   |  2 +-
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/eql.cpp b/src/eql.cpp
index 0213a63..7a551cb 100644
--- a/src/eql.cpp
+++ b/src/eql.cpp
@@ -7,6 +7,7 @@
 #include <QApplication>
 #include <QTimer>
 #include <QStringList>
+#include <QDebug>
 
 const char EQL::version[] = "20.7.1"; // July 2020
 
@@ -54,9 +55,22 @@ void EQL::ini(int argc, char** argv) {
     cl_booted = true;
     cl_boot(argc, argv); }
 
-void EQL::eval(const char* lisp_code) {
+void EQL::eval(const char* lisp_code, const EvalMode mode) {
     CL_CATCH_ALL_BEGIN(ecl_process_env()) {
-        si_safe_eval(2, ecl_read_from_cstring((char*)lisp_code), Cnil); }
+        switch(mode) {
+            case DebugOnError:
+                si_safe_eval(2, ecl_read_from_cstring((char*)lisp_code), Cnil);
+                break;
+            case LogOnError:
+            case DieOnError:
+                cl_object ret = si_safe_eval(3, ecl_read_from_cstring((char*)lisp_code), Cnil, ecl_make_fixnum(EVAL_ERROR_VALUE));
+                if (ecl_t_of(ret) == t_fixnum && fix(ret) == EVAL_ERROR_VALUE) {
+                    qDebug()<<"Error evaluating " <<lisp_code;
+                    if (mode == DieOnError)
+                        exit(-1);
+                }
+        }
+    }
     CL_CATCH_ALL_END; }
 
 void EQL::ignoreIOStreams() {
@@ -74,6 +88,9 @@ void EQL::exec(const QStringList& args) {
         arguments.removeAll("-norc"); }
     else {
         eval("(x:when-it (probe-file \"~/.eclrc\") (load x:it))"); }
+    if (arguments.contains("-debug-on-error")) {
+        arguments.removeAll("-debug-on-error");
+        evalMode = DebugOnError; }
     // Slime
     int i_swank = arguments.indexOf(QRegExp("*start-swank*.lisp", Qt::CaseInsensitive, QRegExp::Wildcard));
     if(arguments.contains("-slime") || (i_swank != -1))  {
@@ -193,6 +210,7 @@ void EQL::runOnUiThread(void* function_or_closure) {
         CL_UNWIND_PROTECT_END; }
     CL_CATCH_ALL_END; }
 
+EQL::EvalMode EQL::evalMode = DieOnError;
 bool EQL::cl_booted = false;
 bool EQL::return_value_p = false;
 bool EQL::qexec = true;
diff --git a/src/eql5/eql.h b/src/eql5/eql.h
index 00381da..636d63c 100644
--- a/src/eql5/eql.h
+++ b/src/eql5/eql.h
@@ -17,6 +17,7 @@ QT_BEGIN_NAMESPACE
 
 #define QSLOT(x)   "1"#x
 #define QSIGNAL(x) "2"#x
+#define EVAL_ERROR_VALUE -1
 
 typedef void (*lisp_ini)(cl_object);
 
@@ -25,6 +26,12 @@ class EQL_EXPORT EQL : public QObject {
 public:
     EQL();
 
+    enum EvalMode {
+        DebugOnError,
+        LogOnError,
+        DieOnError,
+    };
+
     static bool cl_booted;
     static bool return_value_p;
     static bool qexec;
@@ -32,7 +39,8 @@ public:
     static QEventLoop* eventLoop;
     static void ini(int, char**);
     static void ini(char**);
-    static void eval(const char*);
+    static void eval(const char*, const EvalMode = evalMode);
+    static EvalMode evalMode;
 
     void exec(const QStringList&);
     void exec(lisp_ini, const QByteArray& = "nil", const QByteArray& = "eql-user"); // see my_app example
diff --git a/src/main.cpp b/src/main.cpp
index bc8771a..b0bd7f1 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -61,7 +61,7 @@ int main(int argc, char** argv) {
     QApplication qapp(argc, argv);
     QStringList args(QCoreApplication::arguments());
     if(args.contains("-h") || (args.contains("--help"))) {
-        std::cout << "Usage: eql5 [file] [-qtpl] [-qgui] [-quic file.ui [:ui-package] [:maximized]] [-slime] [-norc]" << std::endl;
+        std::cout << "Usage: eql5 [file] [-qtpl] [-qgui] [-quic file.ui [:ui-package] [:maximized]] [-slime] [-norc] [-debug-on-error]" << std::endl;
         exit(0); }
     ini();