-
Notifications
You must be signed in to change notification settings - Fork 13.5k
cmake : add sanitizer flags for llama.cpp #11279
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.
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
Conversation
common/CMakeLists.txt
Outdated
| if (NOT MSVC) | ||
| if (LLAMA_SANITIZE_THREAD) | ||
| add_compile_options(-fsanitize=thread) | ||
| link_libraries (-fsanitize=thread) | ||
| endif() | ||
|
|
||
| if (LLAMA_SANITIZE_ADDRESS) | ||
| add_compile_options(-fsanitize=address -fno-omit-frame-pointer) | ||
| link_libraries (-fsanitize=address) | ||
| endif() | ||
|
|
||
| if (LLAMA_SANITIZE_UNDEFINED) | ||
| add_compile_options(-fsanitize=undefined) | ||
| link_libraries (-fsanitize=undefined) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is done in the top-level CMakeLists.txt it should automatically apply to all included subdirectories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved them to the llama_add_compile_flags() to be consistent with the existing LLAMA_FATAL_WARNINGS and LLAMA_ALL_WARNINGS options.
Moved a single call of llama_add_compile_flags() to the top-level CMakeLists.txt. It's OK for now, but in general I think that having separate calls in the sub-trees is better, because it allows for example to have 3rd-party code in the examples tree that would not be a subject to the llama.cpp compile flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sanitizer flags should still be applied to 3rd-party code regardless, but maybe not other flags like warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the sanitizer flags are now at the top level. Also removed the GGML_SANITIZE_* options as they are not actually useful, because all the code must be compiled with sanitizers.
| typedef std::pair<enum ggml_type, std::array<int64_t, GGML_MAX_DIMS>> tensor_config_t; | ||
|
|
||
| std::vector<tensor_config_t> get_tensor_configs(std::mt19937 & rng) { | ||
| static std::vector<tensor_config_t> get_tensor_configs(std::mt19937 & rng) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, compiler warnings normally pick this up, but not here it seems
|
The reason the tests are failing is another bug in |
|
@JohannesGaessler Thanks for taking a look. I've cherry-picked your commit into this branch. Let's see if the CI passes now. |
* cmake : add sanitizer flags for llama.cpp ggml-ci * tests : fix compile warnings ggml-ci * cmake : move sanitizer flags to llama_add_compile_flags ggml-ci * cmake : move llama.cpp compile flags to top level lists ggml-ci * cmake : apply only sanitizer flags at top level ggml-ci * tests : fix gguf context use in same_tensor_data * gguf-test: tensor data comparison * dummy : trigger ggml-ci * unicode : silence gcc warnings ggml-ci * ci : use sanitizer builds only in Debug mode ggml-ci * cmake : add status messages [no ci] --------- Co-authored-by: Johannes Gäßler <[email protected]>
* cmake : add sanitizer flags for llama.cpp ggml-ci * tests : fix compile warnings ggml-ci * cmake : move sanitizer flags to llama_add_compile_flags ggml-ci * cmake : move llama.cpp compile flags to top level lists ggml-ci * cmake : apply only sanitizer flags at top level ggml-ci * tests : fix gguf context use in same_tensor_data * gguf-test: tensor data comparison * dummy : trigger ggml-ci * unicode : silence gcc warnings ggml-ci * ci : use sanitizer builds only in Debug mode ggml-ci * cmake : add status messages [no ci] --------- Co-authored-by: Johannes Gäßler <[email protected]>
* cmake : add sanitizer flags for llama.cpp ggml-ci * tests : fix compile warnings ggml-ci * cmake : move sanitizer flags to llama_add_compile_flags ggml-ci * cmake : move llama.cpp compile flags to top level lists ggml-ci * cmake : apply only sanitizer flags at top level ggml-ci * tests : fix gguf context use in same_tensor_data * gguf-test: tensor data comparison * dummy : trigger ggml-ci * unicode : silence gcc warnings ggml-ci * ci : use sanitizer builds only in Debug mode ggml-ci * cmake : add status messages [no ci] --------- Co-authored-by: Johannes Gäßler <[email protected]>
* cmake : add sanitizer flags for llama.cpp ggml-ci * tests : fix compile warnings ggml-ci * cmake : move sanitizer flags to llama_add_compile_flags ggml-ci * cmake : move llama.cpp compile flags to top level lists ggml-ci * cmake : apply only sanitizer flags at top level ggml-ci * tests : fix gguf context use in same_tensor_data * gguf-test: tensor data comparison * dummy : trigger ggml-ci * unicode : silence gcc warnings ggml-ci * ci : use sanitizer builds only in Debug mode ggml-ci * cmake : add status messages [no ci] --------- Co-authored-by: Johannes Gäßler <[email protected]>
ref #11252 (comment)
Also, enable compile flags for
tests.