Merge pull request #12655 from NixOS/mergify/bp/2.27-maintenance/pr-12645

Make debugger significantly faster (backport #12645)

maintainers/flake-module.nix

@@ -127,7 +127,6 @@
               ''^src/libexpr/nixexpr\.cc$''
               ''^src/libexpr/nixexpr\.hh$''
               ''^src/libexpr/parser-state\.hh$''
-              ''^src/libexpr/pos-table\.hh$''
               ''^src/libexpr/primops\.cc$''
               ''^src/libexpr/primops\.hh$''
               ''^src/libexpr/primops/context\.cc$''

src/libcmd/repl.cc

@@ -140,16 +140,13 @@ static std::ostream & showDebugTrace(std::ostream & out, const PosTable & positi
         out << ANSI_RED "error: " << ANSI_NORMAL;
     out << dt.hint.str() << "\n";
 
-    // prefer direct pos, but if noPos then try the expr.
+    auto pos = dt.getPos(positions);
-    auto pos = dt.pos
-        ? dt.pos
-        : positions[dt.expr.getPos() ? dt.expr.getPos() : noPos];
 
     if (pos) {
-        out << *pos;
+        out << pos;
-        if (auto loc = pos->getCodeLines()) {
+        if (auto loc = pos.getCodeLines()) {
             out << "\n";
-            printCodeLines(out, "", *pos, *loc);
+            printCodeLines(out, "", pos, *loc);
             out << "\n";
         }
     }

src/libexpr/eval-error.cc

@@ -45,7 +45,7 @@ EvalErrorBuilder<T> & EvalErrorBuilder<T>::withFrame(const Env & env, const Expr
     // TODO: check compatibility with nested debugger calls.
     // TODO: What side-effects??
     error.state.debugTraces.push_front(DebugTrace{
-        .pos = error.state.positions[expr.getPos()],
+        .pos = expr.getPos(),
         .expr = expr,
         .env = env,
         .hint = HintFmt("Fake frame for debugging purposes"),

src/libexpr/eval.cc

@@ -771,18 +771,26 @@ void EvalState::runDebugRepl(const Error * error, const Env & env, const Expr &
     if (!debugRepl || inDebugger)
         return;
 
-    auto dts =
+    auto dts = [&]() -> std::unique_ptr<DebugTraceStacker> {
-        error && expr.getPos()
+        if (error && expr.getPos()) {
-        ? std::make_unique<DebugTraceStacker>(
+            auto trace = DebugTrace{
-            *this,
+                .pos = [&]() -> std::variant<Pos, PosIdx> {
-            DebugTrace {
+                    if (error->info().pos) {
-                .pos = error->info().pos ? error->info().pos : positions[expr.getPos()],
+                        if (auto * pos = error->info().pos.get())
+                            return *pos;
+                        return noPos;
+                    }
+                    return expr.getPos();
+                }(),
                 .expr = expr,
                 .env = env,
                 .hint = error->info().msg,
-                .isError = true
+                .isError = true};
-            })
+
-        : nullptr;
+            return std::make_unique<DebugTraceStacker>(*this, std::move(trace));
+        }
+        return nullptr;
+    }();
 
     if (error)
     {
@@ -827,7 +835,7 @@ static std::unique_ptr<DebugTraceStacker> makeDebugTraceStacker(
     EvalState & state,
     Expr & expr,
     Env & env,
-    std::shared_ptr<Pos> && pos,
+    std::variant<Pos, PosIdx> pos,
     const Args & ... formatArgs)
 {
     return std::make_unique<DebugTraceStacker>(state,
@@ -1104,7 +1112,7 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
                 *this,
                 *e,
                 this->baseEnv,
-                e->getPos() ? std::make_shared<Pos>(positions[e->getPos()]) : nullptr,
+                e->getPos(),
                 "while evaluating the file '%1%':", resolvedPath.to_string())
             : nullptr;
 
@@ -1330,9 +1338,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
             state,
             *this,
             env2,
-            getPos()
+            getPos(),
-                ? std::make_shared<Pos>(state.positions[getPos()])
-                : nullptr,
             "while evaluating a '%1%' expression",
             "let"
         )
@@ -1401,7 +1407,7 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
                 state,
                 *this,
                 env,
-                state.positions[getPos()],
+                getPos(),
                 "while evaluating the attribute '%1%'",
                 showAttrPath(state, env, attrPath))
             : nullptr;
@@ -1602,7 +1608,7 @@ void EvalState::callFunction(Value & fun, std::span<Value *> args, Value & vRes,
             try {
                 auto dts = debugRepl
                     ? makeDebugTraceStacker(
-                        *this, *lambda.body, env2, positions[lambda.pos],
+                        *this, *lambda.body, env2, lambda.pos,
                         "while calling %s",
                         lambda.name
                         ? concatStrings("'", symbols[lambda.name], "'")
@@ -1737,9 +1743,7 @@ void ExprCall::eval(EvalState & state, Env & env, Value & v)
             state,
             *this,
             env,
-            getPos()
+            getPos(),
-                ? std::make_shared<Pos>(state.positions[getPos()])
-                : nullptr,
             "while calling a function"
         )
         : nullptr;
@@ -2123,7 +2127,7 @@ void EvalState::forceValueDeep(Value & v)
                 try {
                     // If the value is a thunk, we're evaling. Otherwise no trace necessary.
                     auto dts = debugRepl && i.value->isThunk()
-                        ? makeDebugTraceStacker(*this, *i.value->payload.thunk.expr, *i.value->payload.thunk.env, positions[i.pos],
+                        ? makeDebugTraceStacker(*this, *i.value->payload.thunk.expr, *i.value->payload.thunk.env, i.pos,
                             "while evaluating the attribute '%1%'", symbols[i.name])
                         : nullptr;
 

src/libexpr/eval.hh

@@ -171,11 +171,28 @@ struct RegexCache;
 std::shared_ptr<RegexCache> makeRegexCache();
 
 struct DebugTrace {
-    std::shared_ptr<Pos> pos;
+    /* WARNING: Converting PosIdx -> Pos should be done with extra care. This is
+       due to the fact that operator[] of PosTable is incredibly expensive. */
+    std::variant<Pos, PosIdx> pos;
     const Expr & expr;
     const Env & env;
     HintFmt hint;
     bool isError;
+
+    Pos getPos(const PosTable & table) const
+    {
+        return std::visit(
+            overloaded{
+                [&](PosIdx idx) {
+                    // Prefer direct pos, but if noPos then try the expr.
+                    if (!idx)
+                        idx = expr.getPos();
+                    return table[idx];
+                },
+                [&](Pos pos) { return pos; },
+            },
+            pos);
+    }
 };
 
 class EvalState : public std::enable_shared_from_this<EvalState>

src/libexpr/meson.build

@@ -172,8 +172,6 @@ headers = [config_h] + files(
   # internal: 'lexer-helpers.hh',
   'nixexpr.hh',
   'parser-state.hh',
-  'pos-idx.hh',
-  'pos-table.hh',
   'primops.hh',
   'print-ambiguous.hh',
   'print-options.hh',

src/libexpr/nixexpr.cc

@@ -601,41 +601,6 @@ void ExprLambda::setDocComment(DocComment docComment) {
     }
 };
 
-
-
-/* Position table. */
-
-Pos PosTable::operator[](PosIdx p) const
-{
-    auto origin = resolve(p);
-    if (!origin)
-        return {};
-
-    const auto offset = origin->offsetOf(p);
-
-    Pos result{0, 0, origin->origin};
-    auto lines = this->lines.lock();
-    auto linesForInput = (*lines)[origin->offset];
-
-    if (linesForInput.empty()) {
-        auto source = result.getSource().value_or("");
-        const char * begin = source.data();
-        for (Pos::LinesIterator it(source), end; it != end; it++)
-            linesForInput.push_back(it->data() - begin);
-        if (linesForInput.empty())
-            linesForInput.push_back(0);
-    }
-    // as above: the first line starts at byte 0 and is always present
-    auto lineStartOffset = std::prev(
-        std::upper_bound(linesForInput.begin(), linesForInput.end(), offset));
-
-    result.line = 1 + (lineStartOffset - linesForInput.begin());
-    result.column = 1 + (offset - *lineStartOffset);
-    return result;
-}
-
-
-
 /* Symbol table. */
 
 size_t SymbolTable::totalSize() const

src/libutil/error.hh

@@ -50,6 +50,14 @@ struct LinesOfCode {
     std::optional<std::string> nextLineOfCode;
 };
 
+/* NOTE: position.hh recursively depends on source-path.hh -> source-accessor.hh
+   -> hash.hh -> config.hh -> experimental-features.hh -> error.hh -> Pos.
+   There are other such cycles.
+   Thus, Pos has to be an incomplete type in this header. But since ErrorInfo/Trace
+   have to refer to Pos, they have to use pointer indirection via std::shared_ptr
+   to break the recursive header dependency.
+   FIXME: Untangle this mess. Should there be AbstractPos as there used to be before
+   4feb7d9f71? */
 struct Pos;
 
 void printCodeLines(std::ostream & out,

src/libutil/meson.build

@@ -155,6 +155,7 @@ sources = files(
   'memory-source-accessor.cc',
   'mounted-source-accessor.cc',
   'position.cc',
+  'pos-table.cc',
   'posix-source-accessor.cc',
   'references.cc',
   'serialise.cc',
@@ -225,6 +226,8 @@ headers = [config_h] + files(
   'muxable-pipe.hh',
   'os-string.hh',
   'pool.hh',
+  'pos-idx.hh',
+  'pos-table.hh',
   'position.hh',
   'posix-source-accessor.hh',
   'processes.hh',

src/libutil/pos-idx.hh

@@ -1,4 +1,5 @@
 #pragma once
+///@file
 
 #include <cinttypes>
 #include <functional>

src/libutil/pos-table.cc

@@ -0,0 +1,37 @@
+#include "pos-table.hh"
+
+#include <algorithm>
+
+namespace nix {
+
+/* Position table. */
+
+Pos PosTable::operator[](PosIdx p) const
+{
+    auto origin = resolve(p);
+    if (!origin)
+        return {};
+
+    const auto offset = origin->offsetOf(p);
+
+    Pos result{0, 0, origin->origin};
+    auto lines = this->lines.lock();
+    auto linesForInput = (*lines)[origin->offset];
+
+    if (linesForInput.empty()) {
+        auto source = result.getSource().value_or("");
+        const char * begin = source.data();
+        for (Pos::LinesIterator it(source), end; it != end; it++)
+            linesForInput.push_back(it->data() - begin);
+        if (linesForInput.empty())
+            linesForInput.push_back(0);
+    }
+    // as above: the first line starts at byte 0 and is always present
+    auto lineStartOffset = std::prev(std::upper_bound(linesForInput.begin(), linesForInput.end(), offset));
+
+    result.line = 1 + (lineStartOffset - linesForInput.begin());
+    result.column = 1 + (offset - *lineStartOffset);
+    return result;
+}
+
+}

src/libutil/pos-table.hh

@@ -1,4 +1,5 @@
 #pragma once
+///@file
 
 #include <cstdint>
 #include <vector>
@@ -18,9 +19,12 @@ public:
     private:
         uint32_t offset;
 
-        Origin(Pos::Origin origin, uint32_t offset, size_t size):
+        Origin(Pos::Origin origin, uint32_t offset, size_t size)
-            offset(offset), origin(origin), size(size)
+            : offset(offset)
-        {}
+            , origin(origin)
+            , size(size)
+        {
+        }
 
     public:
         const Pos::Origin origin;
@@ -72,6 +76,17 @@ public:
         return PosIdx(1 + origin.offset + offset);
     }
 
+    /**
+     * Convert a byte-offset PosIdx into a Pos with line/column information.
+     *
+     * @param p Byte offset into the virtual concatenation of all parsed contents
+     * @return Position
+     *
+     * @warning Very expensive to call, as this has to read the entire source
+     * into memory each time. Call this only if absolutely necessary. Prefer
+     * to keep PosIdx around instead of needlessly converting it into Pos by
+     * using this lookup method.
+     */
     Pos operator[](PosIdx p) const;
 
     Pos::Origin originOf(PosIdx p) const

src/libutil/position.cc

@@ -66,6 +66,13 @@ std::optional<std::string> Pos::getSource() const
     }, origin);
 }
 
+std::optional<SourcePath> Pos::getSourcePath() const
+{
+    if (auto * path = std::get_if<SourcePath>(&origin))
+        return *path;
+    return std::nullopt;
+}
+
 void Pos::print(std::ostream & out, bool showOrigin) const
 {
     if (showOrigin) {

src/libutil/position.hh

@@ -50,6 +50,7 @@ struct Pos
 
     explicit operator bool() const { return line > 0; }
 
+    /* TODO: Why std::shared_ptr<Pos> and not std::shared_ptr<const Pos>? */
     operator std::shared_ptr<Pos>() const;
 
     /**
@@ -69,9 +70,7 @@ struct Pos
     /**
      * Get the SourcePath, if the source was loaded from a file.
      */
-    std::optional<SourcePath> getSourcePath() const {
+    std::optional<SourcePath> getSourcePath() const;
-        return *std::get_if<SourcePath>(&origin);
-    }
 
     struct LinesIterator {
         using difference_type = size_t;