From 3593f5555efbbbf3ff1a2a62f4da3f026e6a35f4 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Wed, 27 Nov 2024 02:09:08 +0100 Subject: [PATCH] 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 | 23 ++++--- lix/libexpr/eval-error.cc | 18 +++-- lix/libexpr/eval-error.hh | 4 ++ lix/libexpr/eval.cc | 66 +++++++++++-------- lix/libexpr/eval.hh | 46 +++++++++---- lix/libexpr/primops.cc | 13 ++-- .../data/debug_frames.test | 40 +++++++++++ 7 files changed, 145 insertions(+), 65 deletions(-) diff --git a/lix/libcmd/repl.cc b/lix/libcmd/repl.cc index 599086118..541811ef1 100644 --- a/lix/libcmd/repl.cc +++ b/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); - loadDebugTraceEnv(i); + printEnvBindings(state, i->expr, i->env); + loadDebugTraceEnv(*i); break; } } diff --git a/lix/libexpr/eval-error.cc b/lix/libexpr/eval-error.cc index d32c5734d..23b318509 100644 --- a/lix/libexpr/eval-error.cc +++ b/lix/libexpr/eval-error.cc @@ -42,17 +42,14 @@ EvalErrorBuilder & EvalErrorBuilder::withSuggestions(Suggestions & s) template EvalErrorBuilder & EvalErrorBuilder::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,11 +74,12 @@ EvalErrorBuilder::addTrace(PosIdx pos, std::string_view formatString, const A template void EvalErrorBuilder::debugThrow() { - if (error.state.debug && !error.state.debug->traces.empty()) { - const DebugTrace & last = error.state.debug->traces.front(); - const Env * env = &last.env; - const Expr * expr = &last.expr; - error.state.debug->runDebugRepl(error.state, &error, *env, *expr); + if (error.state.debug) { + if (auto last = error.state.debug->traces().next()) { + const Env * env = &(*last)->env; + const Expr * expr = &(*last)->expr; + error.state.debug->runDebugRepl(error.state, &error, *env, *expr); + } } // `EvalState` is the only class that can construct an `EvalErrorBuilder`, diff --git a/lix/libexpr/eval-error.hh b/lix/libexpr/eval-error.hh index db2643b47..2c18ebc06 100644 --- a/lix/libexpr/eval-error.hh +++ b/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 friend class EvalErrorBuilder; + + std::shared_ptr frame; + public: EvalState & state; diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index 81b907aea..c14375532 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -646,15 +646,13 @@ void DebugState::runDebugRepl( auto dts = error && expr.getPos() - ? std::make_unique( - evalState, - DebugTrace { - .pos = error->info().pos ? error->info().pos : evalState.positions[expr.getPos()], - .expr = expr, - .env = env, - .hint = error->info().msg, - .isError = true - }) + ? addTrace(DebugTrace { + .pos = error->info().pos ? error->info().pos : evalState.positions[expr.getPos()], + .expr = expr, + .env = env, + .hint = error->info().msg, + .isError = true + }) : nullptr; if (error) @@ -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 trace( + new auto(std::move(t)), UnlinkDebugTrace{.state = this} + ); + std::shared_ptr entry(std::move(trace)); + latestTrace = entry; + return TraceFrame{entry}; +} + template -static std::unique_ptr makeDebugTraceStacker( +static DebugState::TraceFrame makeDebugTraceStacker( EvalState & state, Expr & expr, Env & env, std::shared_ptr && pos, const Args & ... formatArgs) { - return std::make_unique(state, - DebugTrace { - .pos = std::move(pos), - .expr = expr, - .env = env, - .hint = HintFmt(formatArgs...), - .isError = false - }); -} - -DebugTraceStacker::DebugTraceStacker(EvalState & evalState, DebugTrace t) - : 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); + auto trace = state.debug->addTrace(DebugTrace{ + .pos = std::move(pos), + .expr = expr, + .env = env, + .hint = HintFmt(formatArgs...), + .isError = false, + }); + if (state.debug->stop && state.debug->repl) + state.debug->runDebugRepl(state, nullptr, env, expr); + return trace; } inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval) diff --git a/lix/libexpr/eval.hh b/lix/libexpr/eval.hh index 50075961d..166bd3749 100644 --- a/lix/libexpr/eval.hh +++ b/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 parent; }; struct DebugState { +private: + std::weak_ptr latestTrace; + +public: std::function repl; bool stop = false; bool inDebugger = false; - std::list traces; std::map> exprEnvs; int trylevel = 0; @@ -178,8 +183,35 @@ struct DebugState } return nullptr; } -}; + class TraceFrame + { + friend struct DebugState; + template + 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 entry = nullptr; + + explicit TraceFrame(std::shared_ptr 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 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`. * diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index c59903469..d722eb59a 100644 --- a/lix/libexpr/primops.cc +++ b/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, dt.env, dt.expr); + state.debug->runDebugRepl(state, &error, (*trace)->env, (*trace)->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()) { - const DebugTrace & last = state.debug->traces.front(); - state.debug->runDebugRepl(state, nullptr, last.env, last.expr); + if (auto last = evalSettings.builtinsTraceDebugger && state.debug + ? state.debug->traces().next() + : std::nullopt) + { + state.debug->runDebugRepl(state, nullptr, (*last)->env, (*last)->expr); } state.forceValue(*args[1], pos); v = *args[1]; diff --git a/tests/functional/repl_characterization/data/debug_frames.test b/tests/functional/repl_characterization/data/debug_frames.test index 25e28ef97..a556b2420 100644 --- a/tests/functional/repl_characterization/data/debug_frames.test +++ b/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 + | ^