Skip to content

Commit f651b53

Browse files
committed
fix(patch): make Windows arg-parse patch deterministic (drop override)
The count-guard variant of 0001-win32-arg-parse-embed-guard.patch fixed 21/25 Windows Java tests but collided on the 4 server-integration setups (Rerank, ToolCalling, Multimodal, OpenAiCompatServer) whose argv length happened to equal java.exe's process arg count — the guard then wrongly adopted java.exe's command line and they kept failing with "Failed to parse model parameters". Those tests pass on Linux/macOS, so their args are valid; the collision was the only cause. Switch to the deterministic fix: keep the make_utf8_argv() call referenced (no -Wunused-function) but never adopt its result, so the caller's already-UTF-8 JNI argv is always used. A JNI library is never the process, so the GetCommandLineW override is pure liability for us. Verified the patch applies cleanly to b9739 and the applier stays idempotent. CLAUDE.md + TODO.md updated to record the count-guard -> removal change; upstream PR can still expose an opt-out that preserves the standalone tools' UTF-8 fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfvSZ76NW4e1qX1PjL4RKq
1 parent 0cffac1 commit f651b53

3 files changed

Lines changed: 32 additions & 18 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,7 @@ Current patches:
355355

356356
| Patch | Fixes |
357357
|-------|-------|
358-
| `0001-win32-arg-parse-embed-guard.patch` | Windows JNI regression from llama.cpp **#24779** (b9739): `common_params_parse` unconditionally replaced the caller's argv with the process command line (`GetCommandLineW`), so an embedded/JNI caller (`java.exe`) lost its `--model …` args → "Failed to parse model parameters". The patch guards the override to fire **only when the re-derived arg count equals `argc`** — true for the standalone `llama-*` tools (their UTF-8 CLI fix is preserved), false for a JVM host (our already-UTF-8 argv is kept). This is also the shape to PR upstream. |
358+
| `0001-win32-arg-parse-embed-guard.patch` | Windows JNI regression from llama.cpp **#24779** (b9739): `common_params_parse` unconditionally replaced the caller's argv with the process command line (`GetCommandLineW`), so an embedded/JNI caller (`java.exe`) lost its `--model …` args → "Failed to parse model parameters". The patch **drops the override for our build** (keeps the `make_utf8_argv()` call referenced so there's no `-Wunused-function`, but never adopts its result), so the caller's already-UTF-8 argv is always used. This is **deterministic** — an earlier count-guard variant (only override when the re-derived arg count equals `argc`) collided on the server-integration tests whose argv length happened to equal `java.exe`'s and kept them failing. The upstream PR can instead expose an opt-out / `common_params_parse_argv` that preserves the standalone tools' UTF-8 fix. |
359359

360360
## Upgrading/Downgrading llama.cpp Version
361361

TODO.md

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -139,11 +139,19 @@ proving Ninja Multi-Config + MSVC works on the same tree). The two builds produc
139139

140140
**Status: FIXED via local source patch (`patches/0001-win32-arg-parse-embed-guard.patch`).** Surfaced
141141
while bringing PR #248 green (the b9739 build fixes let the Windows Java jobs run to completion and
142-
exposed this). Resolved by **fix option 1 below** — the count-guard — applied through the generic
143-
`patches/` mechanism (see CLAUDE.md "Local llama.cpp source patches"), so it covers every C++ build
144-
and re-applies on each clean build. Still worth upstreaming (the guard, or a `common_params_parse_argv`
145-
companion) so the patch can eventually be dropped; until then it must be re-verified on each llama.cpp
146-
bump (the applier fails loud if it no longer applies).
142+
exposed this). Applied through the generic `patches/` mechanism (see CLAUDE.md "Local llama.cpp source
143+
patches"), so it covers every C++ build and re-applies on each clean build.
144+
145+
**Note on the fix shape (count-guard → deterministic removal).** The first patch used fix option 1
146+
below — the count-guard (override only when the re-derived arg count equals `argc`). It fixed 21/25
147+
Windows Java tests, but **collided** on the 4 server-integration setups (`OpenAiServerRerank*`,
148+
`OpenAiServerToolCalling*`, `MultimodalIntegrationTest`, `OpenAiCompatServerIntegrationTest`) whose
149+
argv length happened to equal `java.exe`'s, so they kept failing with the same parse error. The patch
150+
was changed to **fix option 2** (drop the override entirely for our build — a JNI library is never the
151+
process, so the override is pure liability), which is deterministic. Still worth upstreaming as an
152+
opt-out / `common_params_parse_argv` that preserves the standalone tools' UTF-8 fix, so the patch can
153+
eventually be dropped; until then it must be re-verified on each llama.cpp bump (the applier fails loud
154+
if it no longer applies).
147155

148156
**Symptom.** On **Windows x86_64 only**, every Java test that loads a real model fails in
149157
`LlamaModel.loadModel` (native) with `LlamaException: "Failed to parse model parameters"`
Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,25 @@
11
diff --git a/common/arg.cpp b/common/arg.cpp
22
--- a/common/arg.cpp
33
+++ b/common/arg.cpp
4-
@@ -924,7 +924,14 @@ bool common_params_parse(int argc, char ** argv, common_params & params, llama_e
4+
@@ -924,10 +924,17 @@ bool common_params_parse(int argc, char ** argv, common_params & params, llama_e
55
bool common_params_parse(int argc, char ** argv, common_params & params, llama_example ex, void(*print_usage)(int, char **)) {
66
#ifdef _WIN32
77
auto utf8 = make_utf8_argv();
88
- if (!utf8.ptrs.empty()) {
9-
+ // java-llama.cpp patch (PR #248): only adopt the process command line (GetCommandLineW) when
10-
+ // the caller actually passed THIS process's own argv -- i.e. the re-derived argument count
11-
+ // matches argc. For the standalone llama-* tools that is always true, so their UTF-8 CLI fix
12-
+ // (upstream llama.cpp #24779) is preserved. For an embedded JNI caller the process is java.exe
13-
+ // (many more args), so the counts differ and our already-UTF-8 argv (from GetStringUTFChars)
14-
+ // is kept instead of being silently discarded -- which otherwise makes common_params_parse_ex
15-
+ // parse java.exe's command line and fail with "Failed to parse model parameters".
16-
+ if (!utf8.ptrs.empty() && static_cast<int>(utf8.buf.size()) == argc) {
17-
argc = static_cast<int>(utf8.buf.size());
18-
argv = utf8.ptrs.data();
19-
}
9+
- argc = static_cast<int>(utf8.buf.size());
10+
- argv = utf8.ptrs.data();
11+
- }
12+
+ // java-llama.cpp patch (PR #248): upstream (llama.cpp #24779) replaced the caller's argv with
13+
+ // the process command line (GetCommandLineW) here to recover UTF-8 args for the standalone
14+
+ // llama-* tools. libjllama is a JNI library, never the process: it already passes correct
15+
+ // UTF-8 argv (GetStringUTFChars), and adopting GetCommandLineW discarded it -> common_params_parse
16+
+ // parsed java.exe's command line and failed with "Failed to parse model parameters". We keep the
17+
+ // make_utf8_argv() call (so it stays referenced -> -Wunused-function-clean) but do NOT adopt its
18+
+ // result, so the caller's already-UTF-8 argv is always used. This is deterministic: a count-guard
19+
+ // (only override when the re-derived arg count equals argc) collided on the server-integration
20+
+ // tests whose argv length happened to equal java.exe's, so they kept failing. The upstream PR
21+
+ // can instead expose an opt-out / a common_params_parse_argv that preserves the standalone fix.
22+
+ (void) utf8;
23+
#endif
24+
25+
auto ctx_arg = common_params_parser_init(params, ex, print_usage);

0 commit comments

Comments
 (0)