Merge pull request #9920 from 9999years/forbid-nested-debuggers

Forbid nested debuggers

(cherry picked from commit e164b39ee90fd655dbb7f479fdd4fbe38cc883bd)
Change-Id: Iff62f40fd251116516a63e2d3f9fb5b21480b16d

7 files changed, 93 insertions, 9 deletions

diff --git a/doc/manual/rl-next/forbid-nested-debuggers.md b/doc/manual/rl-next/forbid-nested-debuggers.md
new file mode 100644
index 000000000..a5924b24f
--- /dev/null
+++ b/doc/manual/rl-next/forbid-nested-debuggers.md
@@ -0,0 +1,32 @@
+---
+synopsis: Nested debuggers are no longer supported
+prs: 9920
+---
+
+Previously, evaluating an expression that throws an error in the debugger would
+enter a second, nested debugger:
+
+```
+nix-repl> builtins.throw "what"
+error: what
+
+
+Starting REPL to allow you to inspect the current state of the evaluator.
+
+Welcome to Nix 2.18.1. Type :? for help.
+
+nix-repl>
+```
+
+Now, it just prints the error message like `nix repl`:
+
+```
+nix-repl> builtins.throw "what"
+error:
+       … while calling the 'throw' builtin
+         at «string»:1:1:
+            1| builtins.throw "what"
+             | ^
+
+       error: what
+```
diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc
index ab7d7f18c..45b56d012 100644
--- a/src/libcmd/repl.cc
+++ b/src/libcmd/repl.cc
@@ -233,13 +233,7 @@ ReplExitStatus NixRepl::mainLoop()
               printMsg(lvlError, e.msg());
             }
         } catch (EvalError & e) {
-            // in debugger mode, an EvalError should trigger another repl session.
-            // when that session returns the exception will land here.  No need to show it again;
-            // show the error for this repl session instead.
-            if (state->debugRepl && !state->debugTraces.empty())
-                showDebugTrace(std::cout, state->positions, state->debugTraces.front());
-            else
-                printMsg(lvlError, e.msg());
+            printMsg(lvlError, e.msg());
         } catch (Error & e) {
             printMsg(lvlError, e.msg());
         } catch (Interrupted & e) {
diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index bef0effb6..b24f10c24 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -778,10 +778,24 @@ std::unique_ptr<ValMap> mapStaticEnvBindings(const SymbolTable & st, const Stati
     return vm;
 }
 
+/**
+ * Sets `inDebugger` to true on construction and false on destruction.
+ */
+class DebuggerGuard {
+    bool & inDebugger;
+public:
+    DebuggerGuard(bool & inDebugger) : inDebugger(inDebugger) {
+        inDebugger = true;
+    }
+    ~DebuggerGuard() {
+        inDebugger = false;
+    }
+};
+
 void EvalState::runDebugRepl(const Error * error, const Env & env, const Expr & expr)
 {
-    // double check we've got the debugRepl function pointer.
-    if (!debugRepl)
+    // Make sure we have a debugger to run and we're not already in a debugger.
+    if (!debugRepl || inDebugger)
         return;
 
     auto dts =
@@ -808,6 +822,7 @@ void EvalState::runDebugRepl(const Error * error, const Env & env, const Expr &
     auto se = getStaticEnv(expr);
     if (se) {
         auto vm = mapStaticEnvBindings(symbols, *se.get(), env);
+        DebuggerGuard _guard(inDebugger);
         auto exitStatus = (debugRepl)(ref<EvalState>(shared_from_this()), *vm);
         switch (exitStatus) {
             case ReplExitStatus::QuitAll:
diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh
index d2d8140e9..2291d618c 100644
--- a/src/libexpr/eval.hh
+++ b/src/libexpr/eval.hh
@@ -152,6 +152,7 @@ struct DebugTrace {
     bool isError;
 };
 
+
 class EvalState : public std::enable_shared_from_this<EvalState>
 {
 public:
@@ -210,6 +211,7 @@ public:
      */
     ReplExitStatus (* debugRepl)(ref<EvalState> es, const ValMap & extraEnv);
     bool debugStop;
+    bool inDebugger = false;
     int trylevel;
     std::list<DebugTrace> debugTraces;
     std::map<const Expr*, const std::shared_ptr<const StaticEnv>> exprEnvs;
diff --git a/tests/functional/repl_characterization/data/no_nested_debuggers.nix b/tests/functional/repl_characterization/data/no_nested_debuggers.nix
new file mode 100644
index 000000000..bf3ad7f0a
--- /dev/null
+++ b/tests/functional/repl_characterization/data/no_nested_debuggers.nix
@@ -0,0 +1 @@
+builtins.break {}
diff --git a/tests/functional/repl_characterization/data/no_nested_debuggers.test b/tests/functional/repl_characterization/data/no_nested_debuggers.test
new file mode 100644
index 000000000..5e834a68a
--- /dev/null
+++ b/tests/functional/repl_characterization/data/no_nested_debuggers.test
@@ -0,0 +1,39 @@
+we enter a debugger via builtins.break in the input file.
+
+  info: breakpoint reached
+
+causing another debugger even should not nest, but simply
+print the error, skip the breakpoint, etc as appropriate.
+
+  nix-repl> "values show"
+  "values show"
+
+  nix-repl> builtins.break 2
+  2
+
+  nix-repl> builtins.throw "foo"
+  error:
+         … while calling the 'throw' builtin
+           at «string»:1:1:
+              1| builtins.throw "foo"
+               | ^
+
+         error: foo
+
+  nix-repl> assert false; 2
+  error: assertion 'false' failed
+         at «string»:1:1:
+              1| assert false; 2
+               | ^
+
+exiting the debug frame should allow another to open.
+
+  nix-repl> :c
+
+  nix-repl> builtins.throw "bar"
+  error: bar
+
+and once again, more breakpoints are ignored.
+
+  nix-repl> builtins.break 3
+  3
diff --git a/tests/functional/repl_characterization/repl_characterization.cc b/tests/functional/repl_characterization/repl_characterization.cc
index d0c3b0a71..d46f09553 100644
--- a/tests/functional/repl_characterization/repl_characterization.cc
+++ b/tests/functional/repl_characterization/repl_characterization.cc
@@ -126,5 +126,6 @@ DEBUGGER_TEST(regression_9918);
 DEBUGGER_TEST(regression_9917);
 DEBUGGER_TEST(regression_l145);
 DEBUGGER_TEST(stack_vars);
+DEBUGGER_TEST(no_nested_debuggers);
 
 };