Skip to content

Add ci sanitizers #234

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

armintoepfer
Copy link

No description provided.

@armintoepfer
Copy link
Author

Let's see if that works as intended :)

@armintoepfer
Copy link
Author

@Martinsos You need to approve that I can run CI

@Martinsos
Copy link
Owner

@armintoepfer thanks for the PR! Would you mind updating this PR with the latest master, since it fixes the issue with CMake failing due to too old minimal version of it in the CMakefile.txt?

Also, just to clarify, since my C/C++ knowledge is quite rusty: this is an additional build that runs sanitizers for address and for udefined behaviours (UB). You also added lundef=true which checks for missing symbols. Is there anything else we should be adding, that is also a good practice to check for, or is this it?
I had LLM suggest adding -Db_pie=false -Db_lto=false -Db_ndebug=false for stabler sanitization checks (less false positives, ensures assertions are kept) -> does that make any sense, should we add those also maybe?

Thanks! Looking forward to merging this.

Copy link
Owner

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Some small suggestions.

Comment on lines +4 to +5
# Additional arguments for meson setup
MESON_SETUP_ARGS ?=
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
# Additional arguments for meson setup
MESON_SETUP_ARGS ?=
MESON_SETUP_ADDITIONAL_ARGS ?=

@@ -21,7 +23,8 @@ configure:
rm -rf ${BUILD_DIR}
meson setup ${BUILD_DIR} . \
--backend=ninja \
-Ddefault_library=${LIBRARY_TYPE}
-Ddefault_library=${LIBRARY_TYPE} \
${MESON_SETUP_ARGS}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
${MESON_SETUP_ARGS}
${MESON_SETUP_ADDITIONAL_ARGS}

@@ -66,6 +66,7 @@ jobs:
run: |
make CXXFLAGS="-Werror" LIBRARY_TYPE=static BUILD_DIR=meson-build-static
make CXXFLAGS="-Werror" LIBRARY_TYPE=shared BUILD_DIR=meson-build-shared
make CXXFLAGS="-Werror" LIBRARY_TYPE=static MESON_SETUP_ARGS="--buildtype debugoptimized -Db_sanitize=address,undefined -Db_lundef=true" BUILD_DIR=meson-build-static-debopt-ubasan
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
make CXXFLAGS="-Werror" LIBRARY_TYPE=static MESON_SETUP_ARGS="--buildtype debugoptimized -Db_sanitize=address,undefined -Db_lundef=true" BUILD_DIR=meson-build-static-debopt-ubasan
make CXXFLAGS="-Werror" LIBRARY_TYPE=static MESON_SETUP_ADDITIONAL_ARGS="--buildtype debugoptimized -Db_sanitize=address,undefined -Db_lundef=true" BUILD_DIR=meson-build-static-debug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants