Skip to content

Commit 9c5e17e

Browse files
Bernard Ladenthinclaude
andcommitted
Fix the Windows AMX heap corruption: ggml_aligned_free, not free()
The intermittently red Windows Java test job is one line of upstream ggml. ggml_backend_amx_buffer_type_alloc_buffer() allocates with ggml_aligned_malloc() and ggml_backend_amx_buffer_free_buffer() released that pointer with plain free(). On Windows ggml_aligned_malloc is _aligned_malloc and ggml_aligned_free is _aligned_free (ggml/src/ggml.c), so free() on that pointer is undefined behaviour and corrupts the process heap. Of the six callers of ggml_aligned_malloc in the tree this was the only mismatched pair -- ggml-backend.cpp, ggml-cpu.c twice, ggml-openvino.cpp, ggml-zdnn.cpp and ggml.c itself all pair correctly, and ggml-backend.cpp:2464 is the line patches/0018 copies. Proven rather than reasoned, on a rented Granite Rapids machine running the DLLs CI itself built, with one CPU backend module staged at a time and the same model each run: module runs exit sapphirerapids 6 -1073740940 (0xC0000374 STATUS_HEAP_CORRUPTION) cooperlake 2 0 icelake 2 0 haswell 2 0 Only sapphirerapids carries the AMX path. It crashes at one thread too, which rules out a data race, and it crashes AFTER the inference prints its timings -- the signature of a heap corrupted earlier and detected at free. The same six runs on Linux with the same CPU (amx_tile, amx_int8 present) all exit 0 with byte-identical output text, because ggml_aligned_malloc maps to posix_memalign there and that memory may legitimately go to free(). That is also why it survived so long: the AMX buffer type only exists on Sapphire Rapids and newer, GitHub mixes runner CPUs, and downstream it showed up only as "Tests run: 0" on whichever runs happened to land on AMX hardware -- the same commit passed 1916 tests on Ice Lake-SP and on a Zen 3 workstation. The guard is a configure-time check, not a test, and that is deliberate: llama/cmake/check-aligned-free-pairing.cmake fails the build on EVERY platform when a ggml source calls ggml_aligned_malloc and never ggml_aligned_free. A run-time test cannot replace it -- without an AMX CPU there is nothing to observe, which is exactly how this shipped. Verified red against the unpatched source (naming amx.cpp) and green with the patch applied; the full local build reports "aligned-free pairing: 6 ggml source(s) use ggml_aligned_malloc, all paired" and BUILD_EXIT=0. Upstream-submittable as it stands and not yet filed; on a bump, grep free(buffer->context) in ggml/src/ggml-cpu/amx/amx.cpp at the new tag and drop the patch rather than refreshing it if upstream fixed it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2h8gXyyE5UeimkL9vQv9G
1 parent 2efaf08 commit 9c5e17e

6 files changed

Lines changed: 172 additions & 22 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,20 @@ from version 5.0.0 onward. Pre-fork releases (`1.x`–`4.2.0`) were authored by
99

1010
## [Unreleased]
1111

12+
### Fixed
13+
- **Windows crashed with `STATUS_HEAP_CORRUPTION` on CPUs with AMX (Sapphire Rapids and newer).**
14+
Every model load that ran on an AMX-capable CPU killed the process at cleanup -- after the
15+
inference itself had succeeded -- because ggml's AMX buffer type allocated with
16+
`ggml_aligned_malloc()` and released with plain `free()`. On Windows that is `_aligned_malloc`
17+
against `free()`, which is undefined behaviour and corrupts the heap (`patches/0018`; of the six
18+
callers of `ggml_aligned_malloc` upstream this was the only mismatched pair). Linux was never
19+
affected, because `ggml_aligned_malloc` maps to `posix_memalign` there. Measured on a Granite
20+
Rapids machine with one CPU backend module staged at a time: the `sapphirerapids` module crashed
21+
6 of 6 runs, `cooperlake`, `icelake` and `haswell` none, and the crash reproduces at a single
22+
thread. A configure-time check (`llama/cmake/check-aligned-free-pairing.cmake`) now fails the build
23+
on every platform if a ggml source allocates aligned and frees plain, because a machine without
24+
AMX cannot observe the defect at run time.
25+
1226
### Added
1327
- **Modular natives: the GPU backends are additive module jars, and every platform a GPU can join is
1428
built modular.** A GPU natives jar (`cuda13-linux-x86-64`, `vulkan-windows-aarch64`, ... -- 15 of

‎CLAUDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1288,6 +1288,7 @@ Current patches:
12881288
| `0014-common-log-callback-sink.patch` | **Gives `common_log` a callback sink: `common_log_set_callback(log, cb, user_data)` (`common/log.{h,cpp}`).** This is what `LlamaModel.setLogger` hooks. Before it, the Java logger was a `llama_log_set()` callback, which has two holes, both found while chasing `slot print_timing` lines interleaving with the Atmosphere agent's streamed answer: **(a)** every model load runs `common_init()`, which re-points `llama_log_set()` at `common_log_default_callback` (`common.cpp:394`), so `setLogger(…)` *before* `new LlamaModel(…)` silently lost the callback; **(b)** the server's own `SRV_*`/`SLT_*` macros are `LOG_INF` and write straight into `common_log`, which `llama_log_set()` never carried, so the per-request `slot …`/`srv …` lines could not be routed to Java at all (the reason `LlamaModelTest#testLogText/JSON` sat `@Disabled` for years). `common_log` upstream offers file, colors, prefix, timestamps, verbosity and JSONL but no hook. The patch adds one: while a callback is set, the worker thread hands every entry to it **instead of** printing to stdout/stderr (a `--log-file` still receives them); the callback gets the bare formatted message (no prefix/timestamp/colors) with the `ggml_log_callback` signature; swapping the callback pauses the worker first, so queued entries reach the *previous* sink (which is what makes `setLogger(format, null)` a synchronous drain). With the sink, `common_init()`'s `llama_log_set()` reset is harmless — it points at the default callback that feeds `common_log`, i.e. exactly the path into the sink — so the ordering problem (a) disappears without any re-install logic, and the `srv`/`slot` lines (b) arrive because they are `common_log` entries. `jllama.cpp`'s `setLogger` therefore sets `common_log_set_callback(common_log_main(), trampoline)` **plus** `llama_log_set(common_log_default_callback)` (so llama/ggml lines feed `common_log` even before the first load). Two consequences to know: the callback runs on `common_log`'s **worker thread**, a plain `std::thread` llama.cpp re-creates on every pause/resume and never attaches to the JVM — the trampoline attaches it **once per worker thread** as a daemon, named `jllama-log-worker`, and a `thread_local` guard detaches it when the thread exits (`get_jni_env_attaching`; `common_log::pause()` joins the worker, a normal exit on which TLS destructors run, and `JNI_OnUnload` pauses it before anything else). Until 5.2.x it attached and detached per line, which created a `java.lang.Thread` object per log line (13 lines of a failed load, 13 `Thread` objects -- `LlamaLoggerTest` pins the single attach now). Each call runs inside a `PushLocalFrame`/`PopLocalFrame` pair, because a thread that stays attached and has no Java frame would otherwise accumulate local references, `setLogger` must call `common_log_set_callback` **outside** `g_log_mutex`, because the pause joins the worker, which needs that mutex to read the callback, and `setLogger` callers are serialized by a **separate** `g_set_logger_mutex`: two unserialized swaps race on the worker's `std::thread` (one joins it while the other assigns a fresh thread over the still-joinable object = `std::terminate`, the whole JVM), which `LlamaLoggerTest#concurrentSetLoggerCallsDoNotRaceOnTheLogWorker` reproduced before the mutex existed. Two caveats the Javadoc carries: a caller must not hold a lock the *previous* callback needs (the drain runs it on the worker while the caller waits), and the verbosity threshold is process-wide and reset by **every** load (`common_params_parse` ends with `common_log_set_verbosity_thold(params.verbosity)`, default 3), so a load without `-lv` puts it back to 3 — a review assumed the opposite, and `LlamaLoggerTest#verbosityThresholdIsProcessWideAndEveryLoadSetsIt` now pins the measured behaviour. And the verbosity threshold applies *before* the sink: at the default (`3`) the Java logger sees errors, warnings and the server's INFO lines, while llama/ggml INFO lines (`common_log_get_verbosity` maps them to TRACE = 4) arrive only from `setLogVerbosity(4)` on — the same filtering the console gets, and a behaviour change for consumers who captured the unfiltered `llama_log_set()` stream before. **Runnable guards:** `src/test/cpp/test_common_log_callback.cpp` (6 tests over a private `common_log_init()` instance: delivery, bare text under prefix+timestamps, clear, swap-drains-to-old-sink, file kept, levels pass through) links the function on every platform, so a bump that drops the patch reds `C++ Tests` at link time; `LlamaLoggerTest` (model-free, needs only `libjllama`: a logger set before a deliberately failing load on a non-GGUF file sees the `srv … loading model` INFO line and llama's ERROR line, in TEXT and JSON) and the re-enabled `LlamaModelTest#testLogText/testLogJSON` plus `#testLoggerSetBeforeLoadSurvivesTheLoad` (vocab-only load) cover the Java side. Upstream-submittable ("common : add a callback sink to common_log for embedding hosts"); **not yet filed upstream**. Touches only `common/log.{h,cpp}`, which no other patch touches. **Refreshed at the b11401 bump** (#29895 adds a `colors` member next to the sink's members and a `get_colors()` next to `set_callback()`; only context moved, the `+`/`-` lines are unchanged, and upstream still has no hook -- it added `common_log_get_colors` only). **On a bump, check whether upstream added a hook of its own (grep `callback` in `common/log.h`) and, if so, DROP this patch and port `setLogger` to theirs rather than refreshing it.** |
12891289
| `0015-rpc-embeddable-client-and-stoppable-server.patch` | **Makes ggml-rpc usable inside a JVM** (see "RPC backend" below). Upstream treats every client-side RPC problem as `GGML_ABORT` — a malformed endpoint, a server that is not running, a failed handshake — which in a JVM kills the application over a typo in `--rpc`. **(1)** The *registration* path reports failure instead: `rpc_dispatcher::start()` returns `false`, `try_get_dispatcher()` `nullptr`, `ggml_backend_rpc_get_device_count()` `0`, `ggml_backend_rpc_add_server()` `nullptr`, and `add_rpc_devices()` (`common/arg.cpp`) throws `std::invalid_argument` naming the server where it used to register `nullptr` and silently run without it. Every path *after* registration keeps the original contract through `get_dispatcher()`, which still aborts — a server that vanishes mid-inference stays fatal (TODO). **(2)** `add_server()`'s cache hit is re-checked (a stopped server used to pass registration and abort on the first tensor upload), and `ggml_backend_rpc_get_device_memory()` of a gone server returns 0/0 instead of aborting — RPC devices stay in ggml's process-wide registry forever, and `common_init()` queries the memory of **every** registered device at `-lv 4`, so a stopped server would otherwise take down an unrelated later load. **(3)** The server becomes stoppable: `ggml_backend_rpc_stop_server()` (disconnects the client being served, wakes `accept()` with a loopback connection — the one portable way) and `ggml_backend_rpc_server_listening()`; the loop now reaches its cleanup and frees its backends on every early return, but deliberately **not** `rpc_transport_shutdown()`, whose `WSACleanup()` would invalidate every other RPC socket in the process. **(4)** transport: `MSG_NOSIGNAL`/`SO_NOSIGPIPE`, `socket_t::shutdown()`, and the fds `connect()`/`create_server()`/`accept()` leaked on their error paths are closed. Touches `ggml/include/ggml-rpc.h`, `ggml/src/ggml-rpc/{ggml-rpc.cpp,transport.cpp,transport.h}` and one function in `common/arg.cpp` (`add_rpc_devices`, away from `0001`'s `common_params_parse` hunks). **Runnable guard: `src/test/cpp/test_rpc.cpp`** — links both new functions (a bump that drops the patch fails `C++ Tests` at link time on every platform) and drives registration, stop and the memory query over loopback. Upstream-submittable; **not yet filed upstream**. On a bump, check whether upstream grew its own stop function or non-aborting registration (grep `stop_server` / `GGML_ABORT("Failed to connect` in `ggml-rpc.cpp`). **Refreshed at the b11450 bump** (#26610, `-sm tensor` over RPC, ~800 lines in `ggml-rpc.cpp`, protocol 7 → 8): three hunks lost their context -- new dispatcher members beside `start()`, and new `get_proc_address` names before `return NULL` (the two new names now sit after `ggml_backend_comm_allreduce_tensor`); the `+`/`-` lines are unchanged. The server-to-server comm #26610 adds (a listener on `0.0.0.0`, an `accept()` a stop does not wake) is on file in `TODO.md`. |
12901290
| `0017-ggml-cpu-x86-prefetch-byte-offset-and-msvc-type.patch` | **Makes `GGML_CPU_ALL_VARIANTS=ON` buildable with plain clang on Windows, and fixes a prefetch address that is wrong everywhere.** The four `_mm_prefetch` calls in `ggml/src/ggml-cpu/arch/x86/quants.c` (the SSSE3-without-AVX branch of `ggml_vec_dot_q4_0_q8_0`, and the only `_mm_prefetch` calls in the whole ggml tree) carry two independent defects; both are fixed by routing the address through one new `static inline` helper in the new header `ggml/src/ggml-cpu/arch/x86/prefetch.h`, which is included relatively, so **no CMake change is needed**. **(1) Type -- fatal only on a current clang:** MSVC declares the intrinsic as `void _mm_prefetch(char const *, int)`; clang's own `xmmintrin.h` defines a casting macro but only `#ifndef _MSC_VER`, so on a `*-windows-msvc` target clang leaves the declaration to Microsoft and a typed pointer is `-Wincompatible-pointer-types`. **clang 22 promoted that to an error by default** ([llvm-project #157364](https://github.com/llvm/llvm-project/pull/157364); the [22.1.0 release notes](https://releases.llvm.org/22.1.0/tools/clang/docs/ReleaseNotes.html) add that `-Wno-error=incompatible-pointer-types` downgrades it) -- *not* clang 16, which promoted only `-Wincompatible-FUNCTION-pointer-types`; an earlier version of this row said otherwise. Measured on the real file at `-msse4.2`, unpatched → patched: clang **20.1.8** 4 warnings → 0/0, clang **22.1.3** 4 errors → 0/0, clang **23.1.3** 4 errors → 0/0. Plain clang is exactly what `cmake/x64-windows-llvm.cmake` selects -- and plain clang is the only way to get all **14** CPU variants, because `clang-cl` makes CMake's `MSVC` true and the `if (NOT MSVC)` block in `ggml/src/CMakeLists.txt` then drops `zen4`, `cooperlake`, `sapphirerapids`, `ivybridge` and `piledriver`. **(2) Arithmetic:** `&x[ib] + sizeof(block_q4_0)` adds `sizeof()` *elements*, not bytes -- +324 and +1156 instead of +18 and +34 (the sizes `ggml-common.h` pins by `static_assert`), and +648 instead of +36 for the `2 *` pair. That half is wrong on every platform, including where the call compiles. **Measured:** no throughput difference (see the test-table row), so this is a correctness and portability fix, not a performance one. **Upstream-submittable and meant to be submitted.** **Why upstream's own build is green is measured, not an open question:** their `windows-cpu` release job builds exactly this with plain clang, `GGML_CPU_ALL_VARIANTS=ON` and no suppressing flag (`CMAKE_ARGS` carries only build-target toggles), and ships `ggml-cpu-sse42.dll` -- because the `windows-2025-vs2026` image has **LLVM 20.1.8** on `PATH`, where these are warnings. That is narrower than it looks: the same image also ships **clang 22.1.3** inside Visual Studio 2026 (`VC/Tools/Llvm/x64`), which *fails* on the unpatched file -- upstream reaches the working compiler only because `C:\Program Files/LLVM/bin` comes first on `PATH`. It is **not** a header difference: all three MSVC toolsets on that image (14.29.30133, 14.44.35207, 14.51.36231) declare it identically as `extern void _mm_prefetch(char const*_A, int _Sel);` -- that was the first hypothesis, and a one-off probe workflow (deleted again once it had answered) disproved it by printing the declaration from each toolset on the image. So this patch is **ahead of** upstream rather than diverging: their build breaks the moment the image ships a clang that makes the diagnostic fatal, and until then they compile four warnings and a prefetch address 18x/34x too far. **Runnable guard: `src/test/cpp/test_prefetch.cpp`.** On a bump, run `git grep -n _mm_prefetch ggml/src/ggml-cpu/arch/x86/quants.c` at the new tag: if the call sites already cast **before** the addition, **drop this patch rather than refreshing it**. |
1291+
| `0018-ggml-amx-aligned-free.patch` | **The AMX buffer was allocated with `ggml_aligned_malloc()` and released with plain `free()`, which corrupts the Windows heap.** `ggml_backend_amx_buffer_free_buffer()` (`ggml/src/ggml-cpu/amx/amx.cpp`) called `free(buffer->context)` on a pointer from `ggml_aligned_malloc()` -- which on Windows is `_aligned_malloc`, whose memory **must** go to `_aligned_free` (`ggml_aligned_free`, `ggml/src/ggml.c`); anything else is undefined behaviour. Of the six callers of `ggml_aligned_malloc` in the tree this was the only mismatched pair (`ggml-backend.cpp`, `ggml-cpu.c` twice, `ggml-openvino.cpp`, `ggml-zdnn.cpp` and `ggml.c` all pair correctly), and `ggml-backend.cpp:2464` is the line the fix copies. **Why it stayed hidden:** the AMX buffer type only exists on a CPU with `AMX_TILE`/`AMX_INT8`, i.e. Sapphire Rapids and newer, so only the `sapphirerapids` module reaches it -- and only on Windows, because `ggml_aligned_malloc` maps to `posix_memalign` on Linux and that memory may legitimately be freed with `free()`. **Measured** on a rented Granite Rapids machine with one CPU module staged at a time, same model, the DLLs CI itself built: `sapphirerapids` crashed **6 of 6** runs with `0xC0000374 STATUS_HEAP_CORRUPTION` -- including at **one** thread, which rules out a data race -- while `cooperlake`, `icelake` and `haswell` were clean 6 of 6; on Linux with the same CPU all six exit 0 with byte-identical output. The crash lands *after* the inference prints its timings, in cleanup, which is the signature of a heap corrupted earlier and detected at `free`. Downstream it showed up only as `Tests run: 0` on whichever CI runs happened to land on AMX hardware, which is why it survived: GitHub mixes runner CPUs. **Runnable guard: `llama/cmake/check-aligned-free-pairing.cmake`**, which fails the configure on **every** platform when a ggml source calls `ggml_aligned_malloc` and never `ggml_aligned_free` -- verified red against the unpatched source (naming `amx.cpp`) and green with this patch. A run-time test cannot replace it: without an AMX CPU there is nothing to observe. **Upstream-submittable; not yet filed.** On a bump, `git grep -n 'free(buffer->context)' ggml/src/ggml-cpu/amx/amx.cpp` at the new tag -- if upstream fixed it, **drop this patch** rather than refreshing it. |
12911292
| `0016-model-kolibri1.patch` | **Adds Aleph Alpha's Kolibri-1 (architecture `kolibri1`) ahead of upstream** -- a 78B German/English reasoning MoE (3.46B active) that upstream does not support yet ([ggml-org/llama.cpp#29922](https://github.com/ggml-org/llama.cpp/issues/29922), no PR at the time). A **temporary carry, not a fix**: drop it the moment upstream registers the architecture (`git grep -n kolibri src/llama-arch.cpp` at the new tag), and keep `test_kolibri1.cpp` -- its numerical comparisons must then pass against upstream's implementation, while its GGUF-format rows follow whatever format upstream's converter fixes (see the test-table row). **Architecture** (Aleph Alpha's vLLM reference, `aleph_alpha_inference/kolibri1.py` at `049a6a7`): GQA with per-head q/k RMSNorm; `layer_types` interleaves sliding-window layers (NEOX RoPE) with full-attention layers that have **no** positional encoding; sandwich norms around attention and MoE; every layer routed MoE + one ungated shared expert; the router selects top-k on **`logits + expert_bias`** and weights by the **unbiased `sigmoid(logits)`** -- not DeepSeek-V3's sigmoid router (selection on `sigmoid(logits) + bias`), which picks other experts as soon as the bias is non-zero. The graph therefore builds the selection itself and passes it to `build_moe_ffn` (`selected_experts_in`), leaving upstream's shared MoE code untouched. **Derived from two community ports, which are incompatible with each other's GGUFs** -- Eliasfpv28's AFMoE-based one (base b11378, `expert_gating_func` 2, pre-tokenizer `qwen2`) and the Qwen3-MoE-based one behind Hob-forge's GGUFs (base b11381, as carried in [mjwsolo/localcode#101](https://github.com/mjwsolo/localcode/pull/101): gating 5, pre-tokenizer `kolibri1`, rejects gating 2). This patch **loads both dialects**: gating 2, 5 or absent (all mean the one router), `kolibri1` mapped to the Qwen2 pre-tokenizer in `llama-vocab.cpp`, `expert_weights_norm` / `expert_shared_count` / `output.weight` optional. Every source, with license, is listed in the patch header; `REUSE.toml` annotates the file `MIT AND Apache-2.0` (Eliasfpv28's additions are Apache-2.0). Touches `src/llama-arch.{h,cpp}`, `src/llama-model.cpp` (mapping + NEOX rope list), `src/llama-vocab.cpp` (one name), `src/models/models.h` and the new `src/models/kolibri1.cpp`; no converter. **Runnable guard: `src/test/cpp/test_kolibri1.cpp`** (writes tiny random GGUFs in both dialects and compares every logit with an independent double-precision reference). **Not verified here:** the real 78B model (HuggingFace is unreachable from the sessions this was written in, and the smallest GGUF is 28.6 GB) and GPU backends -- the community ports report real-model checks (top-1 67/67 against a float64 reference for Q8_0, tool calls, German text). |
12921293

12931294
**Dropped patches** (`0004`, `0005`, `0009`, `0010`, `0011`, `0013` -- upstream fixed each defect

0 commit comments

Comments
 (0)