[PATCH] Optional support for dynamic Asan runtime

[Yury Gribov]

================
Comment at: CMakeLists.txt:233
@@ -232,1 +232,3 @@
 
+option(COMPILER_RT_INCLUDE_ASAN_SHARED "Build shared version of AddressSanitizer runtime" OFF)
+
----------------
Alexey Samsonov wrote:
> ASAN_BUILD_SHARED_RUNTIME
Hm, are you sure? I thought all compiler-rt configs should start with COMPILER_RT...

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

================
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)
----------------
Alexey Samsonov wrote:
> ldl or lpthread may not be available, see COMPILER_RT_HAS_LIBDL for example.
Ok, I'll add checks then. What about adding these for static rt as well (in Clang driver)?

================
Comment at: lib/asan/asan_win.cc:77
@@ +76,3 @@
+void AsanCheckIncompatibleRT() {
+}
+
----------------
Alexey Samsonov wrote:
> Why don't we use UNIMPLEMENTED() here?
Well, they are still called from initialization code (like other empty functions in this file).

================
Comment at: lib/sanitizer_common/sanitizer_internal_defs.h:151
@@ +150,3 @@
+# ifdef ASAN_DYNAMIC
+#  define THREADLOCAL __thread __attribute__((tls_model("initial-exec")))
+# else
----------------
Alexey Samsonov wrote:
> Please don't do this. We should really specfiy this for each variable instead.
This was Kostya's request and I think it makes sense. We require ASan runtime to be the first dynamic library to load so we may safely assume that initial-exec works.

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

================
Comment at: lib/asan/asan_rtl.cc:563
@@ +562,3 @@
+  AsanInitializer() {
+    AsanCheckIncompatibleRT();
+    if (!asan_inited) {
----------------
Alexey Samsonov wrote:
> Can't you simply call __asan_init() here? It also calls AsanCheckIncompatibleRT, checks for asan_inited, and you may add a call to AsanCheckDynamicRTPrereqs under #if ASAN_DYNAMIC
No, because then __asan_init may then get called twice:
1) once from .preinit_array
2) once from this ctor
which will force ASan in activated state. Actually ability to start in deactivated state greatly complicates startup.

================
Comment at: lib/asan/asan_rtl.cc:632
@@ +631,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE
+#ifdef ASAN_DYNAMIC
+int __asan_dynamic;
----------------
Alexey Samsonov wrote:
> 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
Will do.

================
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)
----------------
Alexey Samsonov wrote:
> Do you use symbols from dynamic runtime anywhere?
No, probably not. I'll remove this.

================
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})
+
----------------
Alexey Samsonov wrote:
> accidental duplication?
Most probably result of code rewrite.

================
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>
----------------
Alexey Samsonov wrote:
> 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?
Well, there is already so much code dup (e.g. compare add_compiler_rt_static_runtime against add_compiler_rt_osx_static_runtime). Perhaps it should be addressed in a separate patch?


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