Use hybrid C / Pascal strings in the evaluator

Replace the null-terminated C-style strings in Value with hybrid C /
Pascal strings, where the length is stored in the allocation before the
data, and there is still a null byte at the end for the sake of C
interopt.

Co-Authored-By: Taeer Bar-Yam <[email protected]>

Author: 2 people

src/libexpr-c/nix_api_value.cc

@@ -254,7 +254,7 @@ const char * nix_get_path_string(nix_c_context * context, const nix_value * valu
         // We could use v.path().to_string().c_str(), but I'm concerned this
         // crashes. Looks like .path() allocates a CanonPath with a copy of the
         // string, then it gets the underlying data from that.
-        return v.pathStr();
+        return v.pathStr().view().data();
     }
     NIXC_CATCH_ERRS_NULL
 }
@@ -439,7 +439,7 @@ nix_get_attr_byidx(nix_c_context * context, nix_value * value, EvalState * state
             return nullptr;
         }
         const nix::Attr & a = (*v.attrs())[i];
-        *name = state->state.symbols[a.name].c_str();
+        *name = state->state.symbols[a.name].string_data().view().data();
         nix_gc_incref(nullptr, a.value);
         state->state.forceValue(*a.value, nix::noPos);
         return as_nix_value_ptr(a.value);
@@ -460,7 +460,7 @@ nix_value * nix_get_attr_byidx_lazy(
             return nullptr;
         }
         const nix::Attr & a = (*v.attrs())[i];
-        *name = state->state.symbols[a.name].c_str();
+        *name = state->state.symbols[a.name].string_data().view().data();
         nix_gc_incref(nullptr, a.value);
         // Note: intentionally NOT calling forceValue() to keep the attribute lazy
         return as_nix_value_ptr(a.value);
@@ -480,7 +480,7 @@ const char * nix_get_attr_name_byidx(nix_c_context * context, nix_value * value,
             return nullptr;
         }
         const nix::Attr & a = (*v.attrs())[i];
-        return state->state.symbols[a.name].c_str();
+        return state->state.symbols[a.name].string_data().view().data();
     }
     NIXC_CATCH_ERRS_NULL
 }

src/libexpr-test-support/include/nix/expr/tests/libexpr.hh

@@ -104,8 +104,10 @@ MATCHER(IsAttrs, "")
 MATCHER_P(IsStringEq, s, fmt("The string is equal to \"%1%\"", s))
 {
     if (arg.type() != nString) {
+        *result_listener << "of non-String type " << arg.type();
         return false;
     }
+    *result_listener << "string(" << arg.string_view() << ")";
     return arg.string_view() == s;
 }
 

src/libexpr-tests/json.cc

@@ -1,5 +1,6 @@
 #include "nix/expr/tests/libexpr.hh"
 #include "nix/expr/value-to-json.hh"
+#include "nix/expr/static-string-data.hh"
 
 namespace nix {
 // Testing the conversion to JSON
@@ -54,15 +55,15 @@ TEST_F(JSONValueTest, IntNegative)
 TEST_F(JSONValueTest, String)
 {
     Value v;
-    v.mkStringNoCopy("test");
+    v.mkStringNoCopy("test"_sds);
     ASSERT_EQ(getJSONValue(v), "\"test\"");
 }
 
 TEST_F(JSONValueTest, StringQuotes)
 {
     Value v;
 
-    v.mkStringNoCopy("test\"");
+    v.mkStringNoCopy("test\""_sds);
     ASSERT_EQ(getJSONValue(v), "\"test\\\"\"");
 }
 

src/libexpr-tests/value/print.cc

@@ -1,4 +1,5 @@
 #include "nix/expr/tests/libexpr.hh"
+#include "nix/expr/static-string-data.hh"
 
 #include "nix/expr/value.hh"
 #include "nix/expr/print.hh"
@@ -35,14 +36,14 @@ TEST_F(ValuePrintingTests, tBool)
 TEST_F(ValuePrintingTests, tString)
 {
     Value vString;
-    vString.mkStringNoCopy("some-string");
+    vString.mkStringNoCopy("some-string"_sds);
     test(vString, "\"some-string\"");
 }
 
 TEST_F(ValuePrintingTests, tPath)
 {
     Value vPath;
-    vPath.mkStringNoCopy("/foo");
+    vPath.mkStringNoCopy("/foo"_sds);
     test(vPath, "\"/foo\"");
 }
 
@@ -289,10 +290,10 @@ TEST_F(StringPrintingTests, maxLengthTruncation)
 TEST_F(ValuePrintingTests, attrsTypeFirst)
 {
     Value vType;
-    vType.mkStringNoCopy("puppy");
+    vType.mkStringNoCopy("puppy"_sds);
 
     Value vApple;
-    vApple.mkStringNoCopy("apple");
+    vApple.mkStringNoCopy("apple"_sds);
 
     BindingsBuilder builder = state.buildBindings(10);
     builder.insert(state.symbols.create("type"), &vType);
@@ -333,15 +334,15 @@ TEST_F(ValuePrintingTests, ansiColorsBool)
 TEST_F(ValuePrintingTests, ansiColorsString)
 {
     Value v;
-    v.mkStringNoCopy("puppy");
+    v.mkStringNoCopy("puppy"_sds);
 
     test(v, ANSI_MAGENTA "\"puppy\"" ANSI_NORMAL, PrintOptions{.ansiColors = true});
 }
 
 TEST_F(ValuePrintingTests, ansiColorsStringElided)
 {
     Value v;
-    v.mkStringNoCopy("puppy");
+    v.mkStringNoCopy("puppy"_sds);
 
     test(
         v,
@@ -389,7 +390,7 @@ TEST_F(ValuePrintingTests, ansiColorsAttrs)
 TEST_F(ValuePrintingTests, ansiColorsDerivation)
 {
     Value vDerivation;
-    vDerivation.mkStringNoCopy("derivation");
+    vDerivation.mkStringNoCopy("derivation"_sds);
 
     BindingsBuilder builder = state.buildBindings(10);
     builder.insert(state.s.type, &vDerivation);
@@ -412,7 +413,7 @@ TEST_F(ValuePrintingTests, ansiColorsError)
 {
     Value throw_ = state.getBuiltin("throw");
     Value message;
-    message.mkStringNoCopy("uh oh!");
+    message.mkStringNoCopy("uh oh!"_sds);
     Value vError;
     vError.mkApp(&throw_, &message);
 
@@ -429,12 +430,12 @@ TEST_F(ValuePrintingTests, ansiColorsDerivationError)
 {
     Value throw_ = state.getBuiltin("throw");
     Value message;
-    message.mkStringNoCopy("uh oh!");
+    message.mkStringNoCopy("uh oh!"_sds);
     Value vError;
     vError.mkApp(&throw_, &message);
 
     Value vDerivation;
-    vDerivation.mkStringNoCopy("derivation");
+    vDerivation.mkStringNoCopy("derivation"_sds);
 
     BindingsBuilder builder = state.buildBindings(10);
     builder.insert(state.s.type, &vDerivation);

src/libexpr-tests/value/value.cc

@@ -1,4 +1,5 @@
 #include "nix/expr/value.hh"
+#include "nix/expr/static-string-data.hh"
 
 #include "nix/store/tests/libstore.hh"
 #include <gtest/gtest.h>
@@ -27,17 +28,17 @@ TEST_F(ValueTest, staticString)
 {
     Value vStr1;
     Value vStr2;
-    vStr1.mkStringNoCopy("foo");
-    vStr2.mkStringNoCopy("foo");
+    vStr1.mkStringNoCopy("foo"_sds);
+    vStr2.mkStringNoCopy("foo"_sds);
 
-    auto sd1 = vStr1.string_view();
-    auto sd2 = vStr2.string_view();
+    auto & sd1 = vStr1.string_data();
+    auto & sd2 = vStr2.string_data();
 
     // The strings should be the same
-    ASSERT_EQ(sd1, sd2);
+    ASSERT_EQ(sd1.view(), sd2.view());
 
     // The strings should also be backed by the same (static) allocation
-    ASSERT_EQ(sd1.data(), sd2.data());
+    ASSERT_EQ(&sd1, &sd2);
 }
 
 } // namespace nix

src/libexpr/eval-cache.cc

@@ -147,7 +147,7 @@ struct AttrDb
                 for (auto * elem : *context) {
                     if (!first)
                         ctx.push_back(' ');
-                    ctx.append(elem);
+                    ctx.append(elem->view());
                     first = false;
                 }
                 state->insertAttributeWithContext.use()(key.first)(symbols[key.second])(AttrType::String) (s) (ctx)

src/libexpr/eval.cc

@@ -3,6 +3,7 @@
 #include "nix/expr/primops.hh"
 #include "nix/expr/print-options.hh"
 #include "nix/expr/symbol-table.hh"
+#include "nix/expr/value.hh"
 #include "nix/util/exit.hh"
 #include "nix/util/types.hh"
 #include "nix/util/util.hh"
@@ -28,6 +29,8 @@
 #include "parser-tab.hh"
 
 #include <algorithm>
+#include <cstddef>
+#include <cstdlib>
 #include <iostream>
 #include <sstream>
 #include <cstring>
@@ -48,28 +51,22 @@ using json = nlohmann::json;
 
 namespace nix {
 
-static char * allocString(size_t size)
+StringData & StringData::alloc(size_t size)
 {
-    char * t;
-    t = (char *) GC_MALLOC_ATOMIC(size);
-    if (!t)
+    auto res = (StringData *) GC_MALLOC_ATOMIC(sizeof(size_t) + size + 1);
+    if (!res) {
         throw std::bad_alloc();
-    return t;
+    }
+    res->m_size = size;
+    return *res;
 }
 
-// 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
-// the empty string.
-static const char * makeImmutableString(std::string_view s)
+StringData & StringData::make(std::string_view s)
 {
-    const size_t size = s.size();
-    if (size == 0)
-        return "";
-    auto t = allocString(size + 1);
-    memcpy(t, s.data(), size);
-    t[size] = '\0';
-    return t;
+    auto & res = alloc(s.size());
+    memcpy(&res.m_data, s.data(), s.size());
+    res.m_data[s.size()] = '\0';
+    return res;
 }
 
 RootValue allocRootValue(Value * v)
@@ -585,7 +582,7 @@ std::optional<EvalState::Doc> EvalState::getDoc(Value & v)
             .name = name,
             .arity = 0, // FIXME: figure out how deep by syntax only? It's not semantically useful though...
             .args = {},
-            .doc = makeImmutableString(s.view()), // NOTE: memory leak when compiled without GC
+            .doc = StringData::make(s.view()).view().data(), // NOTE: memory leak when compiled without GC
         };
     }
     if (isFunctor(v)) {
@@ -819,7 +816,7 @@ DebugTraceStacker::DebugTraceStacker(EvalState & evalState, DebugTrace t)
 
 void Value::mkString(std::string_view s)
 {
-    mkStringNoCopy(makeImmutableString(s));
+    mkStringNoCopy(StringData::make(s));
 }
 
 Value::StringWithContext::Context * Value::StringWithContext::Context::fromBuilder(const NixStringContext & context)
@@ -829,23 +826,23 @@ Value::StringWithContext::Context * Value::StringWithContext::Context::fromBuild
 
     auto ctx = new (allocBytes(sizeof(Context) + context.size() * sizeof(value_type))) Context(context.size());
     std::ranges::transform(
-        context, ctx->elems, [](const NixStringContextElem & elt) { return makeImmutableString(elt.to_string()); });
+        context, ctx->elems, [](const NixStringContextElem & elt) { return &StringData::make(elt.to_string()); });
     return ctx;
 }
 
 void Value::mkString(std::string_view s, const NixStringContext & context)
 {
-    mkStringNoCopy(makeImmutableString(s), Value::StringWithContext::Context::fromBuilder(context));
+    mkStringNoCopy(StringData::make(s), Value::StringWithContext::Context::fromBuilder(context));
 }
 
-void Value::mkStringMove(const char * s, const NixStringContext & context)
+void Value::mkStringMove(const StringData & s, const NixStringContext & context)
 {
     mkStringNoCopy(s, Value::StringWithContext::Context::fromBuilder(context));
 }
 
 void Value::mkPath(const SourcePath & path)
 {
-    mkPath(&*path.accessor, makeImmutableString(path.path.abs()));
+    mkPath(&*path.accessor, StringData::make(path.path.abs()));
 }
 
 inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval)
@@ -1558,6 +1555,7 @@ void EvalState::callFunction(Value & fun, std::span<Value *> args, Value & vRes,
                                 .withFrame(*vCur.lambda().env, lambda)
                                 .debugThrow();
                         }
+
                     unreachable();
                 }
             } else {
@@ -2094,25 +2092,27 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
     } else if (firstType == nFloat) {
         v.mkFloat(nf);
     } else if (firstType == nPath) {
-        if (!context.empty())
+        if (!context.empty()) {
             state.error<EvalError>("a string that refers to a store path cannot be appended to a path")
                 .atPos(pos)
                 .withFrame(env, *this)
                 .debugThrow();
+        }
+
         std::string result_str;
         result_str.reserve(sSize);
         for (const auto & part : strings) {
             result_str += *part;
         }
         v.mkPath(state.rootPath(CanonPath(result_str)));
     } else {
-        char * result_str = allocString(sSize + 1);
-        char * tmp = result_str;
+        auto & result_str = StringData::alloc(sSize);
+        auto * tmp = result_str.data();
         for (const auto & part : strings) {
             memcpy(tmp, part->data(), part->size());
             tmp += part->size();
         }
-        *tmp = 0;
+        *tmp = '\0';
         v.mkStringMove(result_str, context);
     }
 }
@@ -2288,7 +2288,7 @@ void copyContext(const Value & v, NixStringContext & context, const Experimental
 {
     if (auto * ctx = v.context())
         for (auto * elem : *ctx)
-            context.insert(NixStringContextElem::parse(elem, xpSettings));
+            context.insert(NixStringContextElem::parse(elem->view(), xpSettings));
 }
 
 std::string_view EvalState::forceString(
@@ -2370,7 +2370,7 @@ BackedStringView EvalState::coerceToString(
         if (!canonicalizePath && !copyToStore) {
             // FIXME: hack to preserve path literals that end in a
             // slash, as in /foo/${x}.
-            return v.pathStrView();
+            return v.pathStr().view();
         } else if (copyToStore) {
             return store->printStorePath(copyPathToStore(context, v.path()));
         } else {

src/libexpr/include/nix/expr/eval-cache.hh

@@ -79,7 +79,11 @@ struct int_t
 
 typedef uint64_t AttrId;
 typedef std::pair<AttrId, Symbol> AttrKey;
-typedef std::pair<std::string, NixStringContext> string_t;
+typedef std::pair<
+    // XXX aspen: StringData?
+    std::string,
+    NixStringContext>
+    string_t;
 
 typedef std::variant<
     std::vector<Symbol>,

src/libexpr/include/nix/expr/meson.build

@@ -31,6 +31,7 @@ headers = [ config_pub_h ] + files(
   'print.hh',
   'repl-exit-status.hh',
   'search-path.hh',
+  'static-string-data.hh',
   'symbol-table.hh',
   'value-to-json.hh',
   'value-to-xml.hh',