Skip to content
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

OSSM-4792: Add lint and sanitiser tools #113

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

dcillera
Copy link
Contributor

@dcillera dcillera commented Sep 4, 2023

Signed-off-by: Dario Cillerai dcillera@redhat.com
Added clang-tidy lint tool and address/memory/thread sanitisers.

@dcillera dcillera force-pushed the en_sanitisers branch 3 times, most recently from e9ffda6 to c29d94b Compare September 5, 2023 15:21
@tedjpoole
Copy link
Contributor

As far as the sanitisers are concerned, it seems that the usual pattern is to enable the desired sanitiser(s) at configure time, rather than build time. That way, there's no need for all the extra do-*.sh scripts because all the required compiler & linker flags for the specified sanitiser, get baked into the generated build files i.e. you just specify the desired sanitiser as an option to the cmake configure step, and then you can just run make as usual after that.

Exactly how to specify the sanitiser option to cmake is not standard, but it seems quite common (and tidy) to do it by adding additional cmake build type for each sanitiser. The advantage of doing it this way is the extra sanitiser build types show up in cmake aware tooling such as ccmake, vs code, etc. Here's some examples:

Alternatively, this project looks like a nice way of doing it too.

@tedjpoole
Copy link
Contributor

tedjpoole commented Sep 6, 2023

Ignore my comment above about removing the do-*.sh scripts etc. We can stick with what you have done for now and revisit in the future if required.

However, could we move all of the do-*sh scripts into the bssl-compat/tools/ directory please.
Also, could we remove set -x from the scripts so they don't echo.

bssl-compat/CMakeLists.txt Outdated Show resolved Hide resolved
bssl-compat/README.md Show resolved Hide resolved
bssl-compat/CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Dario Cillerai <dcillera@redhat.com>
Copy link
Contributor

@tedjpoole tedjpoole left a comment

Choose a reason for hiding this comment

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

LGTM

@tedjpoole tedjpoole merged commit e4520c2 into envoyproxy:main Sep 7, 2023
1 check passed
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