[PATCH] Optional support for dynamic Asan runtime

[Alexey Samsonov]

  First round of comments.


================
Comment at: cmake/Modules/AddCompilerRT.cmake:42
@@ +41,3 @@
+# directory in the build and install trees.
+# add_compiler_rt_shared_runtime(<name> <arch>
+#                                SOURCES <source files>
----------------
Can you avoid duplicating the function, and instead generalize it?

add_compiler_rt_runtime (<name> <arch> [SOURCES <sources>] [CFLAGS <cflags>] [DEFS <defs>] [SHARED]).
which would build the shared runtime if SHARED is specified, and static runtime otherwise?

================
Comment at: lib/asan/asan_rtl.cc:632
@@ +631,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE
+#ifdef ASAN_DYNAMIC
+int __asan_dynamic;
----------------
We generally assume that all ASAN_xxx compiler definitions should be defined. If they are not, we set the default value in the source file, see asan_internal.h

================
Comment at: lib/asan/CMakeLists.txt:166
@@ +165,3 @@
+
+      if (COMPILER_RT_INCLUDE_ASAN_DYNAMIC)
+        add_sanitizer_rt_symbols(clang_rt.asan-${arch}-dynamic asan.syms.extra)
----------------
Do you use symbols from dynamic runtime anywhere?

================
Comment at: lib/asan/CMakeLists.txt:150
@@ +149,3 @@
+        DEFS ${ASAN_DYNAMIC_DEFINITIONS})
+      target_link_libraries(clang_rt.asan-${arch}-dynamic pthread stdc++ dl m c)
+      add_dependencies(asan clang_rt.asan-${arch}-dynamic)
----------------
ldl or lpthread may not be available, see COMPILER_RT_HAS_LIBDL for example.

================
Comment at: lib/asan/CMakeLists.txt:132
@@ -120,1 +131,3 @@
 
+    set(ASAN_RUNTIME_OBJECTS
+      $<TARGET_OBJECTS:RTAsan.${arch}>
----------------
get rid of this and ASAN_DYNAMIC_RUNTIME_OBJECTS, just write all objects you need inside add_compiler_rt_runtime.

================
Comment at: lib/asan/CMakeLists.txt:140
@@ -124,2 +139,3 @@
       DEFS ${ASAN_COMMON_DEFINITIONS})
-    add_dependencies(asan clang_rt.asan-${arch})
+    add_dependencies(asan clang_rt.asan-${arch} clang_rt.asan-${arch})
+
----------------
accidental duplication?

================
Comment at: lib/asan/asan_win.cc:77
@@ +76,3 @@
+void AsanCheckIncompatibleRT() {
+}
+
----------------
Why don't we use UNIMPLEMENTED() here?

================
Comment at: lib/sanitizer_common/sanitizer_internal_defs.h:151
@@ +150,3 @@
+# ifdef ASAN_DYNAMIC
+#  define THREADLOCAL __thread __attribute__((tls_model("initial-exec")))
+# else
----------------
Please don't do this. We should really specfiy this for each variable instead.

================
Comment at: CMakeLists.txt:233
@@ -232,1 +232,3 @@
 
+option(COMPILER_RT_INCLUDE_ASAN_SHARED "Build shared version of AddressSanitizer runtime" OFF)
+
----------------
ASAN_BUILD_SHARED_RUNTIME

================
Comment at: lib/asan/CMakeLists.txt:77
@@ +76,3 @@
+    if (COMPILER_RT_INCLUDE_ASAN_DYNAMIC)
+      set(ASAN_DYNAMIC_DEFINITIONS
+        ${ASAN_COMMON_DEFINITIONS} ASAN_DYNAMIC)
----------------
define this right after ASAN_COMMON_DEFINITIONS


http://llvm-reviews.chandlerc.com/D3042