From 80e06a4bcf4df9017179d9ac046e7424b9ed3e68 Mon Sep 17 00:00:00 2001 From: dusterbloom <32869278+dusterbloom@users.noreply.github.com> Date: Thu, 21 May 2026 14:00:13 +0200 Subject: [PATCH 1/2] refactor(common): extract gguf_mmap RAII wrapper as standalone Per @howard0su's review on #237: gguf_mmap.h/.cpp are platform abstraction (POSIX mmap / Windows MapViewOfFile) that will be reused by multiple loaders. Extracting into a standalone PR ahead of the loader refactor (target/draft/MTP heads all benefit). Includes the cubic P1 fixes from f6e8e94: - open() is now idempotent (releases prior mapping before re-opening, leaves object in default state on any failure path) - Windows release() now calls CloseHandle() before clearing handle_ New: test_gguf_mmap unit test covers open/close, idempotency, missing-file path, RAII destructor. Stacked on #241 (uses dflash::common namespace from the start). --- dflash/CMakeLists.txt | 5 + dflash/src/common/gguf_mmap.h | 219 +++++++++++++++++++++++++++++++++ dflash/test/test_gguf_mmap.cpp | 162 ++++++++++++++++++++++++ 3 files changed, 386 insertions(+) create mode 100644 dflash/src/common/gguf_mmap.h create mode 100644 dflash/test/test_gguf_mmap.cpp diff --git a/dflash/CMakeLists.txt b/dflash/CMakeLists.txt index 2bdbb2199..de0b5f2dd 100644 --- a/dflash/CMakeLists.txt +++ b/dflash/CMakeLists.txt @@ -521,6 +521,11 @@ if(DFLASH27B_TESTS) target_include_directories(test_kv_quant PRIVATE ${DFLASH27B_SRC_INCLUDE_DIRS}) target_link_libraries(test_kv_quant PRIVATE dflash_common) endif() + if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/test/test_gguf_mmap.cpp") + add_executable(test_gguf_mmap test/test_gguf_mmap.cpp) + target_include_directories(test_gguf_mmap PRIVATE ${DFLASH27B_SRC_INCLUDE_DIRS}) + target_link_libraries(test_gguf_mmap PRIVATE dflash_common) + endif() if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/test/test_draft_vs_reference.cpp") add_executable(test_draft_vs_reference test/test_draft_vs_reference.cpp) target_link_libraries(test_draft_vs_reference PRIVATE dflash_common) diff --git a/dflash/src/common/gguf_mmap.h b/dflash/src/common/gguf_mmap.h new file mode 100644 index 000000000..37416f31c --- /dev/null +++ b/dflash/src/common/gguf_mmap.h @@ -0,0 +1,219 @@ +// common/gguf_mmap.h — RAII wrapper for platform-conditional mmap of GGUF files. +// +// Encapsulates POSIX mmap / Windows MapViewOfFile behind a single interface. +// Loaders that materialize tensor data from disk must use this class instead +// of inlining equivalent platform-conditional code. +// +// Include convention: #include "common/gguf_mmap.h" +// Never: ../common/gguf_mmap.h or absolute paths. + +#pragma once + +#include +#include + +namespace dflash::common { + +class GgufMmap { +public: + GgufMmap() = default; + ~GgufMmap(); + + // Non-copyable. + GgufMmap(const GgufMmap &) = delete; + GgufMmap & operator=(const GgufMmap &) = delete; + + // Movable — transfers ownership, leaves source empty. + GgufMmap(GgufMmap &&) noexcept; + GgufMmap & operator=(GgufMmap &&) noexcept; + + // Open the file at path and mmap it read-only. + // Returns true on success. On failure, writes a human-readable description + // to out_error and leaves this object in the default (empty) state. + bool open(const std::string & path, std::string & out_error); + + const void * data() const; // nullptr when not open + size_t size() const; // 0 when not open + bool is_open() const; + + // Transfer ownership of the mmap'd region to the caller. + // After release() this object is empty (is_open() == false). + // The caller is responsible for unmapping on POSIX or UnmapViewOfFile on + // Windows, and for closing the fd on POSIX. + struct OwnedRegion { + const void * data; + size_t size; + int fd; // POSIX fd; -1 on Windows (handle already closed) + }; + OwnedRegion release(); + +private: + const void * data_ = nullptr; + size_t size_ = 0; +#if defined(_WIN32) + void * handle_ = nullptr; // HANDLE (Windows mapping object, reinterpret_cast'd) +#else + int fd_ = -1; +#endif +}; + +} // namespace dflash::common + +// ── Implementation ──────────────────────────────────────────────────────── +// Header-only: the platform-conditional code lives here rather than in a .cpp +// so that loaders can include a single file without adding a new translation unit. + +#include + +#if defined(_WIN32) +#if !defined(NOMINMAX) +#define NOMINMAX +#endif +#if !defined(WIN32_LEAN_AND_MEAN) +#define WIN32_LEAN_AND_MEAN +#endif +#include +#else +#include +#include +#include +#include +#include +#endif + +namespace dflash::common { + +inline GgufMmap::~GgufMmap() { + if (!data_) return; +#if defined(_WIN32) + UnmapViewOfFile(const_cast(data_)); + if (handle_) CloseHandle(reinterpret_cast(handle_)); +#else + ::munmap(const_cast(data_), size_); + if (fd_ >= 0) ::close(fd_); +#endif + data_ = nullptr; + size_ = 0; +} + +inline GgufMmap::GgufMmap(GgufMmap && o) noexcept + : data_(o.data_), size_(o.size_) +#if defined(_WIN32) + , handle_(o.handle_) +#else + , fd_(o.fd_) +#endif +{ + o.data_ = nullptr; + o.size_ = 0; +#if defined(_WIN32) + o.handle_ = nullptr; +#else + o.fd_ = -1; +#endif +} + +inline GgufMmap & GgufMmap::operator=(GgufMmap && o) noexcept { + if (this != &o) { + this->~GgufMmap(); + new (this) GgufMmap(std::move(o)); + } + return *this; +} + +inline bool GgufMmap::open(const std::string & path, std::string & out_error) { + // Idempotency: if already open, release prior mapping before re-opening. + // This prevents leaking the prior fd/mapping and ensures that, on failure, + // the object is left in the default empty state (not half-overwritten). + if (data_) { this->~GgufMmap(); new (this) GgufMmap(); } +#if defined(_WIN32) + const int wlen = MultiByteToWideChar(CP_UTF8, 0, path.c_str(), -1, nullptr, 0); + if (wlen <= 0) { + out_error = "GgufMmap: MultiByteToWideChar failed for " + path; + return false; + } + std::wstring wpath; + wpath.resize(wlen - 1); + MultiByteToWideChar(CP_UTF8, 0, path.c_str(), -1, wpath.data(), wlen); + + HANDLE hFile = CreateFileW(wpath.c_str(), GENERIC_READ, FILE_SHARE_READ, + nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); + if (hFile == INVALID_HANDLE_VALUE) { + out_error = "GgufMmap: CreateFileW failed for " + path; + return false; + } + LARGE_INTEGER li; + if (!GetFileSizeEx(hFile, &li)) { + out_error = "GgufMmap: GetFileSizeEx failed for " + path; + CloseHandle(hFile); + return false; + } + size_t file_size = static_cast(li.QuadPart); + + HANDLE hMapping = CreateFileMappingA(hFile, nullptr, PAGE_READONLY, 0, 0, nullptr); + CloseHandle(hFile); + if (!hMapping) { + out_error = "GgufMmap: CreateFileMappingA failed for " + path; + return false; + } + void * addr = MapViewOfFile(hMapping, FILE_MAP_READ, 0, 0, 0); + if (!addr) { + out_error = "GgufMmap: MapViewOfFile failed for " + path; + CloseHandle(hMapping); + return false; + } + data_ = addr; + size_ = file_size; + handle_ = reinterpret_cast(hMapping); + return true; +#else + int fd = ::open(path.c_str(), O_RDONLY); + if (fd < 0) { + out_error = std::string("GgufMmap: open failed for ") + path + ": " + strerror(errno); + return false; + } + struct stat st{}; + if (::fstat(fd, &st) < 0) { + out_error = std::string("GgufMmap: fstat failed: ") + strerror(errno); + ::close(fd); + return false; + } + size_t file_size = static_cast(st.st_size); + void * addr = ::mmap(nullptr, file_size, PROT_READ, MAP_PRIVATE, fd, 0); + if (addr == MAP_FAILED) { + out_error = std::string("GgufMmap: mmap failed: ") + strerror(errno); + ::close(fd); + return false; + } + data_ = addr; + size_ = file_size; + fd_ = fd; + return true; +#endif +} + +inline const void * GgufMmap::data() const { return data_; } +inline size_t GgufMmap::size() const { return size_; } +inline bool GgufMmap::is_open() const { return data_ != nullptr; } + +inline GgufMmap::OwnedRegion GgufMmap::release() { + OwnedRegion r{}; + r.data = data_; + r.size = size_; +#if defined(_WIN32) + // Close the mapping handle now. Per MSDN, closing the handle does not + // unmap the view — the view remains valid until UnmapViewOfFile is called. + // OwnedRegion has no handle field; caller unmaps via UnmapViewOfFile(data). + if (handle_) CloseHandle(reinterpret_cast(handle_)); + r.fd = -1; + handle_ = nullptr; +#else + r.fd = fd_; + fd_ = -1; +#endif + data_ = nullptr; + size_ = 0; + return r; +} + +} // namespace dflash::common diff --git a/dflash/test/test_gguf_mmap.cpp b/dflash/test/test_gguf_mmap.cpp new file mode 100644 index 000000000..283618b41 --- /dev/null +++ b/dflash/test/test_gguf_mmap.cpp @@ -0,0 +1,162 @@ +// Unit tests for dflash::common::GgufMmap (RAII platform mmap wrapper). +// Plain int main(), no test frameworks. +// +// T1: open + read first few bytes of a known file → ok, size > 0 +// T2: open the same instance twice (idempotency) → no leak +// T3: open a non-existent path → returns false, object stays empty +// T4: explicit release() → object becomes empty, no crash +// T5: RAII destructor — scope exit, no crash + +#include "common/gguf_mmap.h" + +#include +#include +#include +#include +#include + +#if defined(_WIN32) +#include +#define MKSTEMP_FN(t) (_mktemp_s(t, sizeof(t)), _open(t, _O_CREAT | _O_WRONLY | _O_BINARY, 0600)) +#define CLOSE_FN _close +#define WRITE_FN _write +#else +#include +#define MKSTEMP_FN(t) mkstemp(t) +#define CLOSE_FN close +#define WRITE_FN write +#endif + +// Create a small temp file with known content; returns its path. +static std::string make_temp_file() { +#if defined(_WIN32) + char tmpl[] = "gguf_mmap_test_XXXXXX"; + int fd = MKSTEMP_FN(tmpl); +#else + char tmpl[] = "/tmp/gguf_mmap_test_XXXXXX"; + int fd = mkstemp(tmpl); +#endif + assert(fd >= 0); + const char payload[] = "GGUF_TEST_PAYLOAD_1234"; + WRITE_FN(fd, payload, static_cast(sizeof(payload) - 1)); + CLOSE_FN(fd); + return std::string(tmpl); +} + +// ─── T1: open + basic read ─────────────────────────────────────────────────── + +static void t1_open_and_read() { + std::string path = make_temp_file(); + dflash::common::GgufMmap m; + std::string err; + + assert(m.open(path, err)); + assert(m.is_open()); + assert(m.data() != nullptr); + assert(m.size() > 0); + + // The first bytes should match the payload written above. + const char expected[] = "GGUF_TEST_PAYLOAD"; + assert(m.size() >= sizeof(expected) - 1); + assert(std::memcmp(m.data(), expected, sizeof(expected) - 1) == 0); + + std::remove(path.c_str()); + std::puts("T1 PASS"); +} + +// ─── T2: idempotent re-open ────────────────────────────────────────────────── + +static void t2_idempotent_open() { + std::string path1 = make_temp_file(); + std::string path2 = make_temp_file(); + + dflash::common::GgufMmap m; + std::string err; + + assert(m.open(path1, err)); + assert(m.is_open()); + size_t size1 = m.size(); + + // Second open on the same object must release the first mapping cleanly. + assert(m.open(path2, err)); + assert(m.is_open()); + assert(m.size() == size1); // both temp files have same payload length + assert(err.empty() || true); // no error expected + + std::remove(path1.c_str()); + std::remove(path2.c_str()); + std::puts("T2 PASS"); +} + +// ─── T3: missing file → returns false, object stays empty ─────────────────── + +static void t3_missing_file() { + dflash::common::GgufMmap m; + std::string err; + + bool ok = m.open("/tmp/gguf_mmap_does_not_exist_xyz987.bin", err); + assert(!ok); + assert(!m.is_open()); + assert(m.data() == nullptr); + assert(m.size() == 0); + assert(!err.empty()); + + std::puts("T3 PASS"); +} + +// ─── T4: explicit release() → object becomes empty ────────────────────────── + +static void t4_explicit_release() { + std::string path = make_temp_file(); + dflash::common::GgufMmap m; + std::string err; + + assert(m.open(path, err)); + assert(m.is_open()); + + auto region = m.release(); + assert(region.data != nullptr); + assert(region.size > 0); + assert(!m.is_open()); + assert(m.data() == nullptr); + assert(m.size() == 0); + + // Caller must clean up the released region. +#if defined(_WIN32) + UnmapViewOfFile(const_cast(region.data)); +#else + ::munmap(const_cast(region.data), region.size); + if (region.fd >= 0) ::close(region.fd); +#endif + + std::remove(path.c_str()); + std::puts("T4 PASS"); +} + +// ─── T5: RAII destructor — scope exit, no crash ───────────────────────────── + +static void t5_raii_destructor() { + std::string path = make_temp_file(); + { + dflash::common::GgufMmap m; + std::string err; + assert(m.open(path, err)); + assert(m.is_open()); + // ~GgufMmap() fires here, must not crash or leak. + } + std::remove(path.c_str()); + std::puts("T5 PASS"); +} + +// ─── main ──────────────────────────────────────────────────────────────────── + +int main() { + t1_open_and_read(); + t2_idempotent_open(); + t3_missing_file(); + t4_explicit_release(); + t5_raii_destructor(); + + std::puts("ALL TESTS PASS"); + return 0; +} From 65865826644191d9fe22b6f2e8d7f034d1d57682 Mon Sep 17 00:00:00 2001 From: dusterbloom <32869278+dusterbloom@users.noreply.github.com> Date: Thu, 21 May 2026 21:43:40 +0200 Subject: [PATCH 2/2] fix(test): replace tautological assertion in test_gguf_mmap T2 Per cubic P3 on PR #243: T2's success check was tautological and didn't verify that the error string is empty after a successful open(). Replaced with assert(err.empty()). --- dflash/test/test_gguf_mmap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dflash/test/test_gguf_mmap.cpp b/dflash/test/test_gguf_mmap.cpp index 283618b41..f5d077d84 100644 --- a/dflash/test/test_gguf_mmap.cpp +++ b/dflash/test/test_gguf_mmap.cpp @@ -81,7 +81,7 @@ static void t2_idempotent_open() { assert(m.open(path2, err)); assert(m.is_open()); assert(m.size() == size1); // both temp files have same payload length - assert(err.empty() || true); // no error expected + assert(err.empty()); // no error expected after successful open std::remove(path1.c_str()); std::remove(path2.c_str());