envoyproxy / envoy-openssl

Envoy OpenSSL extensions
Apache License 2.0
47 stars 45 forks source link

OSSM-4792: Add lint and sanitiser tools #113

Closed dcillera closed 1 year ago

dcillera commented 1 year ago

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

tedjpoole commented 1 year ago

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 commented 1 year ago

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.