[PATCH] Optional support for dynamic Asan runtime

[Yury Gribov]

================
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:
> Yury Gribov wrote:
> > 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.
> This is still confusing. sanitizer_internal_defs.h is included not only in ASan source files, but also in sanitizer_common source files, and you don't compile the latter with -DASAN_DYNAMIC=1 (and in general you shouldn't add sanitizer-specific defines to sanitizer_common code).
Ok,I see the point but I'd still prefer not to save performance. Do you think building runtime with -ftls-model=initial-exec would work?

================
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:
> Yury Gribov wrote:
> > 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?
> the diff between the two macro you mention is much larger. In your case the difference is only in: SHARED/STATIC, LIBRARY_OUTPUT_DIRECTORY/ARCHIVE_OUTPUT_DIRECTORY and LIBRARY DESTINATION / ARCHIVE DESTINATION.
> The first choice may be controlled by an input parameter, the second and third may just be combined (you can set both _OUTPUT_DIRECTORY) w/o any harm.
> the diff between the two macro you mention is much larger

Ok, then take a look osx/darwin. Anyway this isn't a big deal, I can refactor my macro.

================
Comment at: lib/asan/asan_rtl.cc:563
@@ +562,3 @@
+  AsanInitializer() {
+    AsanCheckIncompatibleRT();
+    if (!asan_inited) {
----------------
Alexey Samsonov wrote:
> Yury Gribov wrote:
> > 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.
> Then we should be able to somehow use AsanInitFromRtl().
I think it makes sense to detect this error ASAP and not wait till AsanInitFromRtl() gets called. By this time rt inconsistencies may already cause mysterious jumps to NULL, etc.

================
Comment at: lib/asan/asan_win.cc:77
@@ +76,3 @@
+void AsanCheckIncompatibleRT() {
+}
+
----------------
Alexey Samsonov wrote:
> Yury Gribov wrote:
> > 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).
> Looks like AsanCheckDynamicRTPrereqs() is called only in shared-libasan. It makes sense to print "UNIMPLEMENTED" on platforms where it's not available.
> 
> Which reminds me of Mac - we use dynamic runtime there as well. Can you make sure that your changes will play well with it? Does it make sense to build it with -DASAN_DYNAMIC=1 as well?
> It makes sense to print "UNIMPLEMENTED" on platforms where it's not available.

Makes sense.

> Which reminds me of Mac - we use dynamic runtime there as well.

Yeah and it also uses a cool reexec trick which may be useful for other platforms as well (Linux, Android).

> Can you make sure that your changes will play well with it?
> Does it make sense to build it with -DASAN_DYNAMIC=1 as well?

I think there is quite some potential for unification. I could work on this but I have practically no experience with Mac so I'd prefer to do this separately to avoid huge delays.


================
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:
> Yury Gribov wrote:
> > Alexey Samsonov wrote:
> > > ASAN_BUILD_SHARED_RUNTIME
> > Hm, are you sure? I thought all compiler-rt configs should start with COMPILER_RT...
> Alright, let's then use COMPILER_RT_BUILD_SHARED_ASAN, "include" is a bit confusing to me (yep, I know we have COMPILER_RT_INCLUDE_TESTS named after LLVM_INCLUDE_TESTS, but maybe I'll rename it soon).
Ok.

================
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:
> Yury Gribov wrote:
> > 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)?
> We already started to add toolchain-specific logic to adding -l... flags to the Clang driver (for FreeBSD port)
Ok, will check.


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