libexpr: handle debug trace frames as parent pointer list

this also fixes a debugger bug where leaving the debugger does not clean
up old debugger state completely. in such cases the fake frame withFrame
created was left behind after the corresponding caller frame was unwound

Change-Id: I45adcd116276b03b2f87076518c9eae6fe844e06

lix/libcmd/repl.cc

@@ -157,7 +157,7 @@ struct NixRepl
     void addVarToScope(const Symbol name, Value & v);
     Expr & parseString(std::string s);
     void evalString(std::string s, Value & v);
-    void loadDebugTraceEnv(DebugTrace & dt);
+    void loadDebugTraceEnv(const DebugTrace & dt);
 
     /**
      * Load the `repl-overlays` and add the resulting AttrSet to the top-level
@@ -482,7 +482,7 @@ StorePath NixRepl::getDerivationPath(Value & v) {
     return *drvPath;
 }
 
-void NixRepl::loadDebugTraceEnv(DebugTrace & dt)
+void NixRepl::loadDebugTraceEnv(const DebugTrace & dt)
 {
     initEnv();
 
@@ -558,16 +558,18 @@ ProcessLineResult NixRepl::processLine(std::string line)
     }
 
     else if (state.debug && state.debug->inDebugger && (command == ":bt" || command == ":backtrace")) {
-        for (const auto & [idx, i] : enumerate(state.debug->traces)) {
+        auto traces = state.debug->traces();
+        for (const auto & [idx, i] : enumerate(traces)) {
             std::cout << "\n" << ANSI_BLUE << idx << ANSI_NORMAL << ": ";
-            showDebugTrace(std::cout, state.positions, i);
+            showDebugTrace(std::cout, state.positions, *i);
         }
     }
 
     else if (state.debug && state.debug->inDebugger && (command == ":env")) {
-        for (const auto & [idx, i] : enumerate(state.debug->traces)) {
+        auto traces = state.debug->traces();
+        for (const auto & [idx, i] : enumerate(traces)) {
             if (idx == debugTraceIndex) {
-                printEnvBindings(state, i.expr, i.env);
+                printEnvBindings(state, i->expr, i->env);
                 break;
             }
         }
@@ -579,13 +581,14 @@ ProcessLineResult NixRepl::processLine(std::string line)
             debugTraceIndex = stoi(arg);
         } catch (...) { }
 
-        for (const auto & [idx, i] : enumerate(state.debug->traces)) {
+        auto traces = state.debug->traces();
+        for (const auto & [idx, i] : enumerate(traces)) {
              if (idx == debugTraceIndex) {
                  std::cout << "\n" << ANSI_BLUE << idx << ANSI_NORMAL << ": ";
-                 showDebugTrace(std::cout, state.positions, i);
+                 showDebugTrace(std::cout, state.positions, *i);
                  std::cout << std::endl;
-                 printEnvBindings(state, i.expr, i.env);
+                 printEnvBindings(state, i->expr, i->env);
-                 loadDebugTraceEnv(i);
+                 loadDebugTraceEnv(*i);
                  break;
              }
         }

lix/libexpr/eval-error.cc

@@ -42,17 +42,14 @@ EvalErrorBuilder<T> & EvalErrorBuilder<T>::withSuggestions(Suggestions & s)
 template<class T>
 EvalErrorBuilder<T> & EvalErrorBuilder<T>::withFrame(const Env & env, const Expr & expr)
 {
-    // NOTE: This is abusing side-effects.
-    // TODO: check compatibility with nested debugger calls.
-    // TODO: What side-effects??
     if (error.state.debug) {
-        error.state.debug->traces.push_front(DebugTrace{
+        error.frame = error.state.debug->addTrace(DebugTrace{
             .pos = error.state.positions[expr.getPos()],
             .expr = expr,
             .env = env,
             .hint = HintFmt("Fake frame for debugging purposes"),
             .isError = true
-        });
+        }).entry;
     }
     return *this;
 }
@@ -77,12 +74,13 @@ EvalErrorBuilder<T>::addTrace(PosIdx pos, std::string_view formatString, const A
 template<class T>
 void EvalErrorBuilder<T>::debugThrow()
 {
-    if (error.state.debug && !error.state.debug->traces.empty()) {
+    if (error.state.debug) {
-        const DebugTrace & last = error.state.debug->traces.front();
+        if (auto last = error.state.debug->traces().next()) {
-        const Env * env = &last.env;
+            const Env * env = &(*last)->env;
-        const Expr * expr = &last.expr;
+            const Expr * expr = &(*last)->expr;
             error.state.debug->runDebugRepl(error.state, &error, *env, *expr);
         }
+    }
 
     // `EvalState` is the only class that can construct an `EvalErrorBuilder`,
     // and it does so in dynamic storage. This is the final method called on

lix/libexpr/eval-error.hh

@@ -7,6 +7,7 @@
 
 namespace nix {
 
+struct DebugTrace;
 struct Env;
 struct Expr;
 struct Value;
@@ -19,6 +20,9 @@ class EvalError : public Error
 {
     template<class T>
     friend class EvalErrorBuilder;
+
+    std::shared_ptr<const DebugTrace> frame;
+
 public:
     EvalState & state;
 

lix/libexpr/eval.cc

@@ -646,9 +646,7 @@ void DebugState::runDebugRepl(
 
     auto dts =
         error && expr.getPos()
-        ? std::make_unique<DebugTraceStacker>(
+        ? addTrace(DebugTrace {
-            evalState,
-            DebugTrace {
             .pos = error->info().pos ? error->info().pos : evalState.positions[expr.getPos()],
             .expr = expr,
             .env = env,
@@ -695,31 +693,45 @@ void EvalState::addErrorTrace(Error & e, const PosIdx pos, const Args & ... form
     e.addTrace(positions[pos], HintFmt(formatArgs...));
 }
 
+DebugState::TraceFrame DebugState::addTrace(DebugTrace t)
+{
+    struct UnlinkDebugTrace
+    {
+        DebugState * state;
+        void operator()(DebugTrace * trace)
+        {
+            state->latestTrace = trace->parent;
+            delete trace;
+        }
+    };
+
+    t.parent = latestTrace.lock();
+    std::unique_ptr<DebugTrace, UnlinkDebugTrace> trace(
+        new auto(std::move(t)), UnlinkDebugTrace{.state = this}
+    );
+    std::shared_ptr<DebugTrace> entry(std::move(trace));
+    latestTrace = entry;
+    return TraceFrame{entry};
+}
+
 template<typename... Args>
-static std::unique_ptr<DebugTraceStacker> makeDebugTraceStacker(
+static DebugState::TraceFrame makeDebugTraceStacker(
     EvalState & state,
     Expr & expr,
     Env & env,
     std::shared_ptr<Pos> && pos,
     const Args & ... formatArgs)
 {
-    return std::make_unique<DebugTraceStacker>(state,
+    auto trace = state.debug->addTrace(DebugTrace{
-        DebugTrace {
         .pos = std::move(pos),
         .expr = expr,
         .env = env,
         .hint = HintFmt(formatArgs...),
-            .isError = false
+        .isError = false,
     });
-}
+    if (state.debug->stop && state.debug->repl)
-
+        state.debug->runDebugRepl(state, nullptr, env, expr);
-DebugTraceStacker::DebugTraceStacker(EvalState & evalState, DebugTrace t)
+    return trace;
-    : evalState(evalState)
-    , trace(std::move(t))
-{
-    evalState.debug->traces.push_front(trace);
-    if (evalState.debug->stop && evalState.debug->repl)
-        evalState.debug->runDebugRepl(evalState, nullptr, trace.env, trace.expr);
 }
 
 inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval)

lix/libexpr/eval.hh

@@ -4,6 +4,7 @@
 #include "lix/libexpr/attr-set.hh"
 #include "lix/libexpr/eval-error.hh"
 #include "lix/libexpr/gc-alloc.hh"
+#include "lix/libutil/generator.hh"
 #include "lix/libutil/types.hh"
 #include "lix/libexpr/value.hh"
 #include "lix/libexpr/nixexpr.hh"
@@ -150,14 +151,18 @@ struct DebugTrace {
     const Env & env;
     HintFmt hint;
     bool isError;
+    std::shared_ptr<const DebugTrace> parent;
 };
 
 struct DebugState
 {
+private:
+    std::weak_ptr<const DebugTrace> latestTrace;
+
+public:
     std::function<ReplExitStatus(EvalState & es, ValMap const & extraEnv)> repl;
     bool stop = false;
     bool inDebugger = false;
-    std::list<DebugTrace> traces;
     std::map<const Expr *, const std::shared_ptr<const StaticEnv>> exprEnvs;
     int trylevel = 0;
 
@@ -178,8 +183,35 @@ struct DebugState
         }
         return nullptr;
     }
-};
 
+    class TraceFrame
+    {
+        friend struct DebugState;
+        template<class T>
+        friend class EvalErrorBuilder;
+
+        // holds both the data for this frame *and* a deleter that pulls this frame
+        // off the trace stack. EvalErrorBuilder uses this for withFrame fake trace
+        // frames, and to avoid needing to see this class definition in its header.
+        const std::shared_ptr<const DebugTrace> entry = nullptr;
+
+        explicit TraceFrame(std::shared_ptr<const DebugTrace> entry): entry(std::move(entry)) {}
+
+    public:
+        TraceFrame(std::nullptr_t) {}
+    };
+
+    TraceFrame addTrace(DebugTrace t);
+
+    /// Enumerates the debug frame stack, from the current frame to the root frame.
+    /// All values are guaranteed to not be null, but must be pointers because C++.
+    Generator<const DebugTrace *> traces()
+    {
+        for (auto current = latestTrace.lock(); current; current = current->parent) {
+            co_yield current.get();
+        }
+    }
+};
 
 struct StaticSymbols
 {
@@ -761,16 +793,6 @@ private:
     friend struct Value;
 };
 
-struct DebugTraceStacker {
-    DebugTraceStacker(EvalState & evalState, DebugTrace t);
-    ~DebugTraceStacker()
-    {
-        evalState.debug->traces.pop_front();
-    }
-    EvalState & evalState;
-    DebugTrace trace;
-};
-
 /**
  * @return A string representing the type of the value `v`.
  *

lix/libexpr/primops.cc

@@ -566,15 +566,14 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a
 
 static void prim_break(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    if (state.debug && !state.debug->traces.empty()) {
+    if (auto trace = state.debug ? state.debug->traces().next() : std::nullopt) {
         auto error = EvalError(state, ErrorInfo {
             .level = lvlInfo,
             .msg = HintFmt("breakpoint reached"),
             .pos = state.positions[pos],
         });
 
-        auto & dt = state.debug->traces.front();
+        state.debug->runDebugRepl(state, &error, (*trace)->env, (*trace)->expr);
-        state.debug->runDebugRepl(state, &error, dt.env, dt.expr);
     }
 
     // Return the value we were passed.
@@ -696,9 +695,11 @@ static void prim_trace(EvalState & state, const PosIdx pos, Value * * args, Valu
         printError("trace: %1%", args[0]->string.s);
     else
         printError("trace: %1%", ValuePrinter(state, *args[0]));
-    if (evalSettings.builtinsTraceDebugger && state.debug && !state.debug->traces.empty()) {
+    if (auto last = evalSettings.builtinsTraceDebugger && state.debug
-        const DebugTrace & last = state.debug->traces.front();
+            ? state.debug->traces().next()
-        state.debug->runDebugRepl(state, nullptr, last.env, last.expr);
+            : std::nullopt)
+    {
+        state.debug->runDebugRepl(state, nullptr, (*last)->env, (*last)->expr);
     }
     state.forceValue(*args[1], pos);
     v = *args[1];

tests/functional/repl_characterization/data/debug_frames.test

@@ -71,3 +71,43 @@ and resume execution
 the debugger is once again disabled
   nix-repl> :c
   error: unknown command ':c'
+
+leaving the debugger from a toplevel error and entering it again doesn't leave old frames visible
+  nix-repl> with {}; a
+  error: undefined variable 'a'
+         at «string»:1:10:
+              1| with {}; a
+               |          ^
+
+  nix-repl> :s
+  error: undefined variable 'a'
+         at «string»:1:10:
+              1| with {}; a
+               |          ^
+
+  nix-repl> with {}; b
+  error: undefined variable 'b'
+         at «string»:1:10:
+              1| with {}; b
+               |          ^
+
+  nix-repl> :bt
+
+  0: error: undefined variable 'b'
+  «string»:1:10
+
+       1| with {}; b
+        |          ^
+
+  1: error: Fake frame for debugging purposes
+  «string»:1:10
+
+       1| with {}; b
+        |          ^
+
+exiting from here prints the error
+  nix-repl> :q
+  error: undefined variable 'b'
+         at «string»:1:10:
+              1| with {}; b
+               |          ^