debugger backtrace tests, fixes, ordering

.gitignore

@@ -95,6 +95,7 @@ perl/Makefile.config
 /tests/functional/dyn-drv/config.nix
 /tests/functional/repl-result-out
 /tests/functional/test-libstoreconsumer/test-libstoreconsumer
+/tests/functional/debugger/*.out
 
 # /tests/functional/lang/
 /tests/functional/lang/*.out

src/libcmd/repl.cc

@@ -110,7 +110,7 @@ std::string removeWhitespace(std::string s)
 NixRepl::NixRepl(const SearchPath & searchPath, nix::ref<Store> store, ref<EvalState> state,
             std::function<NixRepl::AnnotatedValues()> getValues)
     : AbstractNixRepl(state)
-    , debugTraceIndex(0)
+    , debugTraceIndex(state->debugTraces.size() - 1)
     , getValues(getValues)
     , staticEnv(new StaticEnv(false, state->staticBaseEnv.get()))
     , historyFile(getDataDir() + "/nix/repl-history")
@@ -214,8 +214,8 @@ namespace {
 
 static std::ostream & showDebugTrace(std::ostream & out, const PosTable & positions, const DebugTrace & dt)
 {
-    if (dt.isError)
-        out << ANSI_RED "error: " << ANSI_NORMAL;
+    if (dt.verbosity.has_value())
+        out << showVerbosity(dt.verbosity.value()) << ": " << ANSI_NORMAL;
     out << dt.hint.str() << "\n";
 
     // prefer direct pos, but if noPos then try the expr.
@@ -529,18 +529,22 @@ bool NixRepl::processLine(std::string line)
     }
 
     else if (state->debugRepl && (command == ":bt" || command == ":backtrace")) {
-        for (const auto & [idx, i] : enumerate(state->debugTraces)) {
+        unsigned int idx = 0;
+        for (auto i = state->debugTraces.rbegin(); i != state->debugTraces.rend(); ++i) {
             std::cout << "\n" << ANSI_BLUE << idx << ANSI_NORMAL << ": ";
-            showDebugTrace(std::cout, state->positions, i);
+            showDebugTrace(std::cout, state->positions, *i);
+            ++idx;
         }
     }
 
     else if (state->debugRepl && (command == ":env")) {
-        for (const auto & [idx, i] : enumerate(state->debugTraces)) {
+        unsigned int idx = 0;
+        for (auto i = state->debugTraces.rbegin(); i != state->debugTraces.rend(); ++i) {
             if (idx == debugTraceIndex) {
-                printEnvBindings(*state, i.expr, i.env);
+                printEnvBindings(*state, i->expr, i->env);
                 break;
             }
+            ++idx;
         }
     }
 
@@ -550,15 +554,17 @@ bool NixRepl::processLine(std::string line)
             debugTraceIndex = stoi(arg);
         } catch (...) { }
 
-        for (const auto & [idx, i] : enumerate(state->debugTraces)) {
+        unsigned int idx = 0;
+        for (auto i = state->debugTraces.rbegin(); i != state->debugTraces.rend(); ++i) {
              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;
              }
+             ++idx;
         }
     }
 

src/libexpr/eval.cc

@@ -453,20 +453,6 @@ ErrorBuilder & ErrorBuilder::withSuggestions(Suggestions & s)
     return *this;
 }
 
-ErrorBuilder & ErrorBuilder::withFrame(const Env & env, const Expr & expr)
-{
-    // NOTE: This is abusing side-effects.
-    // TODO: check compatibility with nested debugger calls.
-    state.debugTraces.push_front(DebugTrace {
-        .pos = nullptr,
-        .expr = expr,
-        .env = env,
-        .hint = hintformat("Fake frame for debugging purposes"),
-        .isError = true
-    });
-    return *this;
-}
-
 
 EvalState::EvalState(
     const SearchPath & _searchPath,
@@ -794,17 +780,21 @@ void printWithBindings(const SymbolTable & st, const Env & env)
     }
 }
 
-void printEnvBindings(const SymbolTable & st, const StaticEnv & se, const Env & env, int lvl)
+int printEnvBindings_helper(const SymbolTable & st, const StaticEnv & se, const Env & env)
 {
-    std::cout << "Env level " << lvl << std::endl;
-
     if (se.up && env.up) {
+        // first print parent bindings
+        int lvl = printEnvBindings_helper(st, *se.up, *env.up);
+        // then bindings for this level.
+        ++lvl;
+        std::cout << "Env level " << lvl << std::endl;
         std::cout << "static: ";
         printStaticEnvBindings(st, se);
         printWithBindings(st, env);
         std::cout << std::endl;
-        printEnvBindings(st, *se.up, *env.up, ++lvl);
+        return lvl;
     } else {
+        std::cout << "Env level " << 0 << std::endl;
         std::cout << ANSI_MAGENTA;
         // for the top level, don't print the double underscore ones;
         // they are in builtins.
@@ -815,7 +805,7 @@ void printEnvBindings(const SymbolTable & st, const StaticEnv & se, const Env &
         std::cout << std::endl;
         printWithBindings(st, env);  // probably nothing there for the top level.
         std::cout << std::endl;
-
+        return 0;
     }
 }
 
@@ -824,7 +814,7 @@ void printEnvBindings(const EvalState &es, const Expr & expr, const Env & env)
     // just print the names for now
     auto se = es.getStaticEnv(expr);
     if (se)
-        printEnvBindings(es.symbols, *se, env, 0);
+        printEnvBindings_helper(es.symbols, *se, env);
 }
 
 void mapStaticEnvBindings(const SymbolTable & st, const StaticEnv & se, const Env & env, ValMap & vm)
@@ -859,10 +849,6 @@ std::unique_ptr<ValMap> mapStaticEnvBindings(const SymbolTable & st, const Stati
 
 void EvalState::runDebugRepl(const Error * error, const Env & env, const Expr & expr)
 {
-    // double check we've got the debugRepl function pointer.
-    if (!debugRepl)
-        return;
-
     auto dts =
         error && expr.getPos()
         ? std::make_unique<DebugTraceStacker>(
@@ -872,7 +858,7 @@ void EvalState::runDebugRepl(const Error * error, const Env & env, const Expr &
                 .expr = expr,
                 .env = env,
                 .hint = error->info().msg,
-                .isError = true
+                .verbosity = std::make_optional(error->info().level),
             })
         : nullptr;
 
@@ -917,7 +903,7 @@ static std::unique_ptr<DebugTraceStacker> makeDebugTraceStacker(
             .expr = expr,
             .env = env,
             .hint = hintfmt(s, s2),
-            .isError = false
+            .verbosity = std::nullopt
         });
 }
 
@@ -987,7 +973,7 @@ inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval)
             return j->value;
         }
         if (!env->prevWith)
-            error("undefined variable '%1%'", symbols[var.name]).atPos(var.pos).withFrame(*env, var).debugThrow<UndefinedVarError>();
+            error("undefined variable '%1%'", symbols[var.name]).atPos(var.pos).debugThrow<UndefinedVarError>();
         for (size_t l = env->prevWith; l; --l, env = env->up) ;
     }
 }
@@ -1223,7 +1209,7 @@ inline bool EvalState::evalBool(Env & env, Expr * e, const PosIdx pos, std::stri
         Value v;
         e->eval(*this, env, v);
         if (v.type() != nBool)
-            error("value is %1% while a Boolean was expected", showType(v)).withFrame(env, *e).debugThrow<TypeError>();
+            error("value is %1% while a Boolean was expected", showType(v)).debugThrow<TypeError>();
         return v.boolean;
     } catch (Error & e) {
         e.addTrace(positions[pos], errorCtx);
@@ -1237,7 +1223,7 @@ inline void EvalState::evalAttrs(Env & env, Expr * e, Value & v, const PosIdx po
     try {
         e->eval(*this, env, v);
         if (v.type() != nAttrs)
-            error("value is %1% while a set was expected", showType(v)).withFrame(env, *e).debugThrow<TypeError>();
+            error("value is %1% while a set was expected", showType(v)).debugThrow<TypeError>();
     } catch (Error & e) {
         e.addTrace(positions[pos], errorCtx);
         throw;
@@ -1346,7 +1332,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
         auto nameSym = state.symbols.create(nameVal.string_view());
         Bindings::iterator j = v.attrs->find(nameSym);
         if (j != v.attrs->end())
-            state.error("dynamic attribute '%1%' already defined at %2%", state.symbols[nameSym], state.positions[j->pos]).atPos(i.pos).withFrame(env, *this).debugThrow<EvalError>();
+            state.error("dynamic attribute '%1%' already defined at %2%", state.symbols[nameSym], state.positions[j->pos]).atPos(i.pos).debugThrow<EvalError>();
 
         i.valueExpr->setName(nameSym);
         /* Keep sorted order so find can catch duplicates */
@@ -1450,7 +1436,7 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
                         allAttrNames.insert(state.symbols[attr.name]);
                     auto suggestions = Suggestions::bestMatches(allAttrNames, state.symbols[name]);
                     state.error("attribute '%1%' missing", state.symbols[name])
-                        .atPos(pos).withSuggestions(suggestions).withFrame(env, *this).debugThrow<EvalError>();
+                        .atPos(pos).withSuggestions(suggestions).debugThrow<EvalError>();
                 }
             }
             vAttrs = j->value;
@@ -1568,7 +1554,6 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                                              symbols[i.name])
                                     .atPos(lambda.pos)
                                     .withTrace(pos, "from call site")
-                                    .withFrame(*fun.lambda.env, lambda)
                                     .debugThrow<TypeError>();
                         }
                         env2.values[displ++] = i.def->maybeThunk(*this, env2);
@@ -1595,7 +1580,6 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                                 .atPos(lambda.pos)
                                 .withTrace(pos, "from call site")
                                 .withSuggestions(suggestions)
-                                .withFrame(*fun.lambda.env, lambda)
                                 .debugThrow<TypeError>();
                         }
                     abort(); // can't happen
@@ -1803,7 +1787,7 @@ Nix attempted to evaluate a function as a top level expression; in
 this case it must have its arguments supplied either by default
 values, or passed explicitly with '--arg' or '--argstr'. See
 https://nixos.org/manual/nix/stable/language/constructs.html#functions.)", symbols[i.name])
-                    .atPos(i.pos).withFrame(*fun.lambda.env, *fun.lambda.fun).debugThrow<MissingArgumentError>();
+                    .atPos(i.pos).debugThrow<MissingArgumentError>();
             }
         }
     }
@@ -1836,7 +1820,7 @@ void ExprAssert::eval(EvalState & state, Env & env, Value & v)
     if (!state.evalBool(env, cond, pos, "in the condition of the assert statement")) {
         std::ostringstream out;
         cond->show(state.symbols, out);
-        state.error("assertion '%1%' failed", out.str()).atPos(pos).withFrame(env, *this).debugThrow<AssertionError>();
+        state.error("assertion '%1%' failed", out.str()).atPos(pos).debugThrow<AssertionError>();
     }
     body->eval(state, env, v);
 }
@@ -2014,14 +1998,14 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
                 nf = n;
                 nf += vTmp.fpoint;
             } else
-                state.error("cannot add %1% to an integer", showType(vTmp)).atPos(i_pos).withFrame(env, *this).debugThrow<EvalError>();
+                state.error("cannot add %1% to an integer", showType(vTmp)).atPos(i_pos).debugThrow<EvalError>();
         } else if (firstType == nFloat) {
             if (vTmp.type() == nInt) {
                 nf += vTmp.integer;
             } else if (vTmp.type() == nFloat) {
                 nf += vTmp.fpoint;
             } else
-                state.error("cannot add %1% to a float", showType(vTmp)).atPos(i_pos).withFrame(env, *this).debugThrow<EvalError>();
+                state.error("cannot add %1% to a float", showType(vTmp)).atPos(i_pos).debugThrow<EvalError>();
         } else {
             if (s.empty()) s.reserve(es->size());
             /* skip canonization of first path, which would only be not
@@ -2043,7 +2027,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
         v.mkFloat(nf);
     else if (firstType == nPath) {
         if (!context.empty())
-            state.error("a string that refers to a store path cannot be appended to a path").atPos(pos).withFrame(env, *this).debugThrow<EvalError>();
+            state.error("a string that refers to a store path cannot be appended to a path").atPos(pos).debugThrow<EvalError>();
         v.mkPath(state.rootPath(CanonPath(canonPath(str()))));
     } else
         v.mkStringMove(c_str(), context);

src/libexpr/eval.hh

@@ -151,11 +151,9 @@ struct DebugTrace {
     const Expr & expr;
     const Env & env;
     hintformat hint;
-    bool isError;
+    std::optional<Verbosity> verbosity;
 };
 
-void debugError(Error * e, Env & env, Expr & expr);
-
 class ErrorBuilder
 {
     private:
@@ -184,9 +182,6 @@ class ErrorBuilder
         [[nodiscard, gnu::noinline]]
         ErrorBuilder & withSuggestions(Suggestions & s);
 
-        [[nodiscard, gnu::noinline]]
-        ErrorBuilder & withFrame(const Env & e, const Expr & ex);
-
         template<class ErrorType>
         [[gnu::noinline, gnu::noreturn]]
         void debugThrow();
@@ -274,20 +269,21 @@ public:
     [[gnu::noinline, gnu::noreturn]]
     void debugThrowLastTrace(E && error)
     {
+        // 'nullptr' args mean use env,expr from last debugTrace
         debugThrow(error, nullptr, nullptr);
     }
 
     template<class E>
     [[gnu::noinline, gnu::noreturn]]
     void debugThrow(E && error, const Env * env, const Expr * expr)
     {
-        if (debugRepl && ((env && expr) || !debugTraces.empty())) {
-            if (!env || !expr) {
+        if (debugRepl) {
+            if (env && expr)
+                runDebugRepl(&error, *env, *expr);
+            else if (!debugTraces.empty()) {
                 const DebugTrace & last = debugTraces.front();
-                env = &last.env;
-                expr = &last.expr;
+                runDebugRepl(&error, last.env, last.expr);
             }
-            runDebugRepl(&error, *env, *expr);
         }
 
         throw std::move(error);

src/libutil/error.cc

@@ -185,45 +185,42 @@ static bool printPosMaybe(std::ostream & oss, std::string_view indent, const std
     return hasPos;
 }
 
-std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool showTrace)
+// return verbosity in a string of the appropriate color; doesn't revert to ANSI_NORMAL.
+std::string showVerbosity(Verbosity v)
 {
-    std::string prefix;
-    switch (einfo.level) {
+    switch (v) {
         case Verbosity::lvlError: {
-            prefix = ANSI_RED "error";
-            break;
+            return ANSI_RED "error";
         }
         case Verbosity::lvlNotice: {
-            prefix = ANSI_RED "note";
-            break;
+            return ANSI_RED "note";
         }
         case Verbosity::lvlWarn: {
-            prefix = ANSI_WARNING "warning";
-            break;
+            return ANSI_WARNING "warning";
         }
         case Verbosity::lvlInfo: {
-            prefix = ANSI_GREEN "info";
-            break;
+            return ANSI_GREEN "info";
         }
         case Verbosity::lvlTalkative: {
-            prefix = ANSI_GREEN "talk";
-            break;
+            return ANSI_GREEN "talk";
         }
         case Verbosity::lvlChatty: {
-            prefix = ANSI_GREEN "chat";
-            break;
+            return ANSI_GREEN "chat";
         }
         case Verbosity::lvlVomit: {
-            prefix = ANSI_GREEN "vomit";
-            break;
+            return ANSI_GREEN "vomit";
         }
         case Verbosity::lvlDebug: {
-            prefix = ANSI_WARNING "debug";
-            break;
+            return ANSI_WARNING "debug";
         }
         default:
             assert(false);
     }
+}
+
+std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool showTrace)
+{
+    std::string prefix = showVerbosity(einfo.level);
 
     // FIXME: show the program name as part of the trace?
     if (einfo.programName && einfo.programName != ErrorInfo::programName)

src/libutil/error.hh

@@ -114,6 +114,9 @@ struct ErrorInfo {
     static std::optional<std::string> programName;
 };
 
+
+std::string showVerbosity(Verbosity v);
+
 std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool showTrace);
 
 /**