Skip to content

Add ci sanitizers#234

Open
armintoepfer wants to merge 3 commits intoMartinsos:masterfrom
armintoepfer:add-ci-sanitizers
Open

Add ci sanitizers#234
armintoepfer wants to merge 3 commits intoMartinsos:masterfrom
armintoepfer:add-ci-sanitizers

Conversation

@armintoepfer
Copy link
Copy Markdown

No description provided.

@armintoepfer
Copy link
Copy Markdown
Author

Let's see if that works as intended :)

@armintoepfer
Copy link
Copy Markdown
Author

@Martinsos You need to approve that I can run CI

@Martinsos
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 ?=

--backend=ninja \
-Ddefault_library=${LIBRARY_TYPE}
-Ddefault_library=${LIBRARY_TYPE} \
${MESON_SETUP_ARGS}
Copy link
Copy Markdown
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}

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
Copy Markdown
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