llama-graph: fix UB warning from cross-attn mask loop indices - #18528
llama-graph: fix UB warning from cross-attn mask loop indices#18528nabbi wants to merge 1 commit into
Conversation
|
It's not quite that simple, |
CISC
left a comment
CISC
left a comment
There was a problem hiding this comment.
So, why not fix the others, like f.ex. llm_graph_input_mem_hybrid::set_input with the same issue?
| for (int h = 0; h < 1; ++h) { | ||
| for (int i = 0; i < n_tokens; ++i) { | ||
| for (int j = 0; j < n_enc; ++j) { | ||
| for (int64_t h = 0; h < 1; ++h) { |
There was a problem hiding this comment.
| for (int64_t h = 0; h < 1; ++h) { | |
| for (int h = 0; h < 1; ++h) { |
There was a problem hiding this comment.
I find this a very strange "loop" BTW. :)
There was a problem hiding this comment.
right :) it runs once
The value is also used for multiplication, so I'm guessing the loop is kept for historical reasons.
There was a problem hiding this comment.
It emphasizes that we construct the mask for a single attention head - there was a comment about this, but it got removed at some point. It's ok to keep the loop as it is.
|
Within that function, n_tokens is redefined as an int64_t constant. I think that dead loop came from previous refactoring efforts where there was separate data and data_sma loops. Yet I got lost in the reasoning with all those edits :) |
Sure, I was just curious why you only fixed one of a multitude of functions that would produce this warning?
Probably. |
|
Interestingly, that compilation warning only tripped on that dead loop line. And not the first loop. So I was hyper focused on just this one function... Good conversation :) |
Well, |
b68e1d7 to
cdd47ab
Compare
|
more holistic approach. targeted n_tokens and adjacent code for correctness. I hadn't analyzed the entire repo for other integer type mismatches |
|
|
||
| for (int i = n_tokens; i < n_tokens; ++i) { | ||
| for (int j = 0; j < n_enc; ++j) { | ||
| data[h*(n_enc*n_tokens) + i*n_enc + j] = -INFINITY; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is the only meaningful change.
The rest of the changes in the PR seem redundant? Or am I missing something?
There was a problem hiding this comment.
Well, at least the n_tokens loops are "possible" overflows.
There was a problem hiding this comment.
I'm more than happy to shirk the PR down to just the compilation warning only of type mismatches need a more focused effort.
We are throwing -Wformat= against LLAMA_LOG_DEBUG for the loop counters for at least in print_mask; that that needs correcting yet. Those are bound by a limit of 20 where as most other loops are bound to whatever the upper limit of n_tokens is set as.
Normalize loop index types across batch, graph, and KV code paths to match the width of their bounds (e.g. n_tokens, n_rs, n_seq_id, n_expert_used). This also removes an unreachable loop with identical start/end conditions GCC emited the following warning when building with optimizations: llama-graph.cpp:473:9: warning: iteration 2147483645 invokes undefined behavior [-Waggressive-loop-optimizations] Signed-off-by: Nic Boet <nic@boet.cc>
cdd47ab to
4f310f4
Compare
3 participants
I observed this behavior when building ollama on Gentoo.
Confirmed llm_graph_input_attn_cross::set_input functions are aligned between these two projects so submitting the PR here.
GCC emits the following warning when building with optimizations:
llama-graph.cpp:473:9: warning: iteration 2147483645 invokes undefined
behavior [-Waggressive-loop-optimizations]
The warning is caused by using int loop induction variables against int64_t bounds (n_tokens, n_enc), which allows signed overflow in the induction variable and enables undefined behavior.
This change widens the loop counters to int64_t to match the bounds and removes an unreachable loop of the form:
for (i = n_tokens; i < n_tokens; ++i)
which could never execute.
Thank you