Merge branch 'kma-test-memleaks' into main-dev

include/stringzilla/stringzilla.h

@@ -2091,27 +2091,26 @@ SZ_PUBLIC void sz_string_init(sz_string_t *string) {
     string->u64s[3] = 0;
 }
 
-SZ_PUBLIC sz_bool_t sz_string_init_from(sz_string_t *string, sz_cptr_t added_start, sz_size_t added_length,
+SZ_PUBLIC sz_bool_t sz_string_init_from(sz_string_t *string, sz_cptr_t start, sz_size_t length,
                                         sz_memory_allocator_t *allocator) {
-
+    size_t space_needed = length + 1; // space for trailing \0
     SZ_ASSERT(string && allocator, "String and allocator can't be NULL.");
     // If we are lucky, no memory allocations will be needed.
-    if (added_length + 1 <= sz_string_stack_space) {
+    if (space_needed <= sz_string_stack_space) {
         string->on_stack.start = &string->on_stack.chars[0];
-        sz_copy(string->on_stack.start, added_start, added_length);
-        string->on_stack.start[added_length] = 0;
-        string->on_stack.length = added_length;
-        return sz_true_k;
+        string->on_stack.length = length;
     }
-    // If we are not lucky, we need to allocate memory.
-    sz_ptr_t new_start = (sz_ptr_t)allocator->allocate(added_length + 1, allocator->handle);
-    if (!new_start) return sz_false_k;
-
+    else {
+        // If we are not lucky, we need to allocate memory.
+        string->on_heap.start = (sz_ptr_t)allocator->allocate(space_needed, allocator->handle);
+        if (!string->on_heap.start) return sz_false_k;
+        string->on_heap.length = length;
+        string->on_heap.space = space_needed;
+    }
+    SZ_ASSERT(&string->on_stack.start == &string->on_heap.start, "Alignment confusion");
     // Copy into the new buffer.
-    string->on_heap.start = new_start;
-    sz_copy(string->on_heap.start, added_start, added_length);
-    string->on_heap.start[added_length] = 0;
-    string->on_heap.length = added_length;
+    sz_copy(string->on_heap.start, start, length);
+    string->on_heap.start[length] = 0;
     return sz_true_k;
 }
 

include/stringzilla/stringzilla.hpp

@@ -1195,6 +1195,26 @@ class basic_string {
         SZ_ASSERT(*this == other, "");
     }
 
+  protected:
+    void move(basic_string &other) noexcept {
+        // We can't just assign the other string state, as its start address may be somewhere else on the stack.
+        sz_ptr_t string_start;
+        sz_size_t string_length;
+        sz_size_t string_space;
+        sz_bool_t string_is_on_heap;
+        sz_string_unpack(&other.string_, &string_start, &string_length, &string_space, &string_is_on_heap);
+
+        // Acquire the old string's value bitwise
+        *(&string_) = *(&other.string_);
+        if (!string_is_on_heap) {
+            // Reposition the string start pointer to the stack if it fits.
+            string_.on_stack.start = &string_.on_stack.chars[0];
+        }
+        sz_string_init(&other.string_); // Discard the other string.
+    }
+
+    bool is_sso() const { return string_.on_stack.start == &string_.on_stack.chars[0]; }
+
   public:
     // Member types
     using traits_type = std::char_traits<char>;
@@ -1231,32 +1251,16 @@ class basic_string {
         });
     }
 
-    basic_string(basic_string &&other) noexcept : string_(other.string_) {
-        // We can't just assign the other string state, as its start address may be somewhere else on the stack.
-        sz_ptr_t string_start;
-        sz_size_t string_length;
-        sz_size_t string_space;
-        sz_bool_t string_is_on_heap;
-        sz_string_unpack(&other.string_, &string_start, &string_length, &string_space, &string_is_on_heap);
-
-        // Reposition the string start pointer to the stack if it fits.
-        string_.on_stack.start = string_is_on_heap ? string_start : &string_.on_stack.chars[0];
-        // XXX: memory leak
-        sz_string_init(&other.string_); // Discard the other string.
-    }
+    basic_string(basic_string &&other) noexcept : string_(other.string_) { move(other); }
 
     basic_string &operator=(basic_string &&other) noexcept {
-        // We can't just assign the other string state, as its start address may be somewhere else on the stack.
-        sz_ptr_t string_start;
-        sz_size_t string_length;
-        sz_size_t string_space;
-        sz_bool_t string_is_on_heap;
-        sz_string_unpack(&other.string_, &string_start, &string_length, &string_space, &string_is_on_heap);
-
-        // Reposition the string start pointer to the stack if it fits.
-        string_.on_stack.start = string_is_on_heap ? string_start : &string_.on_stack.chars[0];
-        // XXX: memory leak
-        sz_string_init(&other.string_); // Discard the other string.
+        if (!is_sso()) {
+            with_alloc([&](alloc_t &alloc) {
+                sz_string_free(&string_, &alloc);
+                return sz_true_k;
+            });
+        }
+        move(other);
         return *this;
     }
 

scripts/test.cpp

@@ -136,6 +136,122 @@ static void test_constructors() {
     assert(std::equal(strings.begin(), strings.end(), assignments.begin()));
 }
 
+#include <cstdarg>
+#include <memory>
+
+struct accounting_allocator : protected std::allocator<char> {
+    static bool verbose;
+    static size_t current_bytes_alloced;
+
+    static void dprintf(const char *fmt, ...) {
+        if (!verbose) return;
+        va_list args;
+        va_start(args, fmt);
+        vprintf(fmt, args);
+        va_end(args);
+    }
+
+    char *allocate(size_t n) {
+        current_bytes_alloced += n;
+        dprintf("alloc %zd -> %zd\n", n, current_bytes_alloced);
+        return std::allocator<char>::allocate(n);
+    }
+    void deallocate(char *val, size_t n) {
+        assert(n <= current_bytes_alloced);
+        current_bytes_alloced -= n;
+        dprintf("dealloc: %zd -> %zd\n", n, current_bytes_alloced);
+        std::allocator<char>::deallocate(val, n);
+    }
+
+    template <typename Lambda>
+    static size_t account_block(Lambda lambda) {
+        auto before = accounting_allocator::current_bytes_alloced;
+        dprintf("starting block: %zd\n", before);
+        lambda();
+        auto after = accounting_allocator::current_bytes_alloced;
+        dprintf("ending block: %zd\n", after);
+        return after - before;
+    }
+};
+
+bool accounting_allocator::verbose = false;
+size_t accounting_allocator::current_bytes_alloced;
+
+template <typename Lambda>
+static void assert_balanced_memory(Lambda lambda) {
+    auto bytes = accounting_allocator::account_block(lambda);
+    assert(bytes == 0);
+}
+
+static void test_memory_stability_len(int len = 1 << 10) {
+    int iters(4);
+
+    assert(accounting_allocator::current_bytes_alloced == 0);
+    using string_t = sz::basic_string<accounting_allocator>;
+    string_t base;
+
+    for (auto i = 0; i < len; i++) base.push_back('c');
+    assert(base.length() == len);
+
+    // Do copies leak?
+    assert_balanced_memory([&]() {
+        for (auto i = 0; i < iters; i++) {
+            string_t copy(base);
+            assert(copy.length() == len);
+            assert(copy == base);
+        }
+    });
+
+    // How about assignments?
+    assert_balanced_memory([&]() {
+        for (auto i = 0; i < iters; i++) {
+            string_t copy;
+            copy = base;
+            assert(copy.length() == len);
+            assert(copy == base);
+        }
+    });
+
+    // How about the move ctor?
+    assert_balanced_memory([&]() {
+        for (auto i = 0; i < iters; i++) {
+            string_t unique_item(base);
+            assert(unique_item.length() == len);
+            assert(unique_item == base);
+            string_t copy(std::move(unique_item));
+            assert(copy.length() == len);
+            assert(copy == base);
+        }
+    });
+
+    // And the move assignment operator with an empty target payload?
+    assert_balanced_memory([&]() {
+        for (auto i = 0; i < iters; i++) {
+            string_t unique_item(base);
+            string_t copy;
+            copy = std::move(unique_item);
+            assert(copy.length() == len);
+            assert(copy == base);
+        }
+    });
+
+    // And move assignment where the target had a payload?
+    assert_balanced_memory([&]() {
+        for (auto i = 0; i < iters; i++) {
+            string_t unique_item(base);
+            string_t copy;
+            for (auto j = 0; j < 317; j++) copy.push_back('q');
+            copy = std::move(unique_item);
+            assert(copy.length() == len);
+            assert(copy == base);
+        }
+    });
+
+    // Now let's clear the base and check that we're back to zero
+    base = string_t();
+    assert(accounting_allocator::current_bytes_alloced == 0);
+}
+
 static void test_updates() {
     // Compare STL and StringZilla strings append functionality.
     char const alphabet_chars[] = "abcdefghijklmnopqrstuvwxyz";
@@ -465,6 +581,8 @@ int main(int argc, char const **argv) {
 
     // The string class implementation
     test_constructors();
+    test_memory_stability_len(1024);
+    test_memory_stability_len(14);
     test_updates();
 
     // Advanced search operations