wroclaw-git-backups/nix
1
0
Fork 0
mirror of https://github.com/NixOS/nix synced 2025-06-25 10:41:16 +02:00
Code Activity

Merge pull request #12653 from NixOS/mergify/bp/2.24-maintenance/pr-12645

Browse source 
Make debugger significantly faster (backport #12645)
This commit is contained in:
mergify[bot] 2025-03-14 14:11:27 +00:00 • committed by GitHub
commit 56e113d67a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 123 additions and 73 deletions
Download patch file Download diff file Expand all files Collapse all files

1
maintainers/flake-module.nix
View file

@ -99,7 +99,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$''

11
src/libcmd/repl.cc
View file

@ -157,16 +157,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.pos
? dt.pos
: positions[dt.expr.getPos() ? dt.expr.getPos() : noPos];
auto pos = dt.getPos(positions);
if (pos) {
out << *pos;
if (auto loc = pos->getCodeLines()) {
out << pos;
if (auto loc = pos.getCodeLines()) {
out << "\n";
printCodeLines(out, "", *pos, *loc);
printCodeLines(out, "", pos, *loc);
out << "\n";
}
}

2
src/libexpr/eval-error.cc
View file

@ -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"),

44
src/libexpr/eval.cc
View file

@ -756,18 +756,26 @@ void EvalState::runDebugRepl(const Error * error, const Env & env, const Expr &
if (!debugRepl || inDebugger)
return;
auto dts =
error && expr.getPos()
? std::make_unique<DebugTraceStacker>(
*this,
DebugTrace {
.pos = error->info().pos ? error->info().pos : positions[expr.getPos()],
auto dts = [&]() -> std::unique_ptr<DebugTraceStacker> {
if (error && expr.getPos()) {
auto trace = DebugTrace{
.pos = [&]() -> std::variant<Pos, PosIdx> {
if (error->info().pos) {
if (auto * pos = error->info().pos.get())
return *pos;
return noPos;
}
return expr.getPos();
}(),
.expr = expr,
.env = env,
.hint = error->info().msg,
.isError = true
})
: nullptr;
.isError = true};
return std::make_unique<DebugTraceStacker>(*this, std::move(trace));
}
return nullptr;
}();
if (error)
{
@ -812,7 +820,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,
@ -1088,7 +1096,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;
@ -1314,9 +1322,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
state,
*this,
env2,
getPos()
? std::make_shared<Pos>(state.positions[getPos()])
: nullptr,
getPos(),
"while evaluating a '%1%' expression",
"let"
)
@ -1385,7 +1391,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;
@ -1603,7 +1609,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
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], "'")
@ -1742,9 +1748,7 @@ void ExprCall::eval(EvalState & state, Env & env, Value & v)
state,
*this,
env,
getPos()
? std::make_shared<Pos>(state.positions[getPos()])
: nullptr,
getPos(),
"while calling a function"
)
: nullptr;
@ -2122,7 +2126,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;

19
src/libexpr/eval.hh
View file

@ -155,11 +155,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>

2
src/libexpr/meson.build
View file

@ -175,8 +175,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',

35
src/libexpr/nixexpr.cc
View file

@ -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

8
src/libutil/error.hh
View file

@ -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,

3
src/libutil/meson.build
View file

@ -146,6 +146,7 @@ sources = files(
'logging.cc',
'memory-source-accessor.cc',
'position.cc',
'pos-table.cc',
'posix-source-accessor.cc',
'references.cc',
'serialise.cc',
@ -207,6 +208,8 @@ headers = [config_h] + files(
'memory-source-accessor.hh',
'muxable-pipe.hh',
'pool.hh',
'pos-idx.hh',
'pos-table.hh',
'position.hh',
'posix-source-accessor.hh',
'processes.hh',

1
src/libexpr/pos-idx.hh → src/libutil/pos-idx.hh
View file

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

37
src/libutil/pos-table.cc Normal file
View file

@ -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;
}
}

21
src/libexpr/pos-table.hh → src/libutil/pos-table.hh
View file

@ -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):
offset(offset), origin(origin), size(size)
{}
Origin(Pos::Origin origin, uint32_t offset, size_t 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

7
src/libutil/position.cc
View file

@ -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) {

5
src/libutil/position.hh
View file

@ -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 {
return *std::get_if<SourcePath>(&origin);
}
std::optional<SourcePath> getSourcePath() const;
struct LinesIterator {
using difference_type = size_t;