sanitizers-cmake
sanitizers-cmake copied to clipboard
arsenm
Allow checking whether sanitizers are available
FindXXX packages are supposed to define a <package name>_FOUND variable which shall be set to TRUE. This is the only way to check whether find_package has worked gracefully (i.e., without REQUIRED).
Fixes #16.
@TheAssassin The Sanitizers_FOUND variable shouldn't be used like in your patch. E.g. if you have a module named FindFooBar.cmake, FooBar_FOUND wouldn't be set to true, if the module is found, but if the module detects the library / executable FooBar is found at the system.
Usually, FindPackageHandleStandardArgs will handle this. However, we have to define a logic when sanitizers are available (i.e. which components are required for saying they're available). I'll discuss this in #16.
@alehaa I see your point.
I'd say it makes only sense to set Sanitizers_FOUND to true when at least one of the modules has been found. However, I'd still like to have a way to check whether add_sanitizers is available at all. I tried to check whether the function is defined, but CMake doesn't support that. Therefore, I introduced such a variable. How about renaming the variable I added to some other name, e.g., add_sanitizers_available or something like that?
Found a way to check for it now: if(NOT COMMAND add_sanitizers) [...] seems to work.
I'm working on implementing what I suggested in my last comment at the moment.
Updated the PR to use FindPackageHandleStandardArgs, which works fine in my tests. I'm not 100% sure how this HANDLE_COMPONENTS option works, though. I introduced a list variable Sanitizers_COMPONENTS, as CMake expects at least one REQUIRED_VARS, and thought it was a good way to provide some feedback to the CMake scripts what sanitizers are supported at all. The single components can also be checked like if(Sanitizers_ASan_FOUND) [...].
The only annoyance left is that the function to check for the flags shows warnings. As it is run every time now, you should consider making those warnings status messages or, even better, remove them at all.
All in all, I'm pretty satisfied with the new usage.
You've changed the code in a way, that it'll always search for sanitizers, even if one doesn't want to use them.
No, the sanitizers should be searched on-demand, if the user enabled one of them via -DSANITIZE_ADDRESS=On .... Please give me some hours to elaborate a patch.
@alehaa that doesn't make sense, really. Why is it a problem to perform those tests? This CMake module should provide information on whether sanitizers are available, and then provide information which ones. That's how CMake modules work.
That's right, but then we'll have to rework the messages to be displayed, otherwise one could get several warnings / errors if the compiler doesn't know anything about sanitizers. The user shouldn't be flooded with warnings, if he doesn't want to use sanitizers et all.
@alehaa I think it is sufficient to just remove the warnings, as described in https://github.com/arsenm/sanitizers-cmake/pull/18#issuecomment-403987407. I think hiding the tests themselves is wrong, it's better to give direct feedback to the user. You could introduce this with e.g.,
-- Testing sanitizers support
[...]
-- Sanitizers found: ASan;TSan
With ef2848366f7ec681a8f05a955ad70df7a305b8a4 the variable Sanitizers_FOUND is set to TRUE … always. Not sure if that's correct?
