From a942a34469b22491d381a517e86a17f3999a140a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 23 May 2024 18:07:33 +0200 Subject: [PATCH 1/7] C API: Fix nix_c_primop_wrapper for strict initializers https://github.com/NixOS/nix/pull/10555 added a check requiring that output parameters always have an uninitialized Value as argument. Unfortunately the output parameter of the primop callback received a thunk instead. See the comment for implementation considerations. --- src/libexpr-c/nix_api_value.cc | 24 +++++++++++++++++--- tests/unit/libexpr/nix_api_expr.cc | 35 ++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 0366e5020..37e0012d1 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -73,10 +73,28 @@ static void nix_c_primop_wrapper( PrimOpFun f, void * userdata, nix::EvalState & state, const nix::PosIdx pos, nix::Value ** args, nix::Value & v) { nix_c_context ctx; - f(userdata, &ctx, (EvalState *) &state, (Value **) args, (Value *) &v); - /* TODO: In the future, this should throw different errors depending on the error code */ - if (ctx.last_err_code != NIX_OK) + + // v currently has a thunk, but the C API initializers require an uninitialized value. + // + // We can't destroy the thunk, because that makes it impossible to retry, + // which is needed for tryEval and for evaluation drivers that evaluate more + // than one value (e.g. an attrset with two derivations, both of which + // reference v). + // + // Instead we create a temporary value, and then assign the result to v. + // This does not give the primop definition access to the thunk, but that's + // ok because we don't see a need for this yet (e.g. inspecting thunks, + // or maybe something to make blackholes work better; we don't know). + nix::Value vTmp; + + f(userdata, &ctx, (EvalState *) &state, (Value **) args, (Value *) &vTmp); + + if (ctx.last_err_code != NIX_OK) { + /* TODO: Throw different errors depending on the error code */ state.error("Error from builtin function: %s", *ctx.last_err).atPos(pos).debugThrow(); + } + + v = vTmp; } PrimOp * nix_alloc_primop( diff --git a/tests/unit/libexpr/nix_api_expr.cc b/tests/unit/libexpr/nix_api_expr.cc index 0818f1cab..babc8c6a4 100644 --- a/tests/unit/libexpr/nix_api_expr.cc +++ b/tests/unit/libexpr/nix_api_expr.cc @@ -191,4 +191,39 @@ TEST_F(nix_api_expr_test, nix_expr_realise_context) nix_realised_string_free(r); } +const char * SAMPLE_USER_DATA = "whatever"; + +static void primop_square(void * user_data, nix_c_context * context, EvalState * state, Value ** args, Value * ret) +{ + assert(context); + assert(state); + assert(user_data == SAMPLE_USER_DATA); + auto i = nix_get_int(context, args[0]); + nix_init_int(context, ret, i * i); +} + +TEST_F(nix_api_expr_test, nix_expr_primop) +{ + PrimOp * primop = + nix_alloc_primop(ctx, primop_square, 1, "square", nullptr, "square an integer", (void *) SAMPLE_USER_DATA); + assert_ctx_ok(); + Value * primopValue = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_primop(ctx, primopValue, primop); + assert_ctx_ok(); + + Value * three = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_int(ctx, three, 3); + assert_ctx_ok(); + + Value * result = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_value_call(ctx, state, primopValue, three, result); + assert_ctx_ok(); + + auto r = nix_get_int(ctx, result); + ASSERT_EQ(9, r); +} + } // namespace nixC From 88842270454281c3541296bea87b46d553b42c3a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 23 May 2024 18:31:20 +0200 Subject: [PATCH 2/7] C API: Require initialized value from primop definition --- src/libexpr-c/nix_api_value.cc | 6 ++++++ tests/unit/libexpr/nix_api_expr.cc | 31 ++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 37e0012d1..043c6a568 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -94,6 +94,12 @@ static void nix_c_primop_wrapper( state.error("Error from builtin function: %s", *ctx.last_err).atPos(pos).debugThrow(); } + if (!vTmp.isValid()) { + state.error("Implementation error in custom function: return value was not initialized") + .atPos(pos) + .debugThrow(); + } + v = vTmp; } diff --git a/tests/unit/libexpr/nix_api_expr.cc b/tests/unit/libexpr/nix_api_expr.cc index babc8c6a4..9d629b463 100644 --- a/tests/unit/libexpr/nix_api_expr.cc +++ b/tests/unit/libexpr/nix_api_expr.cc @@ -226,4 +226,35 @@ TEST_F(nix_api_expr_test, nix_expr_primop) ASSERT_EQ(9, r); } +static void +primop_bad_no_return(void * user_data, nix_c_context * context, EvalState * state, Value ** args, Value * ret) +{ +} + +TEST_F(nix_api_expr_test, nix_expr_primop_bad_no_return) +{ + PrimOp * primop = + nix_alloc_primop(ctx, primop_bad_no_return, 1, "badNoReturn", nullptr, "a broken primop", nullptr); + assert_ctx_ok(); + Value * primopValue = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_primop(ctx, primopValue, primop); + assert_ctx_ok(); + + Value * three = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_int(ctx, three, 3); + assert_ctx_ok(); + + Value * result = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_value_call(ctx, state, primopValue, three, result); + ASSERT_EQ(ctx->last_err_code, NIX_ERR_NIX_ERROR); + ASSERT_THAT( + ctx->last_err, + testing::Optional( + testing::HasSubstr("Implementation error in custom function: return value was not initialized"))); + ASSERT_THAT(ctx->last_err, testing::Optional(testing::HasSubstr("badNoReturn"))); +} + } // namespace nixC From 8ef6efc1844a9782dbe265271020f6ffee571af0 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 23 May 2024 18:36:40 +0200 Subject: [PATCH 3/7] C API: Require non-thunk value from primop definition --- src/libexpr-c/nix_api_value.cc | 9 +++++++ tests/unit/libexpr/nix_api_expr.cc | 41 ++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 043c6a568..fa24b4494 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -100,6 +100,15 @@ static void nix_c_primop_wrapper( .debugThrow(); } + if (vTmp.type() == nix::nThunk) { + // We might allow this in the future if it makes sense for the evaluator + // e.g. implementing tail recursion by returning a thunk to the next + // "iteration". Until then, this is most likely a mistake or misunderstanding. + state.error("Implementation error in custom function: return value must not be a thunk") + .atPos(pos) + .debugThrow(); + } + v = vTmp; } diff --git a/tests/unit/libexpr/nix_api_expr.cc b/tests/unit/libexpr/nix_api_expr.cc index 9d629b463..e78f2bf61 100644 --- a/tests/unit/libexpr/nix_api_expr.cc +++ b/tests/unit/libexpr/nix_api_expr.cc @@ -257,4 +257,45 @@ TEST_F(nix_api_expr_test, nix_expr_primop_bad_no_return) ASSERT_THAT(ctx->last_err, testing::Optional(testing::HasSubstr("badNoReturn"))); } +static void +primop_bad_return_thunk(void * user_data, nix_c_context * context, EvalState * state, Value ** args, Value * ret) +{ + nix_init_apply(context, ret, args[0], args[1]); +} +TEST_F(nix_api_expr_test, nix_expr_primop_bad_return_thunk) +{ + PrimOp * primop = + nix_alloc_primop(ctx, primop_bad_return_thunk, 2, "badReturnThunk", nullptr, "a broken primop", nullptr); + assert_ctx_ok(); + Value * primopValue = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_primop(ctx, primopValue, primop); + assert_ctx_ok(); + + Value * toString = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_expr_eval_from_string(ctx, state, "builtins.toString", ".", toString); + assert_ctx_ok(); + + Value * four = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_int(ctx, four, 4); + assert_ctx_ok(); + + Value * partial = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_value_call(ctx, state, primopValue, toString, partial); + assert_ctx_ok(); + + Value * result = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_value_call(ctx, state, partial, four, result); + + ASSERT_EQ(ctx->last_err_code, NIX_ERR_NIX_ERROR); + ASSERT_THAT( + ctx->last_err, + testing::Optional( + testing::HasSubstr("Implementation error in custom function: return value must not be a thunk"))); + ASSERT_THAT(ctx->last_err, testing::Optional(testing::HasSubstr("badReturnThunk"))); +} } // namespace nixC From 4bc4fb40eac1f571ef94694a96a3bf4bf26ad22d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 23 May 2024 18:53:33 +0200 Subject: [PATCH 4/7] C API: builtin -> custom function Not all primops will be in `builtins`. --- src/libexpr-c/nix_api_value.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index fa24b4494..978cf7f43 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -91,7 +91,7 @@ static void nix_c_primop_wrapper( if (ctx.last_err_code != NIX_OK) { /* TODO: Throw different errors depending on the error code */ - state.error("Error from builtin function: %s", *ctx.last_err).atPos(pos).debugThrow(); + state.error("Error from custom function: %s", *ctx.last_err).atPos(pos).debugThrow(); } if (!vTmp.isValid()) { From ab106c5ca3326c939467a1334aeb78f78485503e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 23 May 2024 19:20:04 +0200 Subject: [PATCH 5/7] C API: Test arity 2 primop --- tests/unit/libexpr/nix_api_expr.cc | 59 ++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/unit/libexpr/nix_api_expr.cc b/tests/unit/libexpr/nix_api_expr.cc index e78f2bf61..17ba610e4 100644 --- a/tests/unit/libexpr/nix_api_expr.cc +++ b/tests/unit/libexpr/nix_api_expr.cc @@ -226,6 +226,65 @@ TEST_F(nix_api_expr_test, nix_expr_primop) ASSERT_EQ(9, r); } +static void primop_repeat(void * user_data, nix_c_context * context, EvalState * state, Value ** args, Value * ret) +{ + assert(context); + assert(state); + assert(user_data == SAMPLE_USER_DATA); + + // Get the string to repeat + std::string s; + if (nix_get_string(context, args[0], OBSERVE_STRING(s)) != NIX_OK) + return; + + // Get the number of times to repeat + auto n = nix_get_int(context, args[1]); + if (nix_err_code(context) != NIX_OK) + return; + + // Repeat the string + std::string result; + for (int i = 0; i < n; ++i) + result += s; + + nix_init_string(context, ret, result.c_str()); +} + +TEST_F(nix_api_expr_test, nix_expr_primop_arity_2) +{ + PrimOp * primop = + nix_alloc_primop(ctx, primop_repeat, 2, "repeat", nullptr, "repeat a string", (void *) SAMPLE_USER_DATA); + assert_ctx_ok(); + Value * primopValue = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_primop(ctx, primopValue, primop); + assert_ctx_ok(); + + Value * hello = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_string(ctx, hello, "hello"); + assert_ctx_ok(); + + Value * three = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_int(ctx, three, 3); + assert_ctx_ok(); + + Value * partial = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_value_call(ctx, state, primopValue, hello, partial); + assert_ctx_ok(); + + Value * result = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_value_call(ctx, state, partial, three, result); + assert_ctx_ok(); + + std::string r; + nix_get_string(ctx, result, OBSERVE_STRING(r)); + ASSERT_STREQ("hellohellohello", r.c_str()); +} + static void primop_bad_no_return(void * user_data, nix_c_context * context, EvalState * state, Value ** args, Value * ret) { From 2497d1035182d0d9b209aee6cfcd1a2b2b2be55b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 23 May 2024 19:55:32 +0200 Subject: [PATCH 6/7] C API: Add nix_value_call_multi, NIX_VALUE_CALL _multi can be implemented more efficiently. NIX_VALUE_CALL is a convenient way to invoke it. --- src/libexpr-c/nix_api_expr.cc | 11 +++++++ src/libexpr-c/nix_api_expr.h | 41 +++++++++++++++++++++++++ tests/unit/libexpr/nix_api_expr.cc | 49 +++++++++++++++++++++++++++++- 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/libexpr-c/nix_api_expr.cc b/src/libexpr-c/nix_api_expr.cc index a29c3425e..b86d745db 100644 --- a/src/libexpr-c/nix_api_expr.cc +++ b/src/libexpr-c/nix_api_expr.cc @@ -65,6 +65,17 @@ nix_err nix_value_call(nix_c_context * context, EvalState * state, Value * fn, V NIXC_CATCH_ERRS } +nix_err nix_value_call_multi(nix_c_context * context, EvalState * state, Value * fn, size_t nargs, Value ** args, Value * value) +{ + if (context) + context->last_err_code = NIX_OK; + try { + state->state.callFunction(*(nix::Value *) fn, nargs, (nix::Value * *)args, *(nix::Value *) value, nix::noPos); + state->state.forceValue(*(nix::Value *) value, nix::noPos); + } + NIXC_CATCH_ERRS +} + nix_err nix_value_force(nix_c_context * context, EvalState * state, Value * value) { if (context) diff --git a/src/libexpr-c/nix_api_expr.h b/src/libexpr-c/nix_api_expr.h index 647b23569..b026ea70b 100644 --- a/src/libexpr-c/nix_api_expr.h +++ b/src/libexpr-c/nix_api_expr.h @@ -12,6 +12,7 @@ #include "nix_api_store.h" #include "nix_api_util.h" +#include #ifdef __cplusplus extern "C" { @@ -80,6 +81,46 @@ nix_err nix_expr_eval_from_string( */ nix_err nix_value_call(nix_c_context * context, EvalState * state, Value * fn, Value * arg, Value * value); +/** + * @brief Calls a Nix function with multiple arguments. + * + * Technically these are functions that return functions. It is common for Nix + * functions to be curried, so this function is useful for calling them. + * + * @param[out] context Optional, stores error information + * @param[in] state The state of the evaluation. + * @param[in] fn The Nix function to call. + * @param[in] nargs The number of arguments. + * @param[in] args The arguments to pass to the function. + * @param[out] value The result of the function call. + * + * @see nix_value_call For the single argument primitive. + * @see NIX_VALUE_CALL For a macro that wraps this function for convenience. + */ +nix_err nix_value_call_multi( + nix_c_context * context, EvalState * state, Value * fn, size_t nargs, Value ** args, Value * value); + +/** + * @brief Calls a Nix function with multiple arguments. + * + * Technically these are functions that return functions. It is common for Nix + * functions to be curried, so this function is useful for calling them. + * + * @param[out] context Optional, stores error information + * @param[in] state The state of the evaluation. + * @param[out] value The result of the function call. + * @param[in] fn The Nix function to call. + * @param[in] args The arguments to pass to the function. + * + * @see nix_value_call_multi + */ +#define NIX_VALUE_CALL(context, state, value, fn, ...) \ + do { \ + Value * args_array[] = {__VA_ARGS__}; \ + size_t nargs = sizeof(args_array) / sizeof(args_array[0]); \ + nix_value_call_multi(context, state, fn, nargs, args_array, value); \ + } while (0) + /** * @brief Forces the evaluation of a Nix value. * diff --git a/tests/unit/libexpr/nix_api_expr.cc b/tests/unit/libexpr/nix_api_expr.cc index 17ba610e4..fd7d043c0 100644 --- a/tests/unit/libexpr/nix_api_expr.cc +++ b/tests/unit/libexpr/nix_api_expr.cc @@ -250,7 +250,7 @@ static void primop_repeat(void * user_data, nix_c_context * context, EvalState * nix_init_string(context, ret, result.c_str()); } -TEST_F(nix_api_expr_test, nix_expr_primop_arity_2) +TEST_F(nix_api_expr_test, nix_expr_primop_arity_2_multiple_calls) { PrimOp * primop = nix_alloc_primop(ctx, primop_repeat, 2, "repeat", nullptr, "repeat a string", (void *) SAMPLE_USER_DATA); @@ -285,6 +285,38 @@ TEST_F(nix_api_expr_test, nix_expr_primop_arity_2) ASSERT_STREQ("hellohellohello", r.c_str()); } +TEST_F(nix_api_expr_test, nix_expr_primop_arity_2_single_call) +{ + PrimOp * primop = + nix_alloc_primop(ctx, primop_repeat, 2, "repeat", nullptr, "repeat a string", (void *) SAMPLE_USER_DATA); + assert_ctx_ok(); + Value * primopValue = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_primop(ctx, primopValue, primop); + assert_ctx_ok(); + + Value * hello = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_string(ctx, hello, "hello"); + assert_ctx_ok(); + + Value * three = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_int(ctx, three, 3); + assert_ctx_ok(); + + Value * result = nix_alloc_value(ctx, state); + assert_ctx_ok(); + NIX_VALUE_CALL(ctx, state, result, primopValue, hello, three); + assert_ctx_ok(); + + std::string r; + nix_get_string(ctx, result, OBSERVE_STRING(r)); + assert_ctx_ok(); + + ASSERT_STREQ("hellohellohello", r.c_str()); +} + static void primop_bad_no_return(void * user_data, nix_c_context * context, EvalState * state, Value ** args, Value * ret) { @@ -357,4 +389,19 @@ TEST_F(nix_api_expr_test, nix_expr_primop_bad_return_thunk) testing::HasSubstr("Implementation error in custom function: return value must not be a thunk"))); ASSERT_THAT(ctx->last_err, testing::Optional(testing::HasSubstr("badReturnThunk"))); } + +TEST_F(nix_api_expr_test, nix_value_call_multi_no_args) +{ + Value * n = nix_alloc_value(ctx, state); + nix_init_int(ctx, n, 3); + assert_ctx_ok(); + + Value * r = nix_alloc_value(ctx, state); + nix_value_call_multi(ctx, state, n, 0, nullptr, r); + assert_ctx_ok(); + + auto rInt = nix_get_int(ctx, r); + assert_ctx_ok(); + ASSERT_EQ(3, rInt); +} } // namespace nixC From 97c346329151916727a3dd154e4d088b2e16fd2d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 23 May 2024 19:56:32 +0200 Subject: [PATCH 7/7] C API: Refactor: use NIX_VALUE_CALL --- tests/unit/libexpr/nix_api_expr.cc | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/unit/libexpr/nix_api_expr.cc b/tests/unit/libexpr/nix_api_expr.cc index fd7d043c0..92a6a1175 100644 --- a/tests/unit/libexpr/nix_api_expr.cc +++ b/tests/unit/libexpr/nix_api_expr.cc @@ -373,14 +373,9 @@ TEST_F(nix_api_expr_test, nix_expr_primop_bad_return_thunk) nix_init_int(ctx, four, 4); assert_ctx_ok(); - Value * partial = nix_alloc_value(ctx, state); - assert_ctx_ok(); - nix_value_call(ctx, state, primopValue, toString, partial); - assert_ctx_ok(); - Value * result = nix_alloc_value(ctx, state); assert_ctx_ok(); - nix_value_call(ctx, state, partial, four, result); + NIX_VALUE_CALL(ctx, state, result, primopValue, toString, four); ASSERT_EQ(ctx->last_err_code, NIX_ERR_NIX_ERROR); ASSERT_THAT(