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.

Sign up for GitHub

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

Add CMake options to build using sanitizers #1053

Merged
merged 10 commits into from
Apr 12, 2018
Merged

Add CMake options to build using sanitizers #1053

Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d3196e4
this certainly works
var-const Apr 8, 2018
1c7f4de
also works
var-const Apr 8, 2018
349492b
cmake wip
var-const Apr 8, 2018
cfe3a54
add options
var-const Apr 8, 2018
6ad84d4
more comments and some refactoring
var-const Apr 9, 2018
233d0d4
wip
var-const Apr 9, 2018
014a61a
fix function setting variables only locally
var-const Apr 9, 2018
9eaf635
Revert the test file to default state
var-const Apr 9, 2018
79ee5cc
Fix typo
var-const Apr 9, 2018
001e97c
Review feedback
var-const Apr 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions Firestore/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,53 @@
cmake_minimum_required(VERSION 2.8.11)
project(firestore C CXX)

option(ASAN "Build with Address sanitizer (mutually exculsive with TSAN)" OFF)
Copy link
Member

Choose a reason for hiding this comment

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

All your changes here might make more sense in cmake/CompilerOptions.cmake, though they would then apply across all of the repo. (As long as everything defaults to off, maybe that's ok?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm okay with that, let's postpone until the next round of review.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's better not to hide the options we support since this file serves as documentation for people trying to figure out how to configure this build to their purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wilhuff Do you think that it would be worthwhile to have these options in the Firebase build as well? If that were the case, I thought about splitting the sanitizer logic from here into ./cmake/Sanitizers.cmake and including that in both projects. It's a good point that it would make these options non-obvious, so perhaps it would be okay to have the declaration of sanitizer options twice, once for each project. What do you think?

# TODO(varconst): msan
# Memory sanitizer is more complicated:
# - it requires all dependencies to be compiled with msan enabled (see
# https://github.com/google/sanitizers/wiki/MemorySanitizerLibcxxHowTo);
# - AppleClang doesn't support it.
option(TSAN "Build with Thread sanitizer (mutually exculsive with ASAN)" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

Conventionally these kinds of options are WITH_FOO. For example, see other CMake files that do it:: https://github.com/facebook/rocksdb/blob/master/CMakeLists.txt#L258. This is not universal though, so feel free to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I like that this prefix makes them grouped together in the cache.

option(UBSAN "Build with Undefined behavior sanitizer" OFF)

if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
Copy link
Member

Choose a reason for hiding this comment

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

cmake/CompilerSetup.cmake defines CLANG and GNU with these values, so you could simplify to just if (CLANG OR GNU). But I don't mind this as is.

(Note that the include of CompilerSetup is below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I can't move CompilerSetup here, because it sets compiler options that don't play nice with Abseil. Conversely, it seems that setting sanitizer options below would affect how the dependencies are compiled, and I thought it's best to compile as much stuff with sanitizers enabled as possible. I might be wrong on this, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also factor out compiler detection from CompilerSetup into a separate CMake module. Alternatively, we could make the core logic of setting up the compile a function within CompilerSetup. That way we can source the file wherever we please and then specifically invoke our compiler settings in the location that makes the best sense.

An extension of that would be to move this setup into there too since this is largely compiler setup stuff anyway,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also factor out compiler detection from CompilerSetup into a separate CMake module

It seems to me that it's the least intrusive way, I can do it in one of the follow-ups.

set(HAVE_SANITIZERS 1)
endif()
if(ASAN OR TSAN OR UBSAN)
set(USE_SANITIZERS 1)
endif()

if (NOT HAVE_SANITIZERS AND USE_SANITIZERS)
message(FATAL_ERROR "Only Clang and GCC support sanitizers.")
endif()

function(add_to_compile_and_link_flags flag)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make this a macro instead of a function you can avoid the PARENT_SCOPE messiness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${flag}" PARENT_SCOPE)
set(CMAKE_LINKER_FLAGS "${CMAKE_LINKER_FLAGS} ${flag}" PARENT_SCOPE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Elsehwere I've seen this specified as CMAKE_EXE_FLAGS since we only need to specify these extra flags during compilation and final linking into our tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it looks like I'm using a non-existent CMake variable. Changed to CMAKE_EXE_LINKER_FLAGS.

The build actually works without specifying the linker flag at all. If linking is invoked via the frontend call (e.g., via clang, not ld), then perhaps the CXX flag is bypassed to the linker automatically. Anyway, perhaps I should drop the linker part altogether, since it appears to be extraneous.

endfunction()

if (HAVE_SANITIZERS AND USE_SANITIZERS)
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic seems more complicated than it needs to be. It seems like you could just have

if(CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
  if(ASAN)
    // add flags
  endif()

  if(TSAN)
    if(ASAN OR UBSAN)
      message(FATAL_ERROR "Cannot combine tsan with other santizers")
    endif()
    // add flags
  endif()

  if(UBSAN)
    // add flags
  endif()
else()
  if(ASAN OR TSAN OR UBSAN)
    message(FATAL_ERROR "Only Clang and GCC support sanitizers")
  endif()
endif()

This eliminates HAVE_SANITIZERS and USE_SANITIZERS and all these extra blocks that check conditions you're effectively duplicating in here through nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. The intention behind those named variables was to make things clearer, but I think the shorter version is at least as readable.

if ((ASAN AND TSAN) OR (UBSAN AND TSAN))
message(FATAL_ERROR "You cannot combine sanitizers except asan and ubsan.")
endif()

# Recommended to "get nicer stack traces in error messages"
# TODO(varconst): double-check that tsan actually needs this flag (it's
# explicitly recommended in the docs for asan and ubsan)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-omit-frame-pointer")

if (TSAN)
add_to_compile_and_link_flags("-fsanitize=thread")
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't tsan also need us to specify -pie on final linking and -fPIC during compilation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, apparently not. The most relevant thing I could find is this thread, which doesn't contain a definitive answer, but has this quote from the Clang docs:

Non-position-independent executables are not supported. Therefore, the fsanitize=thread flag will cause Clang to act as though the -fPIE flag had been supplied if compiling without -fPIC, and as though the -pie flag had been supplied if linking an executable.

I couldn't find the same stated explicitly for GCC. However, the wiki page for TSan only has the following instructions:

Simply compile your program with -fsanitize=thread and link it with -fsanitize=thread. To get a reasonable performance add -O2. Use -g to get file names and line numbers in the warning messages.

(Let me know if you think -g should be added)

else()
if (ASAN)
add_to_compile_and_link_flags("-fsanitize=address")
endif()
if (UBSAN)
add_to_compile_and_link_flags("-fsanitize=undefined")
endif()
endif()
endif()

set(FIREBASE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/..)

# CMAKE_INSTALL_PREFIX should be passed in to this build so that it can find
Expand Down