From 6637d259712ab0d490d60287ae686861c6604d7e Mon Sep 17 00:00:00 2001 From: Reithan Date: Fri, 4 Sep 2026 01:03:42 -0700 Subject: [PATCH] fix two bugs in grammar memoization for reject_candidates_for_stack (#2431) TOKEN/TOKEN_NOT early return: operator[] pre-inserts an empty candidates vector at line 1123, setting cache_target. The TOKEN/TOKEN_NOT branch returns without writing to *cache_target, leaving an empty vector in the map. Subsequent lookups return that empty vector, reporting "reject nothing" for what should be a non-empty reject set. Fix: write to cache_target before the early return. Pointer aliasing in candidate key: candidate lists were hashed as raw struct bytes, which includes the code_points pointer value. glibc can reuse heap addresses across calls, so the same pointer value can point to different content, causing false cache hits. Fix: hash actual content (index, id, partial_utf8 fields, and the decoded code_points sequence) rather than raw bytes including the pointer. --- src/llama-grammar.cpp | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/llama-grammar.cpp b/src/llama-grammar.cpp index bb620ae72..bb683e749 100644 --- a/src/llama-grammar.cpp +++ b/src/llama-grammar.cpp @@ -1114,9 +1114,24 @@ llama_grammar_candidates llama_grammar_reject_candidates_for_stack( if (candidates_hash_size < hash_cutoff) { // Only check stash hash first - these are usually ~24b, and almost always under 64b if (auto cache_hit = memo_cache.find(stack_hash); cache_hit != memo_cache.end()) { - auto & candidates_memos = cache_hit->second; - auto candidates_hash_start = reinterpret_cast(candidates.data()); - auto candidates_hash = std::hash{}({ candidates_hash_start, candidates_hash_size }); + auto & candidates_memos = cache_hit->second; + // Hash candidate content (index, id, partial_utf8, decoded code_points sequence) + // rather than raw struct bytes, which include the code_points pointer value. + // Pointer values can be reused by the allocator across calls pointing to different + // content, causing false cache hits that return stale reject sets. + size_t candidates_hash = candidates.size(); + auto combine = [&](size_t v) { + candidates_hash ^= v + 0x9e3779b9 + (candidates_hash << 6) + (candidates_hash >> 2); + }; + for (const auto & c : candidates) { + combine(std::hash{}(c.index)); + combine(std::hash{}(c.id)); + combine(std::hash{}(c.partial_utf8.value)); + combine(std::hash{}(c.partial_utf8.n_remain)); + for (const uint32_t * cp = c.code_points; *cp != 0; ++cp) { + combine(std::hash{}(*cp)); + } + } if (auto cache_hit2 = candidates_memos.find(candidates_hash); cache_hit2 != candidates_memos.end()) { return cache_hit2->second; } else { @@ -1142,6 +1157,11 @@ llama_grammar_candidates llama_grammar_reject_candidates_for_stack( rejects.push_back(tok); } } + // cache_target was set by operator[] which pre-inserted an empty vector; write the + // result here so subsequent lookups don't return an empty reject set. + if (cache_target) { + *cache_target = rejects; + } return rejects; }