Repository navigation
hrx: fix the pin's build - arch_name() is a std::string, not a C string (engine #681) - #220
Conversation
f148781 (engine ggml-org#681) turned the ctx_other refusal into a warning and passed `model.arch_name()` to LLAMA_LOG_WARN's printf-style format. `llama_model::arch_name()` returns `std::string` (src/llama-model.h:742), so the variadic call passes a non-POD object where the `%s` expects `char *`: src/llama-context.cpp:160:42: error: cannot pass non-trivial object of type 'std::string' to variadic function; expected type from format string was 'char *' [-Wnon-pod-varargs] FAILED: src/CMakeFiles/llama.dir/llama-context.cpp.o That is a hard build failure on the branch tip (clang 24 / gfx1151 build of test-backend-ops and llama-server), not a warning: nothing on 1bit/hrx-vulkan-patched compiles without this line. `.c_str()` is valid here because the temporary lives for the whole full-expression, which is the duration of the log call. Found while building the suite for engine ggml-org#1259 (ask 1: re-run the full suite at pin 710c7c2) - the run could not start until the pin built.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bd0762ca.
The pin really does not compile, and this really fixes it. Built both sides on this box with the toolchain the error
came from (AMD clang version 24.0.0git, gfx1151):
BEFORE — 1bit/hrx-vulkan-patched tip ced49c61
FAILED: src/CMakeFiles/llama.dir/llama-context.cpp.o rc 1
src/llama-context.cpp:160:42: error: cannot pass non-trivial object of type 'std::string'
(aka 'basic_string<char>') to variadic function; expected type from format string was 'char *' [-Wnon-pod-varargs]
AFTER — this head bd0762ca
src/CMakeFiles/llama.dir/llama-context.cpp.o rc 0, 0 errors
ninja llama rc 0, libllama.so + .so.0 linked
So the effect as stated is right: nothing that links llama builds from source at the tip, and one .c_str() is the
whole difference. The lifetime claim holds too — the temporary std::string lives to the end of the full-expression, which
is the whole log call, so the pointer cannot dangle.
Two things I checked beyond the diff. First, that the fix survives the two commits it is behind: cherry-picked onto
today's tip (ced49c61, which still carries the broken line) the object compiles rc 0, so the merge is not a rebase
gamble. Second, that there is no second instance of the same mistake, which would make the pin fail again five lines
later — git grep arch_name() over tracked sources gives the other five uses as string concatenation, an add_kv value and
one already-correct .c_str() (llama-model.cpp:1831), so this is the only varargs misuse of it.
Worth noting for the queue rather than as a nit: the branch tip moved twice while I was reviewing it (710c7c23 ->
ced49c61, the #219 and #221 merges), and every one of those keeps the broken line, so this stays the gate on any source
build of the pin until it lands.
Root cause
f1487819e(engine ggml-org#681, merged in #219) turned thectx_otherrefusal inllama_contextinto a warning:llama_model::arch_name()returnsstd::string(src/llama-model.h:742), so this passes a non-POD object to a printf-style variadic call where%sexpectschar *. On this toolchain (clang 24, gfx1151 build) that is a hard error, not a warning:Effect
The tip of
1bit/hrx-vulkan-patched(710c7c2) does not compile. No target that linksllama-llama-server,test-backend-ops,llama-cli- builds from source at the pin. This blocked engine ggml-org#1259's first ask (re-run the fulltest-backend-opssuite at the pin) until I applied this line locally.Fix
model.arch_name().c_str(). The temporarystd::stringlives to the end of the full-expression, i.e. for the whole log call, so the pointer cannot dangle.Verifying
ninja test-backend-ops->FAILED: src/CMakeFiles/llama.dir/llama-context.cpp.o(error above), stopping at 13-189/391.llama-context.cppand relinks;test-backend-opsthen runs the full suite (that run's numbers go to engine cuBLAS: refactor and optimize f16 mat mul performance ggml-org/llama.cpp#1259).No behaviour change: same message, same trigger, same once-per-load path.