From 6c29016a0972f20cb0c91a4d9c8020f09baf6293 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= <7226587+thufschmitt@users.noreply.github.com> Date: Tue, 5 Mar 2024 06:58:29 +0100 Subject: [PATCH] Merge pull request #9920 from 9999years/forbid-nested-debuggers Forbid nested debuggers (cherry picked from commit e164b39ee90fd655dbb7f479fdd4fbe38cc883bd) Change-Id: Iff62f40fd251116516a63e2d3f9fb5b21480b16d --- doc/manual/rl-next/forbid-nested-debuggers.md | 32 +++++++++++++++ src/libcmd/repl.cc | 8 +--- src/libexpr/eval.cc | 19 ++++++++- src/libexpr/eval.hh | 2 + .../data/no_nested_debuggers.nix | 1 + .../data/no_nested_debuggers.test | 39 +++++++++++++++++++ .../repl_characterization.cc | 1 + 7 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 doc/manual/rl-next/forbid-nested-debuggers.md create mode 100644 tests/functional/repl_characterization/data/no_nested_debuggers.nix create mode 100644 tests/functional/repl_characterization/data/no_nested_debuggers.test 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 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(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 { public: @@ -210,6 +211,7 @@ public: */ ReplExitStatus (* debugRepl)(ref es, const ValMap & extraEnv); bool debugStop; + bool inDebugger = false; int trylevel; std::list debugTraces; std::map> 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); };