Add CMake options to build using sanitizers #1053

var-const · 2018-04-09T19:37:50Z

Supported sanitizers:

Address sanitizer;
Thread sanitizer;
Undefined behavior sanitizer.

Sample run: cmake -DASAN=ON .. (note that CMake caches variables values).

Note that this only applies sanitizers to Firestore build, not the whole Firebase build. If I read the docs correctly, this is explicitly okay for ASan:

Q: I've built my main executable with ASan. Do I also need to build shared libraries?
A: ASan will work even if you rebuild just part of your program. But you'll have to rebuild all components to detect all errors.

Not sure about the other two, but empirically they seem to work correctly.

The next step would be to add these to the Travis build. Note that only ASan and UBSan can work together, and that -fsanitize option requires rebuilding. Also note:

ASan and TSan fail upon encountering the first error;
UBSan goes on after encountering an error;
there appears to be an (experimental?) option to make ASan go on after the first error, might be worth looking into.

var-const · 2018-04-09T19:41:01Z

@rsgowman Rich, could you please take a look and check that what I'm doing is reasonable with regards to CMake? A couple of notes:

these could theoretically be options on the parent Firebase project;
checking for compiler versions could be added, but seems like overkill for the first iteration;
see autoid_test.cc file present throughout most of the commits for examples of code that should fail. If you could check that sanitizers work on Linux, it would be awesome.

rsgowman

If you could check that sanitizers work on Linux, it would be awesome.

$ cmake -DASAN=ON ..
-- The C compiler identification is GNU 7.3.0
-- The CXX compiler identification is GNU 7.3.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ZLIB: /usr/lib/x86_64-linux-gnu/libz.so (found version "1.2.8") 
-- Configuring done
-- Generating done
CMake Warning:
  Manually-specified variables were not used by the project:

    ASAN

I think that's superbuild related. We need to pass it down into the subproject like this:

diff --git a/cmake/external/firestore.cmake b/cmake/external/firestore.cmake
index 94f7caee..65c43f84 100644
--- a/cmake/external/firestore.cmake
+++ b/cmake/external/firestore.cmake
@@ -32,6 +32,7 @@ ExternalProject_Add(
   CMAKE_ARGS
     -DCMAKE_BUILD_TYPE:STRING=${CMAKE_BUILD_TYPE}
     -DCMAKE_INSTALL_PREFIX:PATH=${FIREBASE_INSTALL_DIR}
+    -DASAN:BOOL=${ASAN}
 
   BUILD_ALWAYS ON

With that specified, the warning goes away. (Repeat for TSAN/UBSAN).

That said, everything compiles fine for me, so if there's a problem with autoid_test.cc, my compiler doesn't see it. (I also tried against the autoid_test.cc from HEAD^, i.e. right before you reverted it.)

rsgowman · 2018-04-09T20:07:11Z

Firestore/CMakeLists.txt

+option(TSAN "Build with Thread sanitizer (mutually exculsive with ASAN)" OFF)
+option(UBSAN "Build with Undefined behavior sanitizer" OFF)
+
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")


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

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.

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,

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.

rsgowman · 2018-04-09T20:13:30Z

Firestore/CMakeLists.txt

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


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

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

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.

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

var-const · 2018-04-09T22:09:10Z

@rsgowman

I think that's superbuild related.

Right, I always run just Firestore build (from /Firestore/build), I'll have to figure out how it's supposed to work when started from the Firebase root.

var-const · 2018-04-09T22:33:13Z

@rsgowman Rich, I think that passing the options the way you describe it (in external/firestore.cmake) is the way to go, but I would only add that if these options were to also be added to the main Firebase CMake project, which I'd hold off for now. The (hopefully only) reason your tests ran okay is because I had a typo when setting variables -- however, it worked locally for me, so I didn't notice (probably because of caching). Could you please retry with "bad" version of autoid_test.cc?

I run CMake from Firestore/build using cmake -DASAN=ON -DTSAN=OFF -DUBSAN=ON -DCMAKE_INSTALL_PREFIX=/Users/varconst/firebase-ios-sdk/build .. && cmake --build . && core/test/firebase/firestore/util/firebase_firestore_util_test. I also checked that if I modify external/firestore.cmake locally like you describe, it works as well.

wilhuff

I'm broadly in favor of this change though I think we could afford to

factor this out of the FIrestore specific build
cut down on the validation contortion the code is going through

wilhuff · 2018-04-11T06:09:21Z

Firestore/CMakeLists.txt

+option(TSAN "Build with Thread sanitizer (mutually exculsive with ASAN)" OFF)
+option(UBSAN "Build with Undefined behavior sanitizer" OFF)
+
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")


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,

wilhuff · 2018-04-11T06:23:41Z

Firestore/CMakeLists.txt

+# - 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)


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.

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

wilhuff · 2018-04-11T06:24:56Z

Firestore/CMakeLists.txt

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


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.

wilhuff · 2018-04-11T06:27:39Z

Firestore/CMakeLists.txt

+  message(FATAL_ERROR "Only Clang and GCC support sanitizers.")
+endif()
+
+function(add_to_compile_and_link_flags flag)


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

wilhuff · 2018-04-11T06:47:29Z

Firestore/CMakeLists.txt

+  set(CMAKE_LINKER_FLAGS "${CMAKE_LINKER_FLAGS} ${flag}" PARENT_SCOPE)
+endfunction()
+
+if (HAVE_SANITIZERS AND USE_SANITIZERS)


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.

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

wilhuff · 2018-04-11T06:49:01Z

Firestore/CMakeLists.txt

+
+function(add_to_compile_and_link_flags flag)
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${flag}" PARENT_SCOPE)
+  set(CMAKE_LINKER_FLAGS "${CMAKE_LINKER_FLAGS} ${flag}" PARENT_SCOPE)


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.

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.

wilhuff · 2018-04-11T06:49:57Z

Firestore/CMakeLists.txt

+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-omit-frame-pointer")
+
+  if (TSAN)
+    add_to_compile_and_link_flags("-fsanitize=thread")


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

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)

rsgowman · 2018-04-11T18:41:45Z

@var-const : Using your instructions, I got the following. (I might have got it before too? Unsure. The tests actually pass, despite the errors, therefore masking them.):

ordered_code.cc:261:3: runtime error: store to misaligned address 0x7ffd1d95baa1 for type 'uint64_t', which requires 8 byte alignment
0x7ffd1d95baa1: note: pointer points here

Can we adjust so that these cause the test to fail? Otherwise, I'll never notice. :)

rsgowman · 2018-04-11T19:36:17Z

Followup to my comment: with properly bad code (code I was testing with was only a little bad), things fail spectacularly. :)

We'll still have the issue of the asan (?) only outputting warnings (as displayed in my previous comment). You mentioned that turning them into errors causes an immediate failure, so you'd only see one of them (instead of all of them) making it more difficult to find/fix. Maybe once we're in a happy state (ie no errors) we can enable immediate failure, since (assuming quick test cycles) there'd only ever be the error that you just immediately introduced?

var-const · 2018-04-11T19:51:40Z

@rsgowman Just to write up our offline discussion for posterity:

ASan is designed to fail upon encountering the first issue. My impression is that it's more of a limitation than a deliberate decision. From the wiki page:

Q: Can AddressSanitizer continue running after reporting first error?

A: Yes it can, AddressSanitizer has recently got continue-after-error mode. This is somewhat experimental so may not yet be as reliable as default setting (and not as timely supported). Also keep in mind that errors after the first one may actually be spurious. To enable continue-after-error, compile with -fsanitize-recover=address and then run your code with ASAN_OPTIONS=halt_on_error=0.

UBSan, on the other hand, produces warnings by default (the warning you mentioned in your comment about misaligned pointer comes from UBSan), though it's possible to make UBSan fail using -fnosanitize-recover flag. However, I think that having as many issues at once as possible is useful when it comes to fixing (especially when those issues are of a repetitive nature, as the misaligned pointer warnings are). I think we'd get the best of both worlds if UBSan run was wrapped in a script that returned an error if it encountered any warnings (but didn't stop on first warning). I can work on that in a future follow-up.

wilhuff

This is likely worth committing as-is. We can refactor this later if it turns out that more projects within Firebase want it.

Example run: `cmake -DWITH_ASAN=ON ..` (from Firestore build folder)

var-const added 8 commits April 8, 2018 17:22

this certainly works

d3196e4

also works

1c7f4de

cmake wip

349492b

add options

cfe3a54

more comments and some refactoring

6ad84d4

wip

233d0d4

fix function setting variables only locally

014a61a

Revert the test file to default state

9eaf635

googlebot added the cla: yes label Apr 9, 2018

var-const requested a review from rsgowman April 9, 2018 19:37

rsgowman reviewed Apr 9, 2018

View reviewed changes

Fix typo

79ee5cc

var-const assigned rsgowman Apr 11, 2018

wilhuff suggested changes Apr 11, 2018

View reviewed changes

Review feedback

001e97c

wilhuff approved these changes Apr 12, 2018

View reviewed changes

wilhuff assigned var-const and unassigned rsgowman Apr 12, 2018

wilhuff added the api: firestore label Apr 12, 2018

var-const merged commit 9784471 into master Apr 12, 2018

var-const deleted the varconst/sanitizers-cmake branch April 12, 2018 16:15

minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018

Add CMake options to build using sanitizers (firebase#1053)

92bab4c

Example run: `cmake -DWITH_ASAN=ON ..` (from Firestore build folder)

firebase locked and limited conversation to collaborators Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CMake options to build using sanitizers #1053

Add CMake options to build using sanitizers #1053

var-const commented Apr 9, 2018

var-const commented Apr 9, 2018

rsgowman left a comment

rsgowman Apr 9, 2018

var-const Apr 9, 2018

wilhuff Apr 11, 2018

var-const Apr 11, 2018

rsgowman Apr 9, 2018

var-const Apr 9, 2018

wilhuff Apr 11, 2018

var-const Apr 11, 2018

var-const commented Apr 9, 2018

var-const commented Apr 9, 2018 •

edited

Loading

wilhuff left a comment

wilhuff Apr 11, 2018

wilhuff Apr 11, 2018

var-const Apr 11, 2018

wilhuff Apr 11, 2018

wilhuff Apr 11, 2018

var-const Apr 11, 2018

wilhuff Apr 11, 2018

var-const Apr 11, 2018

wilhuff Apr 11, 2018

var-const Apr 11, 2018

wilhuff Apr 11, 2018

var-const Apr 11, 2018

rsgowman commented Apr 11, 2018

rsgowman commented Apr 11, 2018

var-const commented Apr 11, 2018

wilhuff left a comment

Add CMake options to build using sanitizers #1053

Add CMake options to build using sanitizers #1053

Conversation

var-const commented Apr 9, 2018

var-const commented Apr 9, 2018

rsgowman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const commented Apr 9, 2018

var-const commented Apr 9, 2018 • edited Loading

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsgowman commented Apr 11, 2018

rsgowman commented Apr 11, 2018

var-const commented Apr 11, 2018

wilhuff left a comment

Choose a reason for hiding this comment

var-const commented Apr 9, 2018 •

edited

Loading