arsenm / sanitizers-cmake

CMake modules to help use sanitizers
Other
377 stars 65 forks source link

No _FOUND variable is set #16

Open pisto opened 6 years ago

pisto commented 6 years ago

And that makes it difficult to make sanitizers support optional.

alehaa commented 6 years ago

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

pisto commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Working on a quickfix in #18.

alehaa commented 6 years ago

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 commented 6 years ago

@alehaa I would agree with that.

alehaa commented 6 years ago

@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 commented 6 years ago

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 https://github.com/arsenm/sanitizers-cmake/pull/18#issuecomment-403916235, it is possible to check whether this module is available at all by checking if(NOT COMMAND add_sanitizers).

TheAssassin commented 6 years ago

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

alehaa commented 6 years ago

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 commented 6 years ago

+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 commented 5 years ago

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.