From a81f83604b1b487c1f8763aa1c949f95a370d47a Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Tue, 17 Feb 2026 13:11:25 +0300 Subject: [PATCH 1/2] libexpr: Add marker values to InternalType enum This reduces the churn when changing up the order of values in a follow-up commit. This should have been done from the start ideally to improve readability. --- src/libexpr/include/nix/expr/value.hh | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index aef10b9c653..e7e9b6af706 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -35,7 +35,7 @@ class BindingsBuilder; * about how this is mapped into the alignment bits to save significant memory. * This also restricts the number of internal types represented with distinct memory layouts. */ -typedef enum { +enum InternalType { tUninitialized = 0, /* layout: Single/zero field payload */ tInt = 1, @@ -46,16 +46,19 @@ typedef enum { tPrimOp, tAttrs, /* layout: Pair of pointers payload */ - tListSmall, + tFirstPairOfPointers, + tListSmall = tFirstPairOfPointers, tPrimOpApp, tApp, tThunk, tLambda, + tLastPairOfPointers = tLambda, /* layout: Single untaggable field */ - tListN, + tFirstSingleUntaggable, + tListN = tFirstSingleUntaggable, tString, tPath, -} InternalType; +}; /** * This type abstracts over all actual value types in the language, @@ -633,7 +636,7 @@ class alignas(16) template void setPairOfPointersPayload(T * firstPtrField, U * secondPtrField) noexcept { - static_assert(type >= tListSmall && type <= tLambda); + static_assert(type >= tFirstPairOfPointers && type <= tLastPairOfPointers); { auto firstFieldPayload = std::bit_cast(firstPtrField); assertAligned(firstFieldPayload); @@ -642,7 +645,7 @@ class alignas(16) { auto secondFieldPayload = std::bit_cast(secondPtrField); assertAligned(secondFieldPayload); - payload[1] = (type - tListSmall) | secondFieldPayload; + payload[1] = (type - tFirstPairOfPointers) | secondFieldPayload; } } @@ -670,9 +673,9 @@ protected: case pdListN: case pdString: case pdPath: - return static_cast(tListN + (pd - pdListN)); + return static_cast(tFirstSingleUntaggable + (pd - pdListN)); case pdPairOfPointers: - return static_cast(tListSmall + (payload[1] & discriminatorMask)); + return static_cast(tFirstPairOfPointers + (payload[1] & discriminatorMask)); [[unlikely]] default: unreachable(); } From 3df91bea6270b01045ebaa54275a4a3f4aca437d Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Tue, 17 Feb 2026 16:50:07 +0300 Subject: [PATCH 2/2] libexpr: Optimise Value::type(), ValueStorage::getInternalType() Using nix::unreachable() in getInternalType() and type() turns out to be quite expensive and prevents inlining. Also Value::type got compiled to a jump table which has a high overhead from indirect jumps. Using an explicit lookup table turns out to be more efficient. This does mean that we lose out on nice diagnostics from nix::unreachable calls, but this code is probably one of the hottests functions in the whole evaluator, so I think the tradeoff is worth it. The nixUnreachableWhenHardened boils down to nix::unreachable when UBSan is enabled so we still have good coverage there. --- .../common/asan-options/meson.build | 6 ++ src/libexpr-tests/value/value.cc | 3 +- src/libexpr/eval.cc | 2 +- src/libexpr/include/nix/expr/value.hh | 76 +++++++++---------- src/libutil/include/nix/util/error.hh | 9 +++ 5 files changed, 55 insertions(+), 41 deletions(-) diff --git a/nix-meson-build-support/common/asan-options/meson.build b/nix-meson-build-support/common/asan-options/meson.build index 56e6a6a56a7..c0d02186fa5 100644 --- a/nix-meson-build-support/common/asan-options/meson.build +++ b/nix-meson-build-support/common/asan-options/meson.build @@ -9,3 +9,9 @@ endif if 'address' in get_option('b_sanitize') deps_other += declare_dependency(sources : 'asan-options.cc') endif + +if 'undefined' in get_option('b_sanitize') + add_project_arguments('-DNIX_UBSAN_ENABLED=1', language : 'cpp') +else + add_project_arguments('-DNIX_UBSAN_ENABLED=0', language : 'cpp') +endif diff --git a/src/libexpr-tests/value/value.cc b/src/libexpr-tests/value/value.cc index 420db0f31b1..285b586c0df 100644 --- a/src/libexpr-tests/value/value.cc +++ b/src/libexpr-tests/value/value.cc @@ -13,8 +13,7 @@ TEST_F(ValueTest, unsetValue) { Value unsetValue; ASSERT_EQ(false, unsetValue.isValid()); - ASSERT_EQ(nThunk, unsetValue.type(true)); - ASSERT_DEATH(unsetValue.type(), ""); + ASSERT_EQ(nThunk, unsetValue.type()); } TEST_F(ValueTest, vInt) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index d9d8a986a3e..005ea7ab5f7 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -466,7 +466,7 @@ void EvalState::addConstant(const std::string & name, Value * v, Constant info) We might know the type of a thunk in advance, so be allowed to just write it down in that case. */ - if (auto gotType = v->type(true); gotType != nThunk) + if (auto gotType = v->type(); gotType != nThunk) assert(info.type == gotType); /* Install value the base environment. */ diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index e7e9b6af706..5b317e7acfc 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -58,6 +58,7 @@ enum InternalType { tListN = tFirstSingleUntaggable, tString, tPath, + tNumberOfInternalTypes, // Must be last }; /** @@ -677,7 +678,7 @@ protected: case pdPairOfPointers: return static_cast(tFirstPairOfPointers + (payload[1] & discriminatorMask)); [[unlikely]] default: - unreachable(); + nixUnreachableWhenHardened(); } } @@ -1030,7 +1031,7 @@ private: T getStorage() const noexcept { if (getInternalType() != detail::payloadTypeToInternalType) [[unlikely]] - unreachable(); + nixUnreachableWhenHardened(); T out; ValueStorage::getStorage(out); return out; @@ -1082,45 +1083,44 @@ public: * Returns the normal type of a Value. This only returns nThunk if * the Value hasn't been forceValue'd * - * @param invalidIsThunk Instead of aborting an an invalid (probably + * @param invalidIsThunk Instead of UB an an invalid (probably * 0, so uninitialized) internal type, return `nThunk`. */ - inline ValueType type(bool invalidIsThunk = false) const - { - switch (getInternalType()) { - case tUninitialized: - break; - case tInt: - return nInt; - case tBool: - return nBool; - case tString: - return nString; - case tPath: - return nPath; - case tNull: - return nNull; - case tAttrs: - return nAttrs; - case tListSmall: - case tListN: - return nList; - case tLambda: - case tPrimOp: - case tPrimOpApp: - return nFunction; - case tExternal: - return nExternal; - case tFloat: - return nFloat; - case tThunk: - case tApp: - return nThunk; + template + inline ValueType type() const + { + /* Explicit lookup table. switch() might compile down (and it does at least with GCC 14) + to a jump table. Let's help the compiler a bit here. */ + static constexpr auto table = [] { + std::array t{}; + t[tUninitialized] = nThunk; + t[tInt] = nInt; + t[tBool] = nBool; + t[tNull] = nNull; + t[tFloat] = nFloat; + t[tExternal] = nExternal; + t[tAttrs] = nAttrs; + t[tPrimOp] = nFunction; + t[tLambda] = nFunction; + t[tPrimOpApp] = nFunction; + t[tApp] = nThunk; + t[tThunk] = nThunk; + t[tListSmall] = nList; + t[tListN] = nList; + t[tString] = nString; + t[tPath] = nPath; + return t; + }(); + + auto it = getInternalType(); + if (it == tUninitialized || it >= tNumberOfInternalTypes) [[unlikely]] { + if constexpr (invalidIsThunk) + return nThunk; + else + nixUnreachableWhenHardened(); } - if (invalidIsThunk) - return nThunk; - else - unreachable(); + + return table[it]; } /** diff --git a/src/libutil/include/nix/util/error.hh b/src/libutil/include/nix/util/error.hh index 284cded8ea9..d7ca4397450 100644 --- a/src/libutil/include/nix/util/error.hh +++ b/src/libutil/include/nix/util/error.hh @@ -350,6 +350,15 @@ int handleExceptions(const std::string & programName, std::function fun) */ [[gnu::noinline, gnu::cold, noreturn]] void unreachable(std::source_location loc = std::source_location::current()); +#if NIX_UBSAN_ENABLED == 1 +/* When building with sanitizers, also enable expensive unreachable checks. In + optimised builds this explicitly invokes UB with std::unreachable for better + optimisations. */ +# define nixUnreachableWhenHardened ::nix::unreachable +#else +# define nixUnreachableWhenHardened std::unreachable +#endif + #ifdef _WIN32 namespace windows {