Enable -std= for cmake builds, fix warnings

[sw]

The Makefile has -std=c11/-std=c++11, but those were missing in CMakeLists.txt. Add them and fix the warnings.

[sw]

Hmm that doesn't trigger the CI - should I have made a branch on this repo instead of my own?

[cornelk]

github actions are having issues: https://www.githubstatus.com/

[anzz1]

github actions are having issues: https://www.githubstatus.com/

Yeah github has been a mess yesterday and today. Getting unicorns all over the place.

[niclimcy]

Isn’t this a thing:

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_C_STANDARD 11)

or is current cmake version too low for that

[sw]

Isn’t this a thing: set(CMAKE_CXX_STANDARD 11) set(CMAKE_C_STANDARD 11)

You're right, and CMAKE_C[XX]_STANDARD_REQUIRED is already set anyway. They're all supported since cmake 3.1.

Updated it, and now it looks like CI is working again.

[slaren]

Looks like the recent changes broke this PR and needs to rebased.

[Green-Sky]

this is kinda bad, we already have it defined per target
see https://github.com/ggerganov/llama.cpp/blob/3525899277d2e2bdc8ec3f0e6e40c47251608700/CMakeLists.txt#L235
and https://github.com/ggerganov/llama.cpp/blob/3525899277d2e2bdc8ec3f0e6e40c47251608700/CMakeLists.txt#L246

(and since they are public, they get inherited)

[sw]

The point is that -std=c11 and -std=c++11 were not passed to the compiler before, but now they are. What cmake is doing to amuse itself is not really relevant ;-)

[Green-Sky]

That just seems wrong 🤔 can you point me to the logs? I can't find said warnings.

[sw]

@Green-Sky :

That just seems wrong thinking can you point me to the logs? I can't find said warnings.

The warnings were about writing to the buffer returned by the data() method on std::string, which is UB, as I discussed with @anzz1 in the collapsed thread above. But that has been fixed in the meantime in another commit. The changes to ggml.c fix some other warnings, but they are a red herring here.

You never saw these warnings in the CI logs or when running cmake locally, precisely because the -std=c++11 wasn't passed to the compiler. You only got warnings with make.

[niclimcy]

this is kinda bad, we already have it defined per target see

https://github.com/ggerganov/llama.cpp/blob/3525899277d2e2bdc8ec3f0e6e40c47251608700/CMakeLists.txt#L235

and
https://github.com/ggerganov/llama.cpp/blob/3525899277d2e2bdc8ec3f0e6e40c47251608700/CMakeLists.txt#L246

(and since they are public, they get inherited)

This looks wrong following cmake convention ngl. Should we remove it since cmake has the ability to differentiate c++ and c by itself?

[sw]

I think it comes down to this:

https://cmake.org/cmake/help/latest/command/target_compile_features.html

If the use of the feature requires an additional compiler flag, such as -std=gnu++11, the flag will be added automatically.

Use of standard C++11 does not require -std=c++11. But we want that flag to be passed to the compiler so that it will warn us when we don't write standard C++11. Dito for C11.

You could argue that target_compile_features is now unnecessary, but I'm not 100% sure on this.