llama.cpp icon indicating copy to clipboard operation
llama.cpp copied to clipboard

alloca is not standard c

Open Rhialto opened this issue 1 year ago • 4 comments

Prerequisites

Please answer the following questions for yourself before submitting an issue.

  • [x] I am running the latest code. Development is very rapid so there are no tagged versions as of now.
  • [x] I carefully followed the README.md.
  • [x] I searched using keywords relevant to my issue to make sure that I am creating a new issue that is not already open (or closed).
  • [x] I reviewed the Discussions, and have a new bug or useful enhancement to share.

Expected Behavior

The code should link without warnings

Current Behavior

The linker warned about alloca, that this is not standard and won't work without the proper compiler flags (see first comment for the exact text)

Environment and Context

  • Operating System

NetBSD 9.3

Failure Information (for bugs)

Fix: enable GNU extensions. That these are needed is also indicated by #define _GNU_SOURCE at the top of ggml.c, although that flag is probably mis-used (these flags typically remove features to comply with standards, not add them).

diff --git a/Makefile b/Makefile
index 3e58a28..cd4e522 100644
--- a/Makefile
+++ b/Makefile
@@ -31,8 +31,8 @@ endif
 #
 
 # keep standard at C11 and C++11
-CFLAGS   = -I.              -O3 -DNDEBUG -std=c11   -fPIC
-CXXFLAGS = -I. -I./examples -O3 -DNDEBUG -std=c++11 -fPIC
+CFLAGS   = -I.              -O3 -DNDEBUG -std=gnu11   -fPIC
+CXXFLAGS = -I. -I./examples -O3 -DNDEBUG -std=gnu++11 -fPIC
 LDFLAGS  =
 

Rhialto avatar Apr 10 '23 19:04 Rhialto

Rebuilding with the original flags:

llama.cpp$ gmake
I llama.cpp build info:
I UNAME_S:  NetBSD
I UNAME_P:  x86_64
I UNAME_M:  amd64
I CFLAGS:   -I.              -O3 -DNDEBUG -std=c11   -fPIC -Wall -Wextra -Wpedan
tic -Wcast-qual -Wdouble-promotion -Wshadow -Wstrict-prototypes -Wpointer-arith 
-Wno-unused-function -pthread
I CXXFLAGS: -I. -I./examples -O3 -DNDEBUG -std=c++11 -fPIC -Wall -Wextra -Wpedan
tic -Wcast-qual -Wno-unused-function -Wno-multichar -pthread
I LDFLAGS:
I CC:       cc (nb4 20200810) 7.5.0
I CXX:      g++ (nb4 20200810) 7.5.0

...

====  Run ./main -h for help.  ====

g++ -I. -I./examples -O3 -DNDEBUG -std=c++11 -fPIC -Wall -Wextra -Wpedantic -Wcast-qual -Wno-unused-function -Wno-multichar -pthread examples/quantize/quantize.cpp ggml.o llama.o -o quantize
ld: ggml.o: in function `ggml_graph_compute':
ggml.c:(.text+0x12fd7): warning: Warning: reference to the libc supplied alloca(3); this most likely will not work. Please use the compiler provided version of alloca(3), by supplying the appropriate compiler flags (e.g. not -std=c89).

...

Rhialto avatar Apr 10 '23 19:04 Rhialto

_GNU_SOURCE does in fact add functions. If you try removing it, asprintf will indeed not be defined. The comment claims it's also needed for CLOCK_MONOTONIC… which on my machine is not true, because -pthread defines _REENTRANT, which /usr/include/features.h interprets as equivalent to #define _POSIX_C_SOURCE 199506L, which enables CLOCK_MONOTONIC. But the comment is specific enough that I expect it was added after an actual build failure, so there's probably some other Linux configuration that requires it (maybe using a non-glibc C library?).

(That said, the comment is outdated because ggml.c no longer uses asprintf; I'll fix that.)

Using -std=gnu++11 would also enable arbitrary GNU language extensions, which is probably not desired. If NetBSD is breaking alloca without it then I don't think it's making the right choice. If there's a workaround then it should only be enabled for NetBSD.

comex avatar Apr 11 '23 02:04 comex

I don't think one can say that NetBSD is deliberately breaking alloca. It's more that NetBSD is often a bit more pedantic about not allowing non-standard things, where Linux is often more lenient. The "compiler support" that's required for proper support for alloca is probably not much more than something like #define alloca __builtin_alloca, but since that invades into the user namespace, it's not done by default. It would be nice if there were an option to say "we want no extensions, except alloca" but I don't think there is one. Since alloca has always been somewhat dubious anyway, the best solution would be to avoid using it. But I haven't looked at that yet, if it's easy or not.

Rhialto avatar Apr 11 '23 17:04 Rhialto

I looked at how much alloca is actually used, and it isn't even that much. In one file, there were #includes for it but it wasn't even used (I suppose it was in the past).

One real case was also very simple: the function has just a single (implicit) return at the end.

The other case involved multiple return statements. I added a free() before each of them. But do double-check that I didn't miss any :)

Merge Request: https://github.com/ggerganov/llama.cpp/pull/918

Edit: Another potential solution could be some Variable Length Arrays (VLAs), but if I remember correctly, compilers are not required to implement them in more recent C standard versions. So, for the purpose of not using non- or less-standard things, they are not so great.

Rhialto avatar Apr 12 '23 16:04 Rhialto

This issue was closed because it has been inactive for 14 days since being marked as stale.

github-actions[bot] avatar Apr 11 '24 01:04 github-actions[bot]