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.

Author: glittershark

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

@@ -54,15 +54,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

@@ -35,14 +35,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 +289,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 +333,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 +389,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 +412,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 +429,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

@@ -27,17 +27,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(std::bit_cast<size_t>(sd1.data()), std::bit_cast<size_t>(sd2.data()));
+    ASSERT_EQ(std::bit_cast<size_t>(sd1), std::bit_cast<size_t>(sd2));
 }
 
 } // namespace nix

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>
@@ -47,28 +50,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)
@@ -584,7 +581,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)) {
@@ -818,7 +815,7 @@ DebugTraceStacker::DebugTraceStacker(EvalState & evalState, DebugTrace t)
 
 void Value::mkString(std::string_view s)
 {
-    mkStringNoCopy(makeImmutableString(s));
+    mkStringNoCopy(StringData::make(s));
 }
 
 static const char ** encodeContext(const NixStringContext & context)
@@ -827,7 +824,7 @@ static const char ** encodeContext(const NixStringContext & context)
         size_t n = 0;
         auto ctx = (const char **) allocBytes((context.size() + 1) * sizeof(char *));
         for (auto & i : context) {
-            ctx[n++] = makeImmutableString({i.to_string()});
+            ctx[n++] = StringData::make({i.to_string()}).view().data();
         }
         ctx[n] = nullptr;
         return ctx;
@@ -837,17 +834,17 @@ static const char ** encodeContext(const NixStringContext & context)
 
 void Value::mkString(std::string_view s, const NixStringContext & context)
 {
-    mkStringNoCopy(makeImmutableString(s), encodeContext(context));
+    mkStringNoCopy(StringData::make(s), encodeContext(context));
 }
 
-void Value::mkStringMove(const char * s, const NixStringContext & context)
+void Value::mkStringMove(const StringData & s, const NixStringContext & context)
 {
     mkStringNoCopy(s, encodeContext(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)
@@ -1559,6 +1556,7 @@ void EvalState::callFunction(Value & fun, std::span<Value *> args, Value & vRes,
                                 .withFrame(*vCur.lambda().env, lambda)
                                 .debugThrow();
                         }
+
                     unreachable();
                 }
             } else {
@@ -2095,25 +2093,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);
     }
 }
@@ -2369,7 +2369,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/nixexpr.hh

@@ -3,6 +3,7 @@
 
 #include <map>
 #include <span>
+#include <memory>
 #include <vector>
 #include <memory_resource>
 #include <algorithm>
@@ -193,22 +194,18 @@ struct ExprString : Expr
      * This is only for strings already allocated in our polymorphic allocator,
      * or that live at least that long (e.g. c++ string literals)
      */
-    ExprString(const char * s)
+    ExprString(const StringData & s)
     {
         v.mkStringNoCopy(s);
     };
 
     ExprString(std::pmr::polymorphic_allocator<char> & alloc, std::string_view sv)
     {
-        auto len = sv.length();
-        if (len == 0) {
-            v.mkStringNoCopy("");
+        if (sv.size() == 0) {
+            v.mkStringNoCopy(""_sds);
             return;
         }
-        char * s = alloc.allocate(len + 1);
-        sv.copy(s, len);
-        s[len] = '\0';
-        v.mkStringNoCopy(s);
+        v.mkStringNoCopy(StringData::make(*alloc.resource(), sv));
     };
 
     Value * maybeThunk(EvalState & state, Env & env) override;
@@ -223,11 +220,7 @@ struct ExprPath : Expr
     ExprPath(std::pmr::polymorphic_allocator<char> & alloc, ref<SourceAccessor> accessor, std::string_view sv)
         : accessor(accessor)
     {
-        auto len = sv.length();
-        char * s = alloc.allocate(len + 1);
-        sv.copy(s, len);
-        s[len] = '\0';
-        v.mkPath(&*accessor, s);
+        v.mkPath(&*accessor, StringData::make(*alloc.resource(), sv));
     }
 
     Value * maybeThunk(EvalState & state, Env & env) override;