chore: update llama.cpp pin - #1012
Closed
ndizazzo wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThe patch series adds the Skippy C ABI and staged execution foundation, expands model-family and activation-sideband support, adds ordered part loading and sampling controls, introduces native and external MTP workflows, and updates runtime events, chat parsing, validation, and patch metadata. ChangesSkippy staged runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant SkippyABI
participant llama.cpp
participant MTP
Client->>SkippyABI: Open model and create session
SkippyABI->>llama.cpp: Load filtered model parts
Client->>SkippyABI: Submit sampled decode
SkippyABI->>llama.cpp: Execute staged graph
llama.cpp-->>SkippyABI: Predicted token and activation
SkippyABI->>MTP: Propose native draft tokens
MTP-->>SkippyABI: MTP draft
SkippyABI-->>Client: Return token, activation, and draft
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
third_party/llama.cpp/patches/0001-Add-Skippy-ABI-and-package-writer-foundation.patch (1)
1246-1261: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMissing null pointer check for
logits.If
session->stage_model->config.include_outputis false or ifllama_get_logits_ithfails to return logits,logitswill benullptr. Dereferencing it in the loop will cause a segmentation fault.🔒️ Proposed fix
static llama_token skippy_greedy_sample(skippy_session * session) { const llama_vocab * vocab = llama_model_get_vocab(session->stage_model->model); const int32_t n_vocab = llama_vocab_n_tokens(vocab); const float * logits = llama_get_logits_ith(session->ctx, -1); + if (logits == nullptr) { + return -1; + } llama_token best = 0; float best_logit = -std::numeric_limits<float>::infinity();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/llama.cpp/patches/0001-Add-Skippy-ABI-and-package-writer-foundation.patch` around lines 1246 - 1261, Update skippy_greedy_sample to handle a null result from llama_get_logits_ith before iterating over logits; return an appropriate safe fallback token, preserving the existing greedy selection when logits is available.third_party/llama.cpp/patches/0008-Add-external-decode-media-prefill-and-newer-family-s.patch (1)
893-911: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPrevent out-of-bounds memory access if
n_embd_inp < n_embd.If a model's architecture utilizes an
n_embd_inpsmaller thann_embd, thestd::memcpycalls will write past the logical stride ofembd_storagefor each token, culminating in an out-of-bounds write on the final token. Additionally, the missing bounds check in the finalelsebranch will causestatic_cast<size_t>(n_embd_inp - n_embd)to wrap around to a huge value, leading to a crash instd::memset.Limit the copy size to
std::min(n_embd, n_embd_inp)and guard thememsetoperation.🔒️ Proposed fix
if ((input_desc->flags & SKIPPY_ACTIVATION_FLAG_GEMMA3N_ALTUP) != 0) { const float * input = static_cast<const float *>(input_payload); + const size_t copy_dim = static_cast<size_t>(std::min(n_embd, n_embd_inp)); for (int32_t i = 0; i < n_tokens; ++i) { float * dst = embd_storage.data() + static_cast<size_t>(i)*n_embd_inp; - std::memcpy(dst, input + static_cast<size_t>(i)*n_embd, static_cast<size_t>(n_embd)*sizeof(float)); + std::memcpy(dst, input + static_cast<size_t>(i)*n_embd, copy_dim*sizeof(float)); if (n_embd_inp > n_embd) { std::memset(dst + n_embd, 0, static_cast<size_t>(n_embd_inp - n_embd)*sizeof(float)); } } } else if (n_embd_inp == n_embd) { std::memcpy(embd_storage.data(), input_payload, hidden_bytes); } else { const float * input = static_cast<const float *>(input_payload); + const size_t copy_dim = static_cast<size_t>(std::min(n_embd, n_embd_inp)); for (int32_t i = 0; i < n_tokens; ++i) { float * dst = embd_storage.data() + static_cast<size_t>(i)*n_embd_inp; - std::memcpy(dst, input + static_cast<size_t>(i)*n_embd, static_cast<size_t>(n_embd)*sizeof(float)); - std::memset(dst + n_embd, 0, static_cast<size_t>(n_embd_inp - n_embd)*sizeof(float)); + std::memcpy(dst, input + static_cast<size_t>(i)*n_embd, copy_dim*sizeof(float)); + if (n_embd_inp > n_embd) { + std::memset(dst + n_embd, 0, static_cast<size_t>(n_embd_inp - n_embd)*sizeof(float)); + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/llama.cpp/patches/0008-Add-external-decode-media-prefill-and-newer-family-s.patch` around lines 893 - 911, Update the embedding-copy branches in the input payload handling to copy only std::min(n_embd, n_embd_inp) elements per token, preventing writes beyond each embd_storage stride. Guard both zero-padding std::memset calls so they run only when n_embd_inp exceeds the copied embedding width, avoiding unsigned underflow when it is smaller.third_party/llama.cpp/patches/0003-Add-staged-sampling-checkpoints-and-part-loading.patch (1)
275-296: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftCritical use-after-free hazard on
ctx_ggufand unintended local variable mutation.This loop contains two severe issues resulting from how the part context is managed:
- Use-after-free: The
ctx_ggufsmart pointer takes ownership of the newly loaded GGUF context and destroys it at the end of theforloop iteration. However, a raw pointer (ctx_gguf.get()) is passed intollama_tensor_weightand stored inweights_map. Whenllama_model_loaderlater accesses this weight (e.g., to read tensor data offsets), it will dereference freed memory and crash.- Local variable overwrite: Passing
&ctxtopart_params.ctxoverwrites the localctxvariable (which initially holds the primary file'sggml_context) on every iteration. Whilecontexts.emplace_back(ctx)technically pushes the newly allocated context because of this side-effect, it leavesctxpointing to the last part's context after the loop and makes the code highly brittle.To fix both issues, transfer ownership of
ctx_ggufto a persistent container that outlives the constructor (such asmeta_splits, which upstreamllama_model_loaderuses for splits) and use an isolated local variable for the part context.🐛 Proposed fix
- struct gguf_init_params part_params = { - /*.no_alloc = */ true, - /*.ctx = */ &ctx, - }; - gguf_context_ptr ctx_gguf { gguf_init_from_file(fname_part, part_params) }; - if (!ctx_gguf) { - throw std::runtime_error(format("%s: failed to load GGUF part from %s", __func__, fname_part)); - } - - files.emplace_back(new llama_file(fname_part, "rb", use_direct_io)); - contexts.emplace_back(ctx); - - for (ggml_tensor * cur = ggml_get_first_tensor(ctx); cur; cur = ggml_get_next_tensor(ctx, cur)) { + struct ggml_context * part_meta = nullptr; + struct gguf_init_params part_params = { + /*.no_alloc = */ true, + /*.ctx = */ &part_meta, + }; + gguf_context_ptr ctx_gguf { gguf_init_from_file(fname_part, part_params) }; + if (!ctx_gguf) { + throw std::runtime_error(format("%s: failed to load GGUF part from %s", __func__, fname_part)); + } + + gguf_context * part_ctx = ctx_gguf.get(); + meta_splits.emplace_back(std::move(ctx_gguf)); // Keep GGUF context alive + + files.emplace_back(new llama_file(fname_part, "rb", use_direct_io)); + contexts.emplace_back(part_meta); + + for (ggml_tensor * cur = ggml_get_first_tensor(part_meta); cur; cur = ggml_get_next_tensor(part_meta, cur)) { std::string tensor_name = std::string(cur->name); if (weights_map.find(tensor_name) != weights_map.end()) { continue; } n_elements += ggml_nelements(cur); n_bytes += ggml_nbytes(cur); - weights_map.emplace(tensor_name, llama_tensor_weight(files.back().get(), part_idx, ctx_gguf.get(), cur)); + weights_map.emplace(tensor_name, llama_tensor_weight(files.back().get(), part_idx, part_ctx, cur)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/llama.cpp/patches/0003-Add-staged-sampling-checkpoints-and-part-loading.patch` around lines 275 - 296, Update the split-loading loop to preserve each part’s GGUF context beyond the iteration by transferring ctx_gguf ownership into the loader’s persistent meta_splits container before storing it in weights_map. Use a separate local ggml_context* for part_params.ctx instead of mutating the surrounding ctx variable, and store that isolated part context in contexts while keeping the primary context unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@third_party/llama.cpp/patches/0001-Add-Skippy-ABI-and-package-writer-foundation.patch`:
- Around line 1246-1261: Update skippy_greedy_sample to handle a null result
from llama_get_logits_ith before iterating over logits; return an appropriate
safe fallback token, preserving the existing greedy selection when logits is
available.
In
`@third_party/llama.cpp/patches/0003-Add-staged-sampling-checkpoints-and-part-loading.patch`:
- Around line 275-296: Update the split-loading loop to preserve each part’s
GGUF context beyond the iteration by transferring ctx_gguf ownership into the
loader’s persistent meta_splits container before storing it in weights_map. Use
a separate local ggml_context* for part_params.ctx instead of mutating the
surrounding ctx variable, and store that isolated part context in contexts while
keeping the primary context unchanged.
In
`@third_party/llama.cpp/patches/0008-Add-external-decode-media-prefill-and-newer-family-s.patch`:
- Around line 893-911: Update the embedding-copy branches in the input payload
handling to copy only std::min(n_embd, n_embd_inp) elements per token,
preventing writes beyond each embd_storage stride. Guard both zero-padding
std::memset calls so they run only when n_embd_inp exceeds the copied embedding
width, avoiding unsigned underflow when it is smaller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e8b8432-e268-43a6-a36a-eb80be5f16eb
📒 Files selected for processing (18)
Justfilethird_party/llama.cpp/patches/0001-Add-Skippy-ABI-and-package-writer-foundation.patchthird_party/llama.cpp/patches/0002-Add-early-staged-model-family-and-chat-support.patchthird_party/llama.cpp/patches/0003-Add-staged-sampling-checkpoints-and-part-loading.patchthird_party/llama.cpp/patches/0004-Add-lanes-external-media-and-chat-grammar-support.patchthird_party/llama.cpp/patches/0005-Add-resident-prefix-cache-and-session-refinements.patchthird_party/llama.cpp/patches/0006-Expand-staged-execution-across-dense-and-recurrent-f.patchthird_party/llama.cpp/patches/0007-Expand-staged-execution-across-VL-and-broad-model-fa.patchthird_party/llama.cpp/patches/0008-Add-external-decode-media-prefill-and-newer-family-s.patchthird_party/llama.cpp/patches/0009-Add-chat-grammar-device-enumeration-and-runtime-even.patchthird_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patchthird_party/llama.cpp/patches/0011-Pass-reasoning-format-through-stage-chat-templates.patchthird_party/llama.cpp/patches/0012-Wire-mmap-and-mlock-runtime-load-options.patchthird_party/llama.cpp/patches/0013-Add-external-MTP-draft-sidecar-attachment.patchthird_party/llama.cpp/patches/0014-Add-non-frame-native-MTP-decode-ABI.patchthird_party/llama.cpp/patches/0015-Fix-stage-activation-graph-input-allocation.patchthird_party/llama.cpp/patches/0016-Recognize-thinking-field-in-chat-auto-parser.patchthird_party/llama.cpp/upstream.txt
Collaborator
Author
|
#1014 raced me and was merged |
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updates the pinned llama.cpp revision and rebases the 16 downstream Skippy patches, including the MiniMax M2 upstream overlap. Also makes
just test-allinstall website dependencies in fresh worktrees.Validation:
just test-allSummary by CodeRabbit
New Features
Bug Fixes