Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect format strings and uninitialized variables. #4133

Merged
merged 3 commits into from
Nov 23, 2023
Merged

Fix incorrect format strings and uninitialized variables. #4133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c6874f9
Fix incorrect format strings and uninitialized variables.
haohui Nov 19, 2023
566785f
Address comments
haohui Nov 21, 2023
9216e7b
Add the missing include statement
haohui Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions examples/server/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1095,6 +1095,7 @@ struct llama_server_context
std::lock_guard<std::mutex> lock(mutex_results);
task_result res;
res.id = id;
res.stop = false;
res.error = true;
res.result_json = { { "content", error } };
queue_results.push_back(res);
Expand Down Expand Up @@ -1255,6 +1256,7 @@ struct llama_server_context
std::lock_guard<std::mutex> lock(mutex_tasks);
task_server task;
task.id = id_gen++;
task.target_id = 0;
task.data = data;
task.infill_mode = infill;
task.embedding_mode = embedding;
Expand Down
2 changes: 1 addition & 1 deletion ggml-cuda.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8057,7 +8057,7 @@ bool ggml_cuda_compute_forward(struct ggml_compute_params * params, struct ggml_
if (tensor->op == GGML_OP_MUL_MAT) {
if (tensor->src[0]->ne[3] != tensor->src[1]->ne[3]) {
#ifndef NDEBUG
fprintf(stderr, "%s: cannot compute %s: src0->ne[3] = %d, src1->ne[3] = %d - fallback to CPU\n", __func__, tensor->name, tensor->src[0]->ne[3], tensor->src[1]->ne[3]);
fprintf(stderr, "%s: cannot compute %s: src0->ne[3] = %ld, src1->ne[3] = %ld - fallback to CPU\n", __func__, tensor->name, tensor->src[0]->ne[3], tensor->src[1]->ne[3]);
cebtenzzre marked this conversation as resolved.
Show resolved Hide resolved
#endif
return false;
}
Expand Down
4 changes: 3 additions & 1 deletion ggml.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1586,7 +1586,7 @@ inline static void ggml_vec_argmax_f32(const int n, int * s, const float * x) {
// data types
//

static const char * GGML_OP_NAME[GGML_OP_COUNT] = {
static const char * GGML_OP_NAME[GGML_OP_COUNT + 2] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a warning about this when building with CMake and gcc 11.3. This seems like the wrong fix - if we are actually missing operators then OP_COUNT should be adjusted and the actual names should be added to the list. What is the warning that you are seeing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see it with gcc 13:

In function ‘ggml_op_name’,
    inlined from ‘ggml_get_n_tasks’ at /home/jared/src/forks/llama.cpp/ggml.c:15698:17:
/home/jared/src/forks/llama.cpp/ggml.c:2019:24: warning: array subscript 69 is above array bounds of ‘const char *[68]’ [-Warray-bounds=]
 2019 |     return GGML_OP_NAME[op];
      |            ~~~~~~~~~~~~^~~~
/home/jared/src/forks/llama.cpp/ggml.c: In function ‘ggml_get_n_tasks’:
/home/jared/src/forks/llama.cpp/ggml.c:1589:21: note: while referencing ‘GGML_OP_NAME’
 1589 | static const char * GGML_OP_NAME[GGML_OP_COUNT] = {
      |                     ^~~~~~~~~~~~

The problem is that we are calling ggml_op_name in ggml_get_n_tasks when the op is not recognized:

llama.cpp/ggml.c

Lines 15696 to 15698 in e937066

default:
{
printf("%s: op %s not implemented\n", __func__, ggml_op_name(node->op));

We should either not attempt to print the name, or only attempt to print it if 'op' is less than GGML_OP_COUNT.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only attempt to print it if 'op' is less than GGML_OP_COUNT.

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code looks correct to me. It's kind of hard to make compiler happy in this case :-(

I'll take it out for now

cebtenzzre marked this conversation as resolved.
Show resolved Hide resolved
"NONE",

"DUP",
Expand Down Expand Up @@ -1664,6 +1664,8 @@ static const char * GGML_OP_NAME[GGML_OP_COUNT] = {

"CROSS_ENTROPY_LOSS",
"CROSS_ENTROPY_LOSS_BACK",
"",
"",
};

static_assert(GGML_OP_COUNT == 68, "GGML_OP_COUNT != 68");
Expand Down
Loading