From a3361557e3f4a53b90ca5067e68ba9788df20928 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 15 Jul 2024 16:18:36 -0600 Subject: [PATCH] libexpr: refactor gc-agnostic helpers into one place Change-Id: Icc4b367e4f670d47256f62a3a002cd248a5c2d3b --- src/libexpr/attr-set.cc | 5 +- src/libexpr/eval-inline.hh | 22 +------ src/libexpr/eval.cc | 23 ++----- src/libexpr/eval.hh | 17 +----- src/libexpr/gc-alloc.hh | 119 +++++++++++++++++++++++++++++++++++++ src/libexpr/get-drvs.hh | 8 +-- src/libexpr/meson.build | 1 + src/libexpr/primops.cc | 11 ++-- src/libexpr/value.hh | 18 ++---- 9 files changed, 142 insertions(+), 82 deletions(-) create mode 100644 src/libexpr/gc-alloc.hh diff --git a/src/libexpr/attr-set.cc b/src/libexpr/attr-set.cc index 877116f1f..463296271 100644 --- a/src/libexpr/attr-set.cc +++ b/src/libexpr/attr-set.cc @@ -1,5 +1,6 @@ #include "attr-set.hh" -#include "eval-inline.hh" +#include "eval.hh" +#include "gc-alloc.hh" #include @@ -19,7 +20,7 @@ Bindings * EvalState::allocBindings(size_t capacity) throw Error("attribute set of size %d is too big", capacity); nrAttrsets++; nrAttrsInAttrsets += capacity; - return new (allocBytes(sizeof(Bindings) + sizeof(Attr) * capacity)) Bindings((Bindings::size_t) capacity); + return new (gcAllocBytes(sizeof(Bindings) + sizeof(Attr) * capacity)) Bindings((Bindings::size_t) capacity); } diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 94d4c55ef..4631b71eb 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -4,26 +4,10 @@ #include "print.hh" #include "eval.hh" #include "eval-error.hh" +#include "gc-alloc.hh" namespace nix { -/** - * Note: Various places expect the allocated memory to be zeroed. - */ -[[gnu::always_inline]] -inline void * allocBytes(size_t n) -{ - void * p; -#if HAVE_BOEHMGC - p = GC_MALLOC(n); -#else - p = calloc(n, 1); -#endif - if (!p) throw std::bad_alloc(); - return p; -} - - [[gnu::always_inline]] Value * EvalState::allocValue() { @@ -43,7 +27,7 @@ Value * EvalState::allocValue() *valueAllocCache = GC_NEXT(p); GC_NEXT(p) = nullptr; #else - void * p = allocBytes(sizeof(Value)); + void * p = gcAllocBytes(sizeof(Value)); #endif nrValues++; @@ -73,7 +57,7 @@ Env & EvalState::allocEnv(size_t size) env = (Env *) p; } else #endif - env = (Env *) allocBytes(sizeof(Env) + size * sizeof(Value *)); + env = (Env *) gcAllocBytes(sizeof(Env) + size * sizeof(Value *)); /* We assume that env->values has been cleared by the allocator; maybeThunk() and lookupVar fromWith expect this. */ diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index c7b708d1d..ce469e5c5 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -9,6 +9,7 @@ #include "store-api.hh" #include "derivations.hh" #include "downstream-placeholder.hh" +#include "gc-alloc.hh" #include "globals.hh" #include "eval-inline.hh" #include "filetransfer.hh" @@ -48,19 +49,6 @@ using json = nlohmann::json; namespace nix { -static char * allocString(size_t size) -{ - char * t; -#if HAVE_BOEHMGC - t = (char *) GC_MALLOC_ATOMIC(size); -#else - t = (char *) malloc(size); -#endif - if (!t) throw std::bad_alloc(); - return t; -} - - // When there's no need to write to the string, we can optimize away empty // string allocations. // This function handles makeImmutableString(std::string_view()) by returning @@ -70,13 +58,12 @@ static const char * makeImmutableString(std::string_view s) const size_t size = s.size(); if (size == 0) return ""; - auto t = allocString(size + 1); + auto t = gcAllocString(size + 1); memcpy(t, s.data(), size); t[size] = '\0'; return t; } - RootValue allocRootValue(Value * v) { #if HAVE_BOEHMGC @@ -806,7 +793,7 @@ static void copyContextToValue(Value & v, const NixStringContext & context) if (!context.empty()) { size_t n = 0; v.string.context = (const char * *) - allocBytes((context.size() + 1) * sizeof(char *)); + gcAllocBytes((context.size() + 1) * sizeof(char *)); for (auto & i : context) v.string.context[n++] = makeImmutableString(i.to_string()); v.string.context[n] = 0; @@ -862,7 +849,7 @@ void EvalState::mkList(Value & v, size_t size) { v.mkList(size); if (size > 2) - v.bigList.elems = (Value * *) allocBytes(size * sizeof(Value *)); + v.bigList.elems = (Value * *) gcAllocBytes(size * sizeof(Value *)); nrListElems += size; } @@ -2076,7 +2063,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) Value. allocating a GC'd string directly and moving it into a Value lets us avoid an allocation and copy. */ const auto c_str = [&] { - char * result = allocString(sSize + 1); + char * result = gcAllocString(sSize + 1); char * tmp = result; for (const auto & part : s) { memcpy(tmp, part->data(), part->size()); diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index ed51f8d1d..e54eede40 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -3,6 +3,7 @@ #include "attr-set.hh" #include "eval-error.hh" +#include "gc-alloc.hh" #include "types.hh" #include "value.hh" #include "nixexpr.hh" @@ -37,22 +38,6 @@ namespace eval_cache { class EvalCache; } -/** Alias for std::map which uses boehmgc's allocator conditional on us actually - * using boehmgc in this build. - */ -#if HAVE_BOEHMGC - template - using GcMap = std::map< - KeyT, - ValueT, - std::less, - traceable_allocator> - >; -#else - using GcMap = std::map -#endif - - /** * Function that implements a primop. */ diff --git a/src/libexpr/gc-alloc.hh b/src/libexpr/gc-alloc.hh new file mode 100644 index 000000000..a16bc65e4 --- /dev/null +++ b/src/libexpr/gc-alloc.hh @@ -0,0 +1,119 @@ +#pragma once +/// @file Aliases and wrapper functions that are transparently GC-enabled +/// if Lix is compiled with BoehmGC enabled. + +#include +#include +#include +#include +#include +#include + +#if HAVE_BOEHMGC +#include // std::less +#include // std::pair +#define GC_INCLUDE_NEW +#include +#include +#include + +/// calloc, transparently GC-enabled. +#define LIX_GC_CALLOC(size) GC_MALLOC(size) + +/// strdup, transaprently GC-enabled. +#define LIX_GC_STRDUP(str) GC_STRDUP(str) + +/// Atomic GC malloc() with GC enabled, or regular malloc() otherwise. +#define LIX_GC_MALLOC_ATOMIC(size) GC_MALLOC_ATOMIC(size) + +namespace nix +{ + +/// Alias for std::map which uses BoehmGC's allocator conditional on this Lix +/// build having GC enabled. +template +using GcMap = std::map< + KeyT, + ValueT, + std::less, + traceable_allocator> +>; + +/// Alias for std::vector which uses BoehmGC's allocator conditional on this Lix +/// build having GC enabled. +template +using GcVector = std::vector>; + +/// Alias for std::list which uses BoehmGC's allocator conditional on this Lix +/// build having GC enabled. +template +using GcList = std::list>; + +} + +#else + +#include + +/// calloc, transparently GC-enabled. +#define LIX_GC_CALLOC(size) calloc(size, 1) + +/// strdup, transparently GC-enabled. +#define LIX_GC_STRDUP(str) strdup(str) + +/// Atomic GC malloc() with GC enabled, or regular malloc() otherwise. +/// The returned memory must never contain pointers. +#define LIX_GC_MALLOC_ATOMIC(size) malloc(size) + +namespace nix +{ + +/// Alias for std::map which uses BoehmGC's allocator conditional on this Lix +/// build having GC enabled. +template +using GcMap = std::map; + +/// Alias for std::vector which uses BoehmGC's allocator conditional on this Lix +/// build having GC enabled. +template +using GcVector = std::vector; + +/// Alias for std::list which uses BoehmGC's allocator conditional on this Lix +/// build having GC enabled. +template +using GcList = std::list; + +} + +#endif + +namespace nix +{ + +[[gnu::always_inline]] +inline void * gcAllocBytes(size_t n) +{ + // Note: various places expect the allocated memory to be zero. + // Hence: calloc(). + void * ptr = LIX_GC_CALLOC(n); + if (ptr == nullptr) { + throw std::bad_alloc(); + } + + return ptr; +} + +/// GC-transparently allocates a buffer for a C-string of @ref size *bytes*, +/// meaning you should include the size needed by the NUL terminator in the +/// passed size. Memory allocated with this function must never contain other +/// pointers. +inline char * gcAllocString(size_t size) +{ + char * cstr = static_cast(LIX_GC_MALLOC_ATOMIC(size)); + if (cstr == nullptr) { + throw std::bad_alloc(); + } + return cstr; +} + +} diff --git a/src/libexpr/get-drvs.hh b/src/libexpr/get-drvs.hh index d620eca63..44aac3015 100644 --- a/src/libexpr/get-drvs.hh +++ b/src/libexpr/get-drvs.hh @@ -80,13 +80,7 @@ public: bool hasFailed() { return failed; }; }; - -#if HAVE_BOEHMGC -typedef std::list> DrvInfos; -#else -typedef std::list DrvInfos; -#endif - +using DrvInfos = GcList; /** * If value `v` denotes a derivation, return a DrvInfo object diff --git a/src/libexpr/meson.build b/src/libexpr/meson.build index 39493dadc..d29359298 100644 --- a/src/libexpr/meson.build +++ b/src/libexpr/meson.build @@ -58,6 +58,7 @@ libexpr_headers = files( 'function-trace.hh', 'gc-small-vector.hh', 'get-drvs.hh', + 'gc-alloc.hh', 'json-to-value.hh', 'nixexpr.hh', 'parser/change_head.hh', diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index c1c606d1d..0951a54de 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -622,14 +622,13 @@ struct CompareValues } }; - +/// NOTE: this type must NEVER be outside of GC-scanned memory. #if HAVE_BOEHMGC -typedef std::list> ValueList; +using UnsafeValueList = std::list>; #else -typedef std::list ValueList; +using UnsafeValueList = std::list; #endif - static Bindings::iterator getAttr( EvalState & state, Symbol attrSym, @@ -652,7 +651,7 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a state.forceList(*startSet->value, noPos, "while evaluating the 'startSet' attribute passed as argument to builtins.genericClosure"); - ValueList workSet; + UnsafeValueList workSet; for (auto elem : startSet->value->listItems()) workSet.push_back(elem); @@ -668,7 +667,7 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a /* Construct the closure by applying the operator to elements of `workSet', adding the result to `workSet', continuing until no new elements are found. */ - ValueList res; + UnsafeValueList res; // `doneKeys' doesn't need to be a GC root, because its values are // reachable from res. auto cmp = CompareValues(state, noPos, "while comparing the `key` attributes of two genericClosure elements"); diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 5c57257be..c35f88f8d 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -4,6 +4,7 @@ #include #include +#include "gc-alloc.hh" #include "symbol-table.hh" #include "value/context.hh" #include "input-accessor.hh" @@ -11,9 +12,6 @@ #include "print-options.hh" #include "checked-arithmetic.hh" -#if HAVE_BOEHMGC -#include -#endif #include namespace nix { @@ -464,17 +462,9 @@ void Value::mkBlackhole() thunk.expr = (Expr*) &eBlackHole; } - -#if HAVE_BOEHMGC -typedef std::vector> ValueVector; -typedef std::map, traceable_allocator>> ValueMap; -typedef std::map, traceable_allocator>> ValueVectorMap; -#else -typedef std::vector ValueVector; -typedef std::map ValueMap; -typedef std::map ValueVectorMap; -#endif - +using ValueVector = GcVector; +using ValueMap = GcMap; +using ValueVectorMap = std::map; /** * A value allocated in traceable memory.