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

No _FOUND variable is set #16

Open
pisto opened this issue Jan 30, 2018 · 13 comments · May be fixed by #18
Open

No _FOUND variable is set #16

pisto opened this issue Jan 30, 2018 · 13 comments · May be fixed by #18

Comments

@pisto
Copy link

pisto commented Jan 30, 2018

And that makes it difficult to make sanitizers support optional.

@alehaa
Copy link
Contributor

alehaa commented Jan 30, 2018

Can you describe a case, where you need this variables? add_sanitizers() will handle this internal for you.

@pisto
Copy link
Author

pisto commented Jan 31, 2018

The point is to be able to pull this module as a whole optionally (e.g. a user may or may not pull it when I set it as a submodule of my own git repo). add_sanitizers() generates a fatal error when called if the module is not found. Currently I use this workaround:

set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/sanitizers-cmake/cmake" ${CMAKE_MODULE_PATH})
find_package(Sanitizers)  #this may fail, in a non fatal way

#[...]

#target
add_executable([...])
if(DEFINED SANITIZE_LINK_STATIC)
add_sanitizers(interazioni)  #this generates a fatal error if called non conditionally, and the module was not found
endif()

because I found that just loading the module defines SANITIZE_LINK_STATIC, but it's rather backward. It's my understanding that most cmake modules set a *_FOUND variable when the find_package() command actually succeeds, see for example Boost.

@TheErk
Copy link

TheErk commented Jan 31, 2018

This is a good point.
Sanitizers related modules
(FindSanitizers, FindTSan, FindASan etc...
could use FPHSA:
https://cmake.org/cmake/help/latest/module/FindPackageHandleStandardArgs.html

@henryiii
Copy link

Also, if you don't set package_name_FOUND, CMake's FeatureSummary doesn't report correctly. (Honestly, since this checks a compiler feature, it seems like it really should be a module rather than a find package. Would be easier to include, too).

@TheAssassin TheAssassin linked a pull request Jul 10, 2018 that will close this issue
@TheAssassin
Copy link

Working on a quickfix in #18.

@alehaa
Copy link
Contributor

alehaa commented Jul 10, 2018

I took a look at #18 and found a little problem: when should Sanitizers_FOUND be true? Usually, this var is true, if a specific library is found. However, add_sanitizers() even works, if the compiler doesn't support any of the sanitizers.

Do you agree in setting Sanitizers_FOUND as always true and setting ASan_FOUND, MSan_FOUND, ... depending on the compiler support for the specific sanitizer?

@henryiii
Copy link

@alehaa I would agree with that.

@alehaa
Copy link
Contributor

alehaa commented Jul 10, 2018

@TheAssassin I'll comment here to get the discussion coherent.

I'd say it makes only sense to set Sanitizers_FOUND to true when at least one of the modules has been found.

I don't think this is useful, as the sanitizers are independent. Therefore setting the variable as always true like done in your patch, seems useful for me. However, find_package(Sanitizers) could be enhanced to check for the sanitizers as components.

@TheAssassin
Copy link

Looking into how to "detect" sanitizer support at the moment. There's sanitizer_check_compiler_flags in the helpers script, I'm looking into how it works and whether it can be modified to set e.g., ASan_FOUND.

I'd say it makes only sense to set Sanitizers_FOUND to true when at least one of the modules has been found.

I don't think this is useful, as the sanitizers are independent. Therefore setting the variable as always true like done in your patch, seems useful for me. However, find_package(Sanitizers) could be enhanced to check for the sanitizers as components.

I have changed my mind since I looked into how FindPackageHandleStandardArgs works. It supports a component-based interface, so if the module supports it, you could check for the support of specific sanitizers using e.g., find_package(Sanitizers COMPONENTS ASan TSan) comfortably. In this scenario, without REQUIRED, you could simply check whether at least one of them is available by checking the "global" Sanitizers_FOUND.

As described in #18 (comment), it is possible to check whether this module is available at all by checking if(NOT COMMAND add_sanitizers).

@TheAssassin
Copy link

Updated the PR to introduce FindPackageHandleStandardArgs, you're welcome to leave your comments there, everyone.

@alehaa
Copy link
Contributor

alehaa commented Jul 17, 2018

I've been thinking a bit about this issue. After all, I don‘t think there is any sense in include FPHSA et all.

FPHSA is used to tell, if a specific library or tool is available or not. One could say this module works the same. However, this module is not used to tell, if compiler XY supports AddressSanitizer or not, but conditionally enable compiler flags if they‘re supported. And there‘s no reliable way to tell, if this module „found“ the sanitizers.

Let me give you a small example: I reworked this module for use in PnMPI. This project includes a library composed out of C and Fortran files. However, clang and GNU sanitizers aren‘t compatible in the same object. How should we anser if sanitizers work for this target, if we don‘t know anything about the targets this module is being used on? That can be done in add_sanitizers() only ...

A second disadvantage is, that the the compiler feature doesn‘t tell anything about the feature actually being used. That depends on the ENABLE_xxx var and the target being compiled.

And what about projects using multiple languages and one of the compilers doesn‘t have sanitizer support? did we find sanitizers then, or didn’t we?

I agree with @henryiii, that this module possibly shouldn‘t be a find module. CMake itself supports conditional compiler features, but as far as I know, these are not extensible with additional features.

After all, I think FPHSA shouldn‘t be used in the described way. For
the described feature request, if (COMMAND add_sanitizer) would just do fine in my opinion.

@edsiper
Copy link

edsiper commented Sep 29, 2018

+1 on this.

I ran into the same problem: we need a clean way to determinate if Sanitizer package was found or not. The way to go is to define Sanitizers_FOUND.

@j1elo
Copy link

j1elo commented Feb 27, 2019

Chiming in to say "me too"...

My use case was that sanitizers-cmake will be distributed as an optional CMake-only tools package with helpful CMake extensions. Users might have or not this package, so any other project should use the provided tools exclusively in a conditional way.

So in other words, it should be possible to do this:

find_package(Sanitizers)

add_executable(some_exe foo.c bar.c)
if(Sanitizers_FOUND)
  add_sanitizers(some_exe)
endif()

add_library(some_lib foo.c bar.c)
if(Sanitizers_FOUND)
  add_sanitizers(some_lib)
endif()

Also I fell victim to a partial lie from the official CMake documentation on find_package, which states that...

<package>_FOUND will be set to indicate whether the package was found.

And well, I'm saying it is a "lie" because it seems to imply that CMake itself will set that variable but seems that's not the case, given the existence of this bug report... even that technically the Sanitizers CMake module was correctly found and loaded.

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 a pull request may close this issue.

7 participants