Enable ASan in all components, switch stdlib to libc++, switch to lld #741

senderista · 2021-06-18T14:32:16Z

This change passes compiler/linker flags to all built components to enable AddressSanitizer when -DSANITIZER=ASAN is passed to cmake (default in debug builds), and to use libc++ rather than libstdc++ (which is the default stdlib with clang even though it's the GCC stdlib and libc++ is the LLVM stdlib). We also set the linker flag -fuse-ld=lld everywhere to force the lld linker to be used rather than GNU ld (again the default with clang even though lld is the LLVM linker). The combination of libc++ and lld will make it possible to enable MemorySanitizer in an upcoming PR. Additionally, lld is reputed to make LLVM builds much faster (particularly debug builds), but I haven't measured the difference. Note that if you aren't using gdev, you need to install the following packages (assuming Ubuntu 20.04):

clang-10
libc++-10-dev
libc++abi-10-dev
lld-10

If you only install the lld-10 package but not the lld package (which installs lld-10 as the default lld version on Ubuntu 20.04 but not on 18.04), you may need to create a symlink from ld.lld to ld.lld-10 so that linker flags will work properly:

update-alternatives --install "/usr/bin/ld.lld" "ld.lld" "/usr/bin/ld.lld-10" 10

(As usual, this is already taken care of for you by gdev.)

PS: I had to enable ASan in all built components in order to switch to libc++, because ASan is able to use a feature called "container overflow detection" when libc++ is used (it's annotated to enable this feature). However, this will lead to false positives when any code is linked that wasn't built with ASan, so I had to pass ASan compiler/linker flags to all our third-party components as well. After doing so, I verified that ASan no longer produced false positives. Enabling UBSan on all built components produced runtime errors in LLVM, so for now UBSan is only enabled on Gaia components.

simone-gaia · 2021-06-18T16:05:12Z

production/CMakeLists.txt

 endif()

 # Debug build-specific options.
 if(CMAKE_BUILD_TYPE STREQUAL "Debug")
-  set(GAIA_COMPILE_FLAGS "-c -Wall -Wextra -O0 -g -fno-limit-debug-info -ftime-trace")
+  set(GAIA_COMPILE_FLAGS "-c -Wall -Wextra -O0 -g -fno-limit-debug-info -fno-omit-frame-pointer -fno-optimize-sibling-calls -ftime-trace")


Looking at other projects I don't get why we use our own GAIA_COMPILE_FLAGS and GAIA_LINKER_FLAGS. Every time you have some arguments you want to apply both to Gaia and other code you need to set it twice. Not something that need to be addressed in this PR, but maybe you know why.

I agree, I don't think it's proper usage of cmake and I've wanted to fix it for a while. But it took so much fiddling to get these changes to work that I'd like to reserve that for a separate PR.

Cool, I just wanted to know if there was a strong reason to keep them.

The idea was to share the default compilation/linking flags across the project. If there's a better way to do that, we should use it.

It would be easy enough to just use add_compile_options()/add_link_options() at the top level, with the current contents of GAIA_*_FLAGS, but I'm worried that might break some 3rd-party code. I'll try that once this PR is checked in.

simone-gaia

Another thing is:

Additionally, since we don't force lld to be used via linker flags, you need to set the following environment variables in your .bashrc or equivalent:

Why don't we force it?

simone-gaia · 2021-06-18T16:07:05Z

production/CMakeLists.txt

+  add_compile_options(-fno-sanitize-recover=all)
+  add_link_options(-fno-sanitize-recover=all)
+
+  # Enabling ASan always enables UBSan as well (they are compatible so we have no separate option for UBSan).
+  if("$CACHE{SANITIZER}" STREQUAL "ASAN")
+    add_compile_options(-fsanitize=address -fsanitize=undefined)
+    add_link_options(-fsanitize=address -fsanitize=undefined)


Do these 2 commands affect GAIA flags?

Yes, they affect the compile/link flags for everything we build. That's the intention (again, everything must be built with ASan after switching to libc++).

simone-gaia · 2021-06-18T16:08:01Z

production/db/core/CMakeLists.txt

@@ -165,8 +161,8 @@ if(Python3_FOUND AND pybind11_FOUND)
  # See https://github.com/pybind/pybind11/issues/1604 for inclusion of -fsized-deallocation for C++17
  set_target_properties(gaia_db_pybind PROPERTIES COMPILE_FLAGS
    "${GAIA_COMPILE_FLAGS} -Wno-deprecated-register -Wno-unused-parameter -fsized-deallocation")
-  set_target_properties(gaia_db_pybind PROPERTIES LINK_FLAGS ${GAIA_LINK_FLAGS})


It could be a good moment to remove both the python and the jni stuff....

I don't think that's in scope for this PR, and it would need prior discussion.

simone-gaia · 2021-06-18T16:09:17Z

production/CMakeLists.txt

+  add_link_options(-fno-sanitize-recover=all)
+
+  # Enabling ASan always enables UBSan as well (they are compatible so we have no separate option for UBSan).
+  if("$CACHE{SANITIZER}" STREQUAL "ASAN")


Why is $CACHE{SANITIZER} inside "?

Why are you using $CACHE to access SANITIZER?

It turned out that I couldn't use the bare variable reference (CACHE{SANITIZER}) with STREQUAL, so I needed to compare two strings, and it's safer to quote a string value.

Any variable specified with -D on the cmake command line is automatically a cache variable.

simone-gaia · 2021-06-18T16:09:55Z

production/CMakeLists.txt

-  if(ENABLE_ASAN)
-    set(GAIA_COMPILE_FLAGS "${GAIA_COMPILE_FLAGS} -fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all -fno-omit-frame-pointer")
-    set(GAIA_LINK_FLAGS "${GAIA_LINK_FLAGS} -fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all")
+  if (BUILD_SHARED_LIBRARY AND RUN_TESTS)


Where this BUILD_SHARED_LIBRARY come from, or better how it affects Gaia since we almost build everything static.

OMG, good catch! Cut'n'paste detritus from SO...

senderista · 2021-06-18T16:49:20Z

Additionally, since we don't force lld to be used via linker flags, you need to set the following environment variables in your .bashrc or equivalent:

Why don't we force it?

We could force it, I just want to avoid unnecessarily passing flags to projects we don't own. But it might be better to pass -fuse-ld=lld-10 everywhere instead since that wouldn't require user configuration to work properly (but note that we already require environment variable configuration for clang-10). Is that your take?

README.md

simone-gaia · 2021-06-18T17:00:41Z

Additionally, since we don't force lld to be used via linker flags, you need to set the following environment variables in your .bashrc or equivalent:

Why don't we force it?

We could force it, I just want to avoid unnecessarily passing flags to projects we don't own. But it might be better to pass -fuse-ld=lld-10 everywhere instead since that wouldn't require user configuration to work properly (but note that we already require environment variable configuration for clang-10). Is that your take?

Yes, probably we should set also the compilers since I don't see us using GCC or any other compiler any time soon. (see: https://stackoverflow.com/questions/7031126/switching-between-gcc-and-clang-llvm-using-cmake)

LaurentiuCristofor · 2021-06-18T17:55:43Z

production/CMakeLists.txt

@@ -33,6 +31,23 @@ set(CMAKE_CXX_STANDARD_REQUIRED True)
 # Ensure debug builds have the DEBUG flag defined.
 add_compile_options("$<$<CONFIG:DEBUG>:-DDEBUG>")

+# Ensure that PIE flags are actually passed to the linker.


Could you add a quick comment on what PIE flags are and are needed for?

Added comment for next commit.

senderista · 2021-06-18T18:27:07Z

Additionally, since we don't force lld to be used via linker flags, you need to set the following environment variables in your .bashrc or equivalent:

Why don't we force it?

We could force it, I just want to avoid unnecessarily passing flags to projects we don't own. But it might be better to pass -fuse-ld=lld-10 everywhere instead since that wouldn't require user configuration to work properly (but note that we already require environment variable configuration for clang-10). Is that your take?

Yes, probably we should set also the compilers since I don't see us using GCC or any other compiler any time soon. (see: https://stackoverflow.com/questions/7031126/switching-between-gcc-and-clang-llvm-using-cmake)

So the top answer suggests using CMAKE_USER_MAKE_RULES_OVERRIDE to point to an override file (which could specify the compiler/linker to use). The problem is that this solution still requires -DCMAKE_USER_MAKE_RULES_OVERRIDE to be passed on the cmake command line, and if we do that we may as well just set CXX/LD on the cmake command line. Again, we can't really enforce any of these requirements without a standardized build command, so that's what I think we need to focus on next.

LaurentiuCristofor · 2021-06-18T21:01:01Z

third_party/production/rocksdb/CMakeLists.txt

@@ -60,6 +60,9 @@ option(WITH_WINDOWS_UTF8_FILENAMES "use UTF8 as characterset for opening files,
 if (WITH_WINDOWS_UTF8_FILENAMES)
  add_definitions(-DROCKSDB_WINDOWS_UTF8_FILENAMES)
 endif()
+# We always want to enable the attributes selectively disabling UBSan.


This doesn't read clearly to me. Is a word missing in this sentence?

My questions:

Which attributes?

Does "selectively" refer to enabling the attributes or to disabling UBSan? What does it mean exactly?

What's the connection between enabling attributes and disabling UBSan?

This is moot because I no longer enable UBSan on 3rd-party components when ASan is enabled via the SANITIZER option.

LaurentiuCristofor

I am not clear on the meaning of a comment, but otherwise I have no suggestions/requests for change.

…LLD to be used everywhere

simone-gaia

If you can add the -fno-limit-debug-info flag to LLVm cool, otherwise no worries.

simone-gaia · 2021-06-19T00:19:16Z

production/CMakeLists.txt

-  if (BUILD_SHARED_LIBRARY AND RUN_TESTS)
-    message(FATAL_ERROR "Can't build shared library and run tests at same time")
-  endif()
+  set(GAIA_COMPILE_FLAGS "-c -Wall -Wextra -O0 -g3 -ggdb -fno-limit-debug-info -fno-omit-frame-pointer -fno-optimize-sibling-calls -ggnu-pubnames -gsplit-dwarf -ftime-trace")


Sorry, forgot to mention it. We need -fno-limit-debug-info also in LLVM, otherwise, it won't be possible to see the values of things such as strings.

Unfortunately there not seem to be one of the LLVM_.... cmake options for this.

Added this flag to all built binaries in latest commit, and verified it is included in the LLVM compile command line.

…shared ASan runtime lib

senderista · 2021-06-19T20:21:04Z

FYI, in the latest commit I removed the -shared-libasan linker flag from the SDK build. It's no longer necessary to run tests because the test executables are now also built with ASan. It would be necessary if the ASan-enabled SDK shared lib were linked with other executables not built from our repo, but I can't think of any such scenario now, and if we encounter such a scenario in the future we can restore the flag (and somehow add the ASan shared runtime path to LD_PRELOAD when the executable is run, like we do now for the Postgres FDW lib).

senderista · 2021-06-19T20:33:23Z

I think I've addressed all concerns, so I'm merging without waiting for approvals from the remaining reviewers. I'll copy the README from this PR to the #engineering channel so there are no surprises from new dependencies.

Enable ASan in all components, switch stdlib to libc++, switch to lld

4f8d036

senderista requested review from LaurentiuCristofor, daxhaw, fineg74 and simone-gaia June 18, 2021 14:32

simone-gaia reviewed Jun 18, 2021

View reviewed changes

LaurentiuCristofor reviewed Jun 18, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

LaurentiuCristofor reviewed Jun 18, 2021

View reviewed changes

LaurentiuCristofor approved these changes Jun 18, 2021

View reviewed changes

senderista added 2 commits June 18, 2021 16:34

Disable UBSan on 3rd-party libs, add more flags to LLVM build, force …

d7d642c

…LLD to be used everywhere

remove useless LD env var from docs

ee1c0eb

simone-gaia approved these changes Jun 19, 2021

View reviewed changes

senderista added 3 commits June 18, 2021 17:33

fix variable quoting

9fed301

forgot LD line

b1d6194

include full debug info in all built binaries, don't link SDK lib to …

9c624de

…shared ASan runtime lib

senderista merged commit 46f59b0 into master Jun 19, 2021

senderista deleted the enable_lld_libcxx branch June 19, 2021 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable ASan in all components, switch stdlib to libc++, switch to lld #741

Enable ASan in all components, switch stdlib to libc++, switch to lld #741

senderista commented Jun 18, 2021 •

edited

Loading

simone-gaia Jun 18, 2021

senderista Jun 18, 2021

simone-gaia Jun 18, 2021

LaurentiuCristofor Jun 18, 2021

senderista Jun 18, 2021

simone-gaia left a comment

simone-gaia Jun 18, 2021

senderista Jun 18, 2021

simone-gaia Jun 18, 2021

senderista Jun 18, 2021

simone-gaia Jun 18, 2021

senderista Jun 18, 2021

simone-gaia Jun 18, 2021

senderista Jun 18, 2021

senderista commented Jun 18, 2021

simone-gaia commented Jun 18, 2021

LaurentiuCristofor Jun 18, 2021

senderista Jun 18, 2021

senderista commented Jun 18, 2021

LaurentiuCristofor Jun 18, 2021 •

edited

Loading

senderista Jun 19, 2021

LaurentiuCristofor left a comment

simone-gaia left a comment

simone-gaia Jun 19, 2021

simone-gaia Jun 19, 2021

senderista Jun 19, 2021

senderista commented Jun 19, 2021

senderista commented Jun 19, 2021

Enable ASan in all components, switch stdlib to libc++, switch to lld #741

Enable ASan in all components, switch stdlib to libc++, switch to lld #741

Conversation

senderista commented Jun 18, 2021 • edited Loading

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

simone-gaia 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

senderista commented Jun 18, 2021

simone-gaia commented Jun 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senderista commented Jun 18, 2021

LaurentiuCristofor Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentiuCristofor left a comment

Choose a reason for hiding this comment

simone-gaia 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

senderista commented Jun 19, 2021

senderista commented Jun 19, 2021

senderista commented Jun 18, 2021 •

edited

Loading

LaurentiuCristofor Jun 18, 2021 •

edited

Loading