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

Allow checking whether sanitizers are available #18

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

Conversation

TheAssassin
Copy link

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.

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`).
@alehaa
Copy link
Contributor

alehaa commented Jul 10, 2018

@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.

@TheAssassin
Copy link
Author

@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?

@TheAssassin
Copy link
Author

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.

@alehaa
Copy link
Contributor

alehaa commented Jul 10, 2018

Please see the discussion in #16.

@TheAssassin
Copy link
Author

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.

@alehaa
Copy link
Contributor

alehaa commented Jul 11, 2018

You've changed the code in a way, that it'll always search for sanitizers, even if one doesn't want to use them.

@TheAssassin
Copy link
Author

@alehaa That's the idea... finding out which sanitizers are available.

@alehaa
Copy link
Contributor

alehaa commented Jul 11, 2018

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.

@TheAssassin
Copy link
Author

@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.

@alehaa
Copy link
Contributor

alehaa commented Jul 11, 2018

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.

@TheAssassin
Copy link
Author

@alehaa I think it is sufficient to just remove the warnings, as described in #18 (comment). 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

@TheAssassin
Copy link
Author

@alehaa if I'd remove those warnings, could we merge this, then?

@s-martin
Copy link

Hi, is there any progress on this PR?

Copy link

@TheErk TheErk left a comment

Choose a reason for hiding this comment

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

I think this is the way to go.
Each sanitizer submodule should provide a way (XSan_FOUND) to tell whether if appropriate compiler flags were found. That way client project can warn when XSan_FOUND is not defined.

@TheErk
Copy link

TheErk commented May 23, 2019

To be clear, I think this PR is worth a merge as it is now.

azubieta added a commit to AppImageCommunity/AppImageUpdate that referenced this pull request Dec 1, 2021
azubieta added a commit to AppImageCommunity/AppImageUpdate that referenced this pull request Dec 2, 2021
TheAssassin pushed a commit to AppImageCommunity/AppImageUpdate that referenced this pull request Jan 12, 2022
@LeSpocky
Copy link
Contributor

With ef28483 the variable Sanitizers_FOUND is set to TRUE … always. Not sure if that's correct?

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.

No _FOUND variable is set
5 participants