From 2b0e95e7aabd075f95cbfb1607330b2284b01918 Mon Sep 17 00:00:00 2001 From: pennae Date: Mon, 11 Dec 2023 16:23:08 +0100 Subject: [PATCH] use singleton expr to generate black hole errors this also reduces forceValue code size and removes the need for hideInDiagnostics. coopting thunk forcing like this has the additional benefit of clarifying how these errors can happen in the first place. --- src/libexpr/eval-inline.hh | 14 +++----------- src/libexpr/eval.cc | 35 +++++++++++++++++++++++++++-------- src/libexpr/eval.hh | 10 ++-------- src/libexpr/nixexpr.cc | 2 ++ src/libexpr/nixexpr.hh | 10 ++++++++++ src/libexpr/primops.cc | 23 ----------------------- src/libexpr/primops.hh | 6 ------ src/libexpr/value.hh | 12 +++++++----- 8 files changed, 51 insertions(+), 61 deletions(-) diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index d48871628..52aa75b5f 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -93,20 +93,12 @@ void EvalState::forceValue(Value & v, const PosIdx pos) expr->eval(*this, *env, v); } catch (...) { v.mkThunk(env, expr); + tryFixupBlackHolePos(v, pos); throw; } } - else if (v.isApp()) { - try { - callFunction(*v.app.left, *v.app.right, v, noPos); - } catch (InfiniteRecursionError & e) { - // only one black hole can *throw* in any given eval stack so we need not - // check whether the position is set already. - if (v.isBlackhole()) - e.err.errPos = positions[pos]; - throw; - } - } + else if (v.isApp()) + callFunction(*v.app.left, *v.app.right, v, pos); } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 4dc5af97a..0c35b3713 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -256,8 +256,8 @@ std::string showType(const Value & v) case tPrimOpApp: return fmt("the partially applied built-in function '%s'", std::string(getPrimOp(v)->primOp->name)); case tExternal: return v.external->showType(); - case tThunk: return "a thunk"; - case tApp: return v.isBlackhole() ? "a black hole" : "a function application"; + case tThunk: return v.isBlackhole() ? "a black hole" : "a thunk"; + case tApp: return "a function application"; default: return std::string(showType(v.type())); } @@ -1624,14 +1624,12 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & auto * fn = vCur.primOp; nrPrimOpCalls++; - // This will count black holes, but that's ok, because unrecoverable errors are rare. if (countCalls) primOpCalls[fn->name]++; try { fn->fun(*this, vCur.determinePos(noPos), args, vCur); } catch (Error & e) { - if (!fn->hideInDiagnostics) - addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); + addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); throw; } @@ -1670,7 +1668,6 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & auto fn = primOp->primOp; nrPrimOpCalls++; - // This will count black holes, but that's ok, because unrecoverable errors are rare. if (countCalls) primOpCalls[fn->name]++; try { @@ -1680,8 +1677,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & // so the debugger allows to inspect the wrong parameters passed to the builtin. fn->fun(*this, vCur.determinePos(noPos), vArgs, vCur); } catch (Error & e) { - if (!fn->hideInDiagnostics) - addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); + addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); throw; } @@ -2035,6 +2031,29 @@ void ExprPos::eval(EvalState & state, Env & env, Value & v) } +void ExprBlackHole::eval(EvalState & state, Env & env, Value & v) +{ + state.error("infinite recursion encountered") + .debugThrow(); +} + +// always force this to be separate, otherwise forceValue may inline it and take +// a massive perf hit +[[gnu::noinline]] +void EvalState::tryFixupBlackHolePos(Value & v, PosIdx pos) +{ + if (!v.isBlackhole()) + return; + auto e = std::current_exception(); + try { + std::rethrow_exception(e); + } catch (InfiniteRecursionError & e) { + e.err.errPos = positions[pos]; + } catch (...) { + } +} + + void EvalState::forceValueDeep(Value & v) { std::set seen; diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 4c7ea1d98..56bc5e48f 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -77,14 +77,6 @@ struct PrimOp */ std::optional experimentalFeature; - /** - * Whether to hide this primop in diagnostics. - * - * Used to hide the fact that black holes are primop applications from - * stack traces. - */ - bool hideInDiagnostics; - /** * Validity check to be performed by functions that introduce primops, * such as RegisterPrimOp() and Value::mkPrimOp(). @@ -473,6 +465,8 @@ public: */ inline void forceValue(Value & v, const PosIdx pos); + void tryFixupBlackHolePos(Value & v, PosIdx pos); + /** * Force a value, then recursively force list elements and * attributes. diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 22be8e68c..84860b30f 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -9,6 +9,8 @@ namespace nix { +ExprBlackHole eBlackHole; + struct PosAdapter : AbstractPos { Pos::Origin origin; diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index cf6fd1a8d..1e57fec7a 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -462,6 +462,16 @@ struct ExprPos : Expr COMMON_METHODS }; +/* only used to mark thunks as black holes. */ +struct ExprBlackHole : Expr +{ + void show(const SymbolTable & symbols, std::ostream & str) const override {} + void eval(EvalState & state, Env & env, Value & v) override; + void bindVars(EvalState & es, const std::shared_ptr & env) override {} +}; + +extern ExprBlackHole eBlackHole; + /* Static environments are used to map variable names onto (level, displacement) pairs used to obtain the value of the variable at diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index b7e903667..2a71747a0 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -4263,29 +4263,6 @@ static RegisterPrimOp primop_splitVersion({ }); -static void prim_blackHoleFn(EvalState & state, const PosIdx pos, Value * * args, Value & v) -{ - state.error("infinite recursion encountered") - .debugThrow(); -} - -static PrimOp primop_blackHole = { - .name = "«blackHole»", - .args = {}, - .fun = prim_blackHoleFn, - .hideInDiagnostics = true, -}; - -static Value makeBlackHole() -{ - Value v; - v.mkPrimOp(&primop_blackHole); - return v; -} - -Value prim_blackHole = makeBlackHole(); - - /************************************************************* * Primop registration *************************************************************/ diff --git a/src/libexpr/primops.hh b/src/libexpr/primops.hh index 244eada86..45486608f 100644 --- a/src/libexpr/primops.hh +++ b/src/libexpr/primops.hh @@ -51,10 +51,4 @@ void prim_importNative(EvalState & state, const PosIdx pos, Value * * args, Valu */ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v); -/** - * Placeholder value for black holes, used to represent black holes as - * applications of this value to the evaluated thunks. - */ -extern Value prim_blackHole; - } diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 52cd0f901..d9860e921 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -61,6 +61,7 @@ class Bindings; struct Env; struct Expr; struct ExprLambda; +struct ExprBlackHole; struct PrimOp; class Symbol; class PosIdx; @@ -442,16 +443,17 @@ public: }; -extern Value prim_blackHole; +extern ExprBlackHole eBlackHole; -inline bool Value::isBlackhole() const +bool Value::isBlackhole() const { - return internalType == tApp && app.left == &prim_blackHole; + return internalType == tThunk && thunk.expr == (Expr*) &eBlackHole; } -inline void Value::mkBlackhole() +void Value::mkBlackhole() { - mkApp(&prim_blackHole, &prim_blackHole); + internalType = tThunk; + thunk.expr = (Expr*) &eBlackHole; }