Skip to content

Commit 38c11b4

Browse files
committed
docs(TODO): record b9739 Windows JNI common_params_parse(GetCommandLineW) regression
Root-cause write-up only (fix deferred per discussion). On Windows x86_64, b9739's common_params_parse has a _WIN32 prologue that replaces the caller-supplied argv with GetCommandLineW() (the host process command line). Correct for the standalone llama-server.exe, but under JNI the process is java.exe (no --model on its command line), so the parse fails with "Failed to parse model parameters" for every model-loading Java test. Affects both the MSVC and Ninja DLLs; Linux/macOS unaffected. Documents symptom, exact upstream code (arg.cpp:924-931), why our argv is platform-neutral, and three fix options to choose from later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfvSZ76NW4e1qX1PjL4RKq
1 parent aaba886 commit 38c11b4

1 file changed

Lines changed: 50 additions & 0 deletions

File tree

TODO.md

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,56 @@ proving Ninja Multi-Config + MSVC works on the same tree). The two builds produc
135135
properties behave exactly as under the VS generator; `/MT` runtime and x64-vs-x86 gating unchanged.
136136
- The arch (`x64`/`x86`) comes from `ilammy/msvc-dev-cmd@v1`, not a `-A` flag (Ninja takes no `-A`).
137137

138+
### Known regression (b9739) — Windows JNI: `common_params_parse` ignores caller argv
139+
140+
**Status: root-caused, fix deferred.** Surfaced while bringing PR #248 green (the b9739 build fixes let
141+
the Windows Java jobs run to completion and exposed this).
142+
143+
**Symptom.** On **Windows x86_64 only**, every Java test that loads a real model fails in
144+
`LlamaModel.loadModel` (native) with `LlamaException: "Failed to parse model parameters"`
145+
(25 errors in `Java Tests Windows 2025 x86_64`, both the VS *and* Ninja DLLs). macOS and Linux Java
146+
tests pass. The argv we build is platform-neutral (`--model models/<file>.gguf`, relative, forward
147+
slashes — `TestConstants.MODEL_PATH`), so it is **not** the Windows-Ninja build, **not** our argv,
148+
and **not** a path/escaping issue.
149+
150+
**Root cause (upstream llama.cpp, new in b9739).** `jllama.cpp` (`load_model_impl`, ~line 606) builds
151+
a CLI argv from `ModelParameters` and calls upstream
152+
`common_params_parse(argc, argv, params, LLAMA_EXAMPLE_SERVER)`. In b9739, `common/arg.cpp`'s
153+
`common_params_parse` gained a **Windows-only** prologue (arg.cpp:924-931):
154+
155+
```cpp
156+
bool common_params_parse(int argc, char ** argv, ...) {
157+
#ifdef _WIN32
158+
auto utf8 = make_utf8_argv(); // = CommandLineToArgvW(GetCommandLineW())
159+
if (!utf8.ptrs.empty()) { // always non-empty under a JVM
160+
argc = (int) utf8.buf.size();
161+
argv = utf8.ptrs.data(); // DISCARDS the caller-supplied argv
162+
}
163+
#endif
164+
... common_params_parse_ex(argc, argv, ctx_arg) ...
165+
}
166+
```
167+
168+
It unconditionally replaces the caller's argv with the host **process** command line
169+
(`GetCommandLineW()`). For the standalone `llama-server.exe` this is correct (fixes UTF-8 CLI args).
170+
For an **embedded/JNI** caller the process is **`java.exe`**, whose command line has no `--model`, so
171+
`common_params_parse_ex` fails and `common_params_parse` returns `false` → our "Failed to parse model
172+
parameters". `common_params_parse_ex` is `static`, so we cannot bypass the block by calling the inner
173+
parser. Our JNI already passes correct UTF-8 argv (`GetStringUTFChars`), so the re-derivation is
174+
unnecessary for us. **This is an upstream bug affecting every embedded Windows consumer of
175+
`common_params_parse`.**
176+
177+
**Fix options (decide later):**
178+
1. FetchContent `PATCH_COMMAND` that **guards** the block — only override when `make_utf8_argv()` arg
179+
count equals the caller's `argc` (true for the standalone exe, false for JNI). Minimal + semantically
180+
correct; also the shape to PR upstream.
181+
2. FetchContent patch that **removes** the `_WIN32` override for our build.
182+
3. File an upstream issue/PR and wait for a fixed llama.cpp build.
183+
184+
Any patch re-applies on every llama.cpp bump — add it to the upgrade checklist. Pre-existing on `main`
185+
since #247 (b9682→b9739); independent of the Windows-Ninja classifier work. Windows **build + C++ ctest**
186+
jobs are green; only the Windows **Java/inference** jobs are affected.
187+
138188
### llama.cpp upstream feature exposure (queued, deferred by policy)
139189
140190
These are JNI plumbing items for upstream API additions. Policy: add only after a real user request — they are mostly relevant to specific model families or specialized workflows.

0 commit comments

Comments
 (0)