From e134f7c9748c2875b18c0c7af58d6fddd5240700 Mon Sep 17 00:00:00 2001 From: pennae Date: Mon, 29 Jan 2024 06:19:23 +0100 Subject: [PATCH] use byte indexed locations for PosIdx we now keep not a table of all positions, but a table of all origins and their sizes. position indices are now direct pointers into the virtual concatenation of all parsed contents. this slightly reduces memory usage and time spent in the parser, at the cost of not being able to report positions if the total input size exceeds 4GiB. this limit is not unique to nix though, rustc and clang also limit their input to 4GiB (although at least clang refuses to process inputs that are larger, we will not). this new 4GiB limit probably will not cause any problems for quite a while, all of nixpkgs together is less than 100MiB in size and already needs over 700MiB of memory and multiple seconds just to parse. 4GiB worth of input will easily take multiple minutes and over 30GiB of memory without even evaluating anything. if problems *do* arise we can probably recover the old table-based system by adding some tracking to Pos::Origin (or increasing the size of PosIdx outright), but for time being this looks like more complexity than it's worth. since we now need to read the entire input again to determine the line/column of a position we'll make unsafeGetAttrPos slightly lazy: mostly the set it returns is only used to determine the file of origin of an attribute, not its exact location. the thunks do not add measurable runtime overhead. notably this change is necessary to allow changing the parser since apparently nothing supports nix's very idiosyncratic line ending choice of "anything goes", making it very hard to calculate line/column positions in the parser (while byte offsets are very easy). (cherry picked from commit 5d9fdab3de0ee17c71369ad05806b9ea06dfceda) Change-Id: Ie0b2430cb120c09097afa8c0101884d94f4bbf34 --- src/libexpr/eval.cc | 7 +- src/libexpr/flake/flake.cc | 3 +- src/libexpr/lexer.l | 23 +---- src/libexpr/nixexpr.cc | 33 +++++++ src/libexpr/nixexpr.hh | 1 - src/libexpr/parser-state.hh | 9 +- src/libexpr/parser.y | 2 +- src/libexpr/pos-idx.hh | 1 + src/libexpr/pos-table.hh | 91 ++++++++++--------- src/libexpr/primops.cc | 48 ++++++++++ src/libexpr/primops.hh | 2 + .../lang/eval-okay-unsafeGetAttrPos.exp | 1 + .../eval-okay-unsafeGetAttrPos.imported-nix | 4 + .../lang/eval-okay-unsafeGetAttrPos.nix | 4 + tests/unit/libexpr/value/print.cc | 8 +- 15 files changed, 154 insertions(+), 83 deletions(-) create mode 100644 tests/functional/lang/eval-okay-unsafeGetAttrPos.exp create mode 100644 tests/functional/lang/eval-okay-unsafeGetAttrPos.imported-nix create mode 100644 tests/functional/lang/eval-okay-unsafeGetAttrPos.nix diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 32766950d..6db1eaffe 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -940,12 +940,11 @@ void EvalState::mkThunk_(Value & v, Expr * expr) void EvalState::mkPos(Value & v, PosIdx p) { - auto pos = positions[p]; - if (auto path = std::get_if(&pos.origin)) { + auto origin = positions.originOf(p); + if (auto path = std::get_if(&origin)) { auto attrs = buildBindings(3); attrs.alloc(sFile).mkString(path->path.abs()); - attrs.alloc(sLine).mkInt(pos.line); - attrs.alloc(sColumn).mkInt(pos.column); + makePositionThunks(*this, p, attrs.alloc(sLine), attrs.alloc(sColumn)); v.mkAttrs(attrs); } else v.mkNull(); diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 305d84a68..72e98f6bb 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -227,11 +227,10 @@ static Flake getFlake( .sourceInfo = std::make_shared(std::move(sourceInfo)) }; + // NOTE evalFile forces vInfo to be an attrset because mustBeTrivial is true. Value vInfo; state.evalFile(CanonPath(flakeFile), vInfo, true); // FIXME: symlink attack - expectType(state, nAttrs, vInfo, state.positions.add({CanonPath(flakeFile)}, 1, 1)); - if (auto description = vInfo.attrs->get(state.sDescription)) { expectType(state, nString, *description->value, description->pos); flake.description = description->value->string.s; diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index adb9d9757..1780f5cb9 100644 --- a/src/libexpr/lexer.l +++ b/src/libexpr/lexer.l @@ -39,33 +39,16 @@ namespace nix { static void initLoc(YYLTYPE * loc) { - loc->first_line = loc->last_line = 1; - loc->first_column = loc->last_column = 1; + loc->first_line = loc->last_line = 0; + loc->first_column = loc->last_column = 0; } static void adjustLoc(YYLTYPE * loc, const char * s, size_t len) { loc->stash(); - loc->first_line = loc->last_line; loc->first_column = loc->last_column; - - for (size_t i = 0; i < len; i++) { - switch (*s++) { - case '\r': - if (*s == '\n') { /* cr/lf */ - i++; - s++; - } - /* fall through */ - case '\n': - ++loc->last_line; - loc->last_column = 1; - break; - default: - ++loc->last_column; - } - } + loc->last_column += len; } diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index c591916e3..a9989b59e 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -581,6 +581,39 @@ std::string ExprLambda::showNamePos(const EvalState & state) const +/* 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 diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index f0265a896..e5755bb99 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -7,7 +7,6 @@ #include "value.hh" #include "symbol-table.hh" #include "error.hh" -#include "chunked-vector.hh" #include "position.hh" #include "eval-error.hh" #include "pos-idx.hh" diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index 4a8b59f08..3305e05b6 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -24,20 +24,15 @@ struct ParserLocation int last_line, last_column; // backup to recover from yyless(0) - int stashed_first_line, stashed_first_column; - int stashed_last_line, stashed_last_column; + int stashed_first_column, stashed_last_column; void stash() { - stashed_first_line = first_line; stashed_first_column = first_column; - stashed_last_line = last_line; stashed_last_column = last_column; } void unstash() { - first_line = stashed_first_line; first_column = stashed_first_column; - last_line = stashed_last_line; last_column = stashed_last_column; } }; @@ -275,7 +270,7 @@ inline Expr * ParserState::stripIndentation(const PosIdx pos, inline PosIdx ParserState::at(const ParserLocation & loc) { - return positions.add(origin, loc.first_line, loc.first_column); + return positions.add(origin, loc.first_column); } } diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 727ee94b4..5424d8741 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -435,7 +435,7 @@ Expr * parseExprFromBuf( .symbols = symbols, .positions = positions, .basePath = basePath, - .origin = {origin}, + .origin = positions.addOrigin(origin, length), .s = astSymbols, }; diff --git a/src/libexpr/pos-idx.hh b/src/libexpr/pos-idx.hh index 9949f1dc5..e94fd85c6 100644 --- a/src/libexpr/pos-idx.hh +++ b/src/libexpr/pos-idx.hh @@ -6,6 +6,7 @@ namespace nix { class PosIdx { + friend struct LazyPosAcessors; friend class PosTable; private: diff --git a/src/libexpr/pos-table.hh b/src/libexpr/pos-table.hh index 1decf3c85..0b60c4f6d 100644 --- a/src/libexpr/pos-table.hh +++ b/src/libexpr/pos-table.hh @@ -7,6 +7,7 @@ #include "chunked-vector.hh" #include "pos-idx.hh" #include "position.hh" +#include "sync.hh" namespace nix { @@ -17,66 +18,68 @@ public: { friend PosTable; private: - // must always be invalid by default, add() replaces this with the actual value. - // subsequent add() calls use this index as a token to quickly check whether the - // current origins.back() can be reused or not. - mutable uint32_t idx = std::numeric_limits::max(); + uint32_t offset; - // Used for searching in PosTable::[]. - explicit Origin(uint32_t idx) - : idx(idx) - , origin{std::monostate()} - { - } + Origin(Pos::Origin origin, uint32_t offset, size_t size): + offset(offset), origin(origin), size(size) + {} public: const Pos::Origin origin; + const size_t size; - Origin(Pos::Origin origin) - : origin(origin) + uint32_t offsetOf(PosIdx p) const { + return p.id - 1 - offset; } }; - struct Offset - { - uint32_t line, column; - }; - private: - std::vector origins; - ChunkedVector offsets; + using Lines = std::vector; -public: - PosTable() - : offsets(1024) - { - origins.reserve(1024); - } + std::map origins; + mutable Sync> lines; - PosIdx add(const Origin & origin, uint32_t line, uint32_t column) + const Origin * resolve(PosIdx p) const { - const auto idx = offsets.add({line, column}).second; - if (origins.empty() || origins.back().idx != origin.idx) { - origin.idx = idx; - origins.push_back(origin); - } - return PosIdx(idx + 1); - } + if (p.id == 0) + return nullptr; - Pos operator[](PosIdx p) const - { - if (p.id == 0 || p.id > offsets.size()) - return {}; const auto idx = p.id - 1; /* we want the last key <= idx, so we'll take prev(first key > idx). - this is guaranteed to never rewind origin.begin because the first - key is always 0. */ - const auto pastOrigin = std::upper_bound( - origins.begin(), origins.end(), Origin(idx), [](const auto & a, const auto & b) { return a.idx < b.idx; }); - const auto origin = *std::prev(pastOrigin); - const auto offset = offsets[idx]; - return {offset.line, offset.column, origin.origin}; + this is guaranteed to never rewind origin.begin because the first + key is always 0. */ + const auto pastOrigin = origins.upper_bound(idx); + return &std::prev(pastOrigin)->second; + } + +public: + Origin addOrigin(Pos::Origin origin, size_t size) + { + uint32_t offset = 0; + if (auto it = origins.rbegin(); it != origins.rend()) + offset = it->first + it->second.size; + // +1 because all PosIdx are offset by 1 to begin with (because noPos == 0), and + // another +1 to ensure that all origins can point to EOF, eg on (invalid) empty inputs. + if (2 + offset + size < offset) + return Origin{origin, offset, 0}; + return origins.emplace(offset, Origin{origin, offset, size}).first->second; + } + + PosIdx add(const Origin & origin, size_t offset) + { + if (offset > origin.size) + return PosIdx(); + return PosIdx(1 + origin.offset + offset); + } + + Pos operator[](PosIdx p) const; + + Pos::Origin originOf(PosIdx p) const + { + if (auto o = resolve(p)) + return o->origin; + return std::monostate{}; } }; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index a5b539202..77601e457 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -2492,6 +2492,54 @@ static RegisterPrimOp primop_unsafeGetAttrPos(PrimOp { .fun = prim_unsafeGetAttrPos, }); +// access to exact position information (ie, line and colum numbers) is deferred +// due to the cost associated with calculating that information and how rarely +// it is used in practice. this is achieved by creating thunks to otherwise +// inaccessible primops that are not exposed as __op or under builtins to turn +// the internal PosIdx back into a line and column number, respectively. exposing +// these primops in any way would at best be not useful and at worst create wildly +// indeterministic eval results depending on parse order of files. +// +// in a simpler world this would instead be implemented as another kind of thunk, +// but each type of thunk has an associated runtime cost in the current evaluator. +// as with black holes this cost is too high to justify another thunk type to check +// for in the very hot path that is forceValue. +static struct LazyPosAcessors { + PrimOp primop_lineOfPos{ + .arity = 1, + .fun = [] (EvalState & state, PosIdx pos, Value * * args, Value & v) { + v.mkInt(state.positions[PosIdx(args[0]->integer)].line); + } + }; + PrimOp primop_columnOfPos{ + .arity = 1, + .fun = [] (EvalState & state, PosIdx pos, Value * * args, Value & v) { + v.mkInt(state.positions[PosIdx(args[0]->integer)].column); + } + }; + + Value lineOfPos, columnOfPos; + + LazyPosAcessors() + { + lineOfPos.mkPrimOp(&primop_lineOfPos); + columnOfPos.mkPrimOp(&primop_columnOfPos); + } + + void operator()(EvalState & state, const PosIdx pos, Value & line, Value & column) + { + Value * posV = state.allocValue(); + posV->mkInt(pos.id); + line.mkApp(&lineOfPos, posV); + column.mkApp(&columnOfPos, posV); + } +} makeLazyPosAccessors; + +void makePositionThunks(EvalState & state, const PosIdx pos, Value & line, Value & column) +{ + makeLazyPosAccessors(state, pos, line, column); +} + /* Dynamic version of the `?' operator. */ static void prim_hasAttr(EvalState & state, const PosIdx pos, Value * * args, Value & v) { diff --git a/src/libexpr/primops.hh b/src/libexpr/primops.hh index 45486608f..9f76975db 100644 --- a/src/libexpr/primops.hh +++ b/src/libexpr/primops.hh @@ -51,4 +51,6 @@ void prim_importNative(EvalState & state, const PosIdx pos, Value * * args, Valu */ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v); +void makePositionThunks(EvalState & state, const PosIdx pos, Value & line, Value & column); + } diff --git a/tests/functional/lang/eval-okay-unsafeGetAttrPos.exp b/tests/functional/lang/eval-okay-unsafeGetAttrPos.exp new file mode 100644 index 000000000..c48e03014 --- /dev/null +++ b/tests/functional/lang/eval-okay-unsafeGetAttrPos.exp @@ -0,0 +1 @@ +{ column = 4; file = "/pwd/lang/eval-okay-unsafeGetAttrPos.imported-nix"; line = 6; } diff --git a/tests/functional/lang/eval-okay-unsafeGetAttrPos.imported-nix b/tests/functional/lang/eval-okay-unsafeGetAttrPos.imported-nix new file mode 100644 index 000000000..bdc364e76 --- /dev/null +++ b/tests/functional/lang/eval-okay-unsafeGetAttrPos.imported-nix @@ -0,0 +1,4 @@ +( + + + { y = "x"; }) diff --git a/tests/functional/lang/eval-okay-unsafeGetAttrPos.nix b/tests/functional/lang/eval-okay-unsafeGetAttrPos.nix new file mode 100644 index 000000000..6d2cb86a6 --- /dev/null +++ b/tests/functional/lang/eval-okay-unsafeGetAttrPos.nix @@ -0,0 +1,4 @@ +let + pos = builtins.unsafeGetAttrPos "y" (import ./eval-okay-unsafeGetAttrPos.imported-nix); +in +pos diff --git a/tests/unit/libexpr/value/print.cc b/tests/unit/libexpr/value/print.cc index aabf156c2..d2d699a64 100644 --- a/tests/unit/libexpr/value/print.cc +++ b/tests/unit/libexpr/value/print.cc @@ -110,8 +110,8 @@ TEST_F(ValuePrintingTests, vLambda) .up = nullptr, .values = { } }; - PosTable::Origin origin((std::monostate())); - auto posIdx = state.positions.add(origin, 1, 1); + PosTable::Origin origin = state.positions.addOrigin(std::monostate(), 1); + auto posIdx = state.positions.add(origin, 0); auto body = ExprInt(0); auto formals = Formals {}; @@ -558,8 +558,8 @@ TEST_F(ValuePrintingTests, ansiColorsLambda) .up = nullptr, .values = { } }; - PosTable::Origin origin((std::monostate())); - auto posIdx = state.positions.add(origin, 1, 1); + PosTable::Origin origin = state.positions.addOrigin(std::monostate(), 1); + auto posIdx = state.positions.add(origin, 0); auto body = ExprInt(0); auto formals = Formals {};