Add compiler support for dynamic Asan runtime

[ygribov]

Now link shared libs against dynamic runtime.

[samsonov]

lib/Driver/SanitizerArgs.cpp

176

Why not hasArg()?

lib/Driver/Tools.cpp

1849

The logic is too obscure here. --whole-archive makes no sense for dynamic library.

If we're linking executable, add "--whole-archive X --no-whole-archive", where X is either full asan, or preinit-asan.
Otherwise add libasan.so.

Maybe it would be even simpler to pull this logic to addAsanRT (we handle dynamic runtime for Android there, for instance),
and call addSanitizerRTLinkFlags with either "asan", or "asan-preinit".

[samsonov]

Please add tests for this

[ygribov]

Please add tests for this

Done as part of D3043. Or do you want Clang tests? But I don't think we have any to date, everything is tested in compiler-rt...

lib/Driver/SanitizerArgs.cpp

176

Will fix.

lib/Driver/Tools.cpp

1849

Ok, agreed. I'll see what I can do.

[ygribov]

Done as part of D3043

Sorry, D3042

[samsonov]

We have tests for Clang driver behavior. It makes sense to verify that driver is doing the right thing w/o actually checking out / building compiler-rt. You should probably extend test/Driver/sanitizer-ld.c

[ygribov]

Based on Alexey's comments. Tests still tbd.

[ygribov]

Ok, this isn't right. I'll fix everything tomorrow.

[ygribov]

Fixed, now passes check-all.

[samsonov]

Please rebase the patch before uploading the next diff.

lib/Driver/Tools.cpp

1882–1898

The logic is still non-understandable. Please find a better way to express it. As a suggestion:

if (NeedsSharedRT) {
  // add asan-dynamic.so to the link line.
}
if (Args.hasArg(options::OPT_shared)) {
  return;  // don't link static parts into shared objects.
}
StringRef AsanStaticPart = NeedsSharedRT ? "asan-preinit" : "asan";
bool BeforeLibStdCXX = true;
bool ExportSymbols = !NeedsSharedRT;
addSanitizerRTLinkFlags(TC, Args, CmdArgs, AsanStaticPart, BeforeLibStdCXX, ExportSymbols);

now, if you don't want do add "-ldl lpthread -lrt -lm" to the link line for executable and -shared-libasan mode, you may add one more
flag to control whether you add these libs or not. Or, as an alternative, factor adding "-ldl ..." to a separate function and call it when necessary.

[ygribov]

Second try to improve readability.

[ygribov]

Fixed (I hate CMake...).

[samsonov]

General comments:

1. please don't modify the behavior of Clang driver for static runtime. I think that some users may depend on the fact that there's a single ASan .a runtime, and wouldn't like to break them without strong reasons.
2. nit: please decrease the amount of added vertical whitespaces.

lib/Driver/Tools.cpp

1817–1823

Update the comment.

1818

just "bool Shared". Also, I think you can avoid using the default value this argument.

1865

Add the comment specifying that we need to explicitly link with system libraries static ASan runtime depends on.

1881–1882

use an early return here to decrease identation:

if (Android) {
  // code
  return;
}
// more code

[ygribov]

Fixed based on Alexey's comments.

[samsonov]

I think this change will be fine to go in after the following fixes. I'll take a look at compiler-rt part tomorrow.

lib/Driver/SanitizerArgs.cpp

174

AsanSharedRuntime should be true on Android.

lib/Driver/Tools.cpp

1870–1871

Delete this. You already have LibSanitizer defined above.

1881–1882

Please put this Android-specific logic to getSanitizerRTLibName. Then you'll be able to merge this block with the following one (and quit early on Android - there's no asan-preinit there.

1895

Oh, let's switch back to a single

addSanitizerRTLinkFlags(TC, Args, CmdArgs, LibAsanStaticPart,
  /* BeforeLibStdCXX */ true, /* Export Symbols */ !Shared, /* Link Deps */ !Shared);

Sorry for changing my mind.

[ygribov]

lib/Driver/Tools.cpp

1881–1882

But this would change behavior for other runtimes (Ubsan, etc.).

1895

Are you sure about those intra-comment whites? We do not use them anywhere else.

[samsonov]

lib/Driver/Tools.cpp

1881–1882

That's ok, we have only ASan for Android at the moment.

1895

Um, yes, there shouldn't be whitespaces.

[ygribov]

More fixes.

[samsonov]

lib/Driver/SanitizerArgs.cpp

174

Why not define AsanSharedRuntime to true for llvm::Triple::Android here?

lib/Driver/Tools.cpp

1822

clang-format-diff this diff as well.

[ygribov]

lib/Driver/SanitizerArgs.cpp

174

Well, I thought about this. But my understanding was that SanitizerArgs is really about cmdline args, not about making control decisions.

[samsonov]

lib/Driver/SanitizerArgs.cpp

174

Kind of... But we may think of it this way: "-shared-libasan argument is implied for Android". Otherwise, logic for dynamic runtime on Android shoudln't be different from dynamic runtime on Linux, there's no need to special-case it in control decisions.

[ygribov]

Updated.

[samsonov]

LGTM. I can commit this after support for shared runtime in compiler-rt is in.

[samsonov]

Landed as r205310. Thanks!

include/clang/Driver/Options.td

 def fPIE : Flag<["-"], "fPIE">, Group<f_Group>;
 def fno_PIE : Flag<["-"], "fno-PIE">, Group<f_Group>;
 def faccess_control : Flag<["-"], "faccess-control">, Group<f_Group>;
 def fallow_unsupported : Flag<["-"], "fallow-unsupported">, Group<f_Group>;
 def fapple_kext : Flag<["-"], "fapple-kext">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use Apple's kernel extensions ABI">;
 def fapple_pragma_pack : Flag<["-"], "fapple-pragma-pack">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Enable Apple gcc-compatible #pragma pack handling">;
+def shared_libasan : Flag<["-"], "shared-libasan">;
 def fasm : Flag<["-"], "fasm">, Group<f_Group>;
 
 def fasm_blocks : Flag<["-"], "fasm-blocks">, Group<f_Group>, Flags<[CC1Option]>;
 def fno_asm_blocks : Flag<["-"], "fno-asm-blocks">, Group<f_Group>;
 
 def fassume_sane_operator_new : Flag<["-"], "fassume-sane-operator-new">, Group<f_Group>;
 def fastcp : Flag<["-"], "fastcp">, Group<f_Group>;
 def fastf : Flag<["-"], "fastf">, Group<f_Group>;

include/clang/Driver/SanitizerArgs.h

#include "clang/Basic/Sanitizers.def"
     NeedsUnwindTables = Address | Thread | Memory | DataFlow
   };
   unsigned Kind;
 
   std::string BlacklistFile;
   int MsanTrackOrigins;
   bool AsanZeroBaseShadow;
   bool UbsanTrapOnError;
+  bool AsanSharedRuntime;
 
  public:
   SanitizerArgs();
   /// Parses the sanitizer arguments from an argument list.
   SanitizerArgs(const ToolChain &TC, const llvm::opt::ArgList &Args);
 
   bool needsAsanRt() const { return Kind & NeedsAsanRt; }
+  bool needsSharedAsanRt() const { return AsanSharedRuntime; }
   bool needsTsanRt() const { return Kind & NeedsTsanRt; }
   bool needsMsanRt() const { return Kind & NeedsMsanRt; }
   bool needsLeakDetection() const { return Kind & NeedsLeakDetection; }
   bool needsLsanRt() const {
     return needsLeakDetection() && !needsAsanRt();
   }
   bool needsUbsanRt() const {
     return !UbsanTrapOnError && (Kind & NeedsUbsanRt);

lib/Driver/SanitizerArgs.cpp

 using namespace llvm::opt;
 
 void SanitizerArgs::clear() {
   Kind = 0;
   BlacklistFile = "";
   MsanTrackOrigins = 0;
   AsanZeroBaseShadow = false;
   UbsanTrapOnError = false;
+  AsanSharedRuntime = false;
 }
 
 SanitizerArgs::SanitizerArgs() {
   clear();
 }
 
 SanitizerArgs::SanitizerArgs(const ToolChain &TC,
                              const llvm::opt::ArgList &Args) {
if (Arg *A =
         if (S.getAsInteger(0, MsanTrackOrigins) || MsanTrackOrigins < 0 ||
             MsanTrackOrigins > 2) {
           D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << S;
         }
       }
     }
   }
 
-  if (NeedsAsan)
+  if (NeedsAsan) {
+    AsanSharedRuntime =
+      (TC.getTriple().getEnvironment() == llvm::Triple::Android) ||
+      Args.hasArg(options::OPT_shared_libasan);
     AsanZeroBaseShadow =
         (TC.getTriple().getEnvironment() == llvm::Triple::Android);
   }
+}
 
 void SanitizerArgs::addArgs(const llvm::opt::ArgList &Args,
                             llvm::opt::ArgStringList &CmdArgs) const {
   if (!Kind)
     return;
   SmallString<256> SanitizeOpt("-fsanitize=");
 #define SANITIZER(NAME, ID) \
   if (Kind & ID) \

lib/Driver/Tools.cpp

static void addProfileRT(
 
   SmallString<128> LibProfile = getCompilerRTLibDir(TC);
   llvm::sys::path::append(LibProfile,
       Twine("libclang_rt.profile-") + getArchNameForCompilerRTLib(TC) + ".a");
 
   CmdArgs.push_back(Args.MakeArgString(LibProfile));
 }
 
-static void addSanitizerRTLinkFlags(
-    const ToolChain &TC, const ArgList &Args, ArgStringList &CmdArgs,
-    const StringRef Sanitizer, bool BeforeLibStdCXX,
-    bool ExportSymbols = true) {
-  // Sanitizer runtime has name "libclang_rt.<Sanitizer>-<ArchName>.a".
+static const char *getSanitizerRTLibName(const ToolChain &TC,
+                                         const StringRef Sanitizer,
+                                         const ArgList &Args, bool Shared) {
+  // Sanitizer runtime has name "libclang_rt.<Sanitizer>-<ArchName>.a"
+  // (or "libclang_rt.<Sanitizer>-<ArchName>-android.so for Android)
+  const char *EnvSuffix = 
+    TC.getTriple().getEnvironment() == llvm::Triple::Android ?  "-android" : "";
   SmallString<128> LibSanitizer = getCompilerRTLibDir(TC);
   llvm::sys::path::append(LibSanitizer,
-                          (Twine("libclang_rt.") + Sanitizer + "-" +
-                           getArchNameForCompilerRTLib(TC) + ".a"));
+                          Twine("libclang_rt.") + Sanitizer + "-" +
+                           getArchNameForCompilerRTLib(TC) +
+                           EnvSuffix +
+                           (Shared ? ".so" : ".a"));
+
+  return Args.MakeArgString(LibSanitizer);
+}
+
+static void addSanitizerRTLinkFlags(const ToolChain &TC, const ArgList &Args,
+                                    ArgStringList &CmdArgs,
+                                    const StringRef Sanitizer,
+                                    bool BeforeLibStdCXX,
+                                    bool ExportSymbols = true,
+                                    bool LinkDeps = true) {
+  const char *LibSanitizer = getSanitizerRTLibName(TC, Sanitizer, Args,
+                                                   /*Shared*/ false);
 
   // Sanitizer runtime may need to come before -lstdc++ (or -lc++, libstdc++.a,
   // etc.) so that the linker picks custom versions of the global 'operator
   // new' and 'operator delete' symbols. We take the extreme (but simple)
   // strategy of inserting it at the front of the link command. It also
   // needs to be forced to end up in the executable, so wrap it in
   // whole-archive.
   SmallVector<const char *, 3> LibSanitizerArgs;
   LibSanitizerArgs.push_back("-whole-archive");
-  LibSanitizerArgs.push_back(Args.MakeArgString(LibSanitizer));
+  LibSanitizerArgs.push_back(LibSanitizer);
   LibSanitizerArgs.push_back("-no-whole-archive");
 
   CmdArgs.insert(BeforeLibStdCXX ? CmdArgs.begin() : CmdArgs.end(),
                  LibSanitizerArgs.begin(), LibSanitizerArgs.end());
 
+  if (LinkDeps) {
+    // Link sanitizer dependencies explicitly
     CmdArgs.push_back("-lpthread");
     CmdArgs.push_back("-lrt");
     CmdArgs.push_back("-lm");
     // There's no libdl on FreeBSD.
     if (TC.getTriple().getOS() != llvm::Triple::FreeBSD)
       CmdArgs.push_back("-ldl");
+  }
 
   // If possible, use a dynamic symbols file to export the symbols from the
   // runtime library. If we can't do so, use -export-dynamic instead to export
   // all symbols from the binary.
   if (ExportSymbols) {
-    if (llvm::sys::fs::exists(LibSanitizer + ".syms"))
-      CmdArgs.push_back(
-          Args.MakeArgString("--dynamic-list=" + LibSanitizer + ".syms"));
+    if (llvm::sys::fs::exists(StringRef(LibSanitizer) + ".syms"))
+      CmdArgs.push_back(Args.MakeArgString("--dynamic-list=" +
+                                           StringRef(LibSanitizer) + ".syms"));
     else
       CmdArgs.push_back("-export-dynamic");
   }
 }
 
 /// If AddressSanitizer is enabled, add appropriate linker flags (Linux).
 /// This needs to be called before we add the C run-time (malloc, etc).
 static void addAsanRT(const ToolChain &TC, const ArgList &Args,
-                      ArgStringList &CmdArgs) {
-  if (TC.getTriple().getEnvironment() == llvm::Triple::Android) {
-    SmallString<128> LibAsan = getCompilerRTLibDir(TC);
-    llvm::sys::path::append(LibAsan,
-                            (Twine("libclang_rt.asan-") +
-                             getArchNameForCompilerRTLib(TC) + "-android.so"));
-    CmdArgs.insert(CmdArgs.begin(), Args.MakeArgString(LibAsan));
-  } else {
-    if (!Args.hasArg(options::OPT_shared))
-      addSanitizerRTLinkFlags(TC, Args, CmdArgs, "asan", true);
+                      ArgStringList &CmdArgs, bool Shared) {
+  if (Shared) {
+    // Link dynamic runtime if necessary
+    const char *LibSanitizer = getSanitizerRTLibName(TC, "asan", Args, Shared);
+    CmdArgs.insert(CmdArgs.begin(), LibSanitizer);
+  }
+
+  // Do not link static runtime to DSOs or if compiling for Android
+  bool LinkStaticRT =
+    !Args.hasArg(options::OPT_shared) &&
+    (TC.getTriple().getEnvironment() != llvm::Triple::Android);
+
+  if (LinkStaticRT) {
+    const char *LibAsanStaticPart = Shared ? "asan-preinit" : "asan";
+    addSanitizerRTLinkFlags(TC, Args, CmdArgs, LibAsanStaticPart,
+                            /*BeforeLibStdCXX*/ true, /*ExportSymbols*/ !Shared,
+                            /*LinkDeps*/ !Shared);
   }
 }
 
 /// If ThreadSanitizer is enabled, add appropriate linker flags (Linux).
 /// This needs to be called before we add the C run-time (malloc, etc).
 static void addTsanRT(const ToolChain &TC, const ArgList &Args,
                       ArgStringList &CmdArgs) {
   if (!Args.hasArg(options::OPT_shared))
static void addSanitizerRuntimes(const ToolChain &TC, const ArgList &Args,
                                  ArgStringList &CmdArgs) {
   const SanitizerArgs &Sanitize = TC.getSanitizerArgs();
   const Driver &D = TC.getDriver();
   if (Sanitize.needsUbsanRt())
     addUbsanRT(TC, Args, CmdArgs, D.CCCIsCXX(),
                     Sanitize.needsAsanRt() || Sanitize.needsTsanRt() ||
                     Sanitize.needsMsanRt() || Sanitize.needsLsanRt());
   if (Sanitize.needsAsanRt())
-    addAsanRT(TC, Args, CmdArgs);
+    addAsanRT(TC, Args, CmdArgs, Sanitize.needsSharedAsanRt());
   if (Sanitize.needsTsanRt())
     addTsanRT(TC, Args, CmdArgs);
   if (Sanitize.needsMsanRt())
     addMsanRT(TC, Args, CmdArgs);
   if (Sanitize.needsLsanRt())
     addLsanRT(TC, Args, CmdArgs);
   if (Sanitize.needsDfsanRt())
     addDfsanRT(TC, Args, CmdArgs);

test/Driver/sanitizer-ld.c

 // CHECK-ASAN-LINUX: libclang_rt.asan-i386.a"
 // CHECK-ASAN-LINUX: "-lpthread"
 // CHECK-ASAN-LINUX: "-lrt"
 // CHECK-ASAN-LINUX: "-ldl"
 // CHECK-ASAN-LINUX-NOT: "-export-dynamic"
 // CHECK-ASAN-LINUX: "--dynamic-list={{.*}}libclang_rt.asan-i386.a.syms"
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:     -target i386-unknown-linux -fsanitize=address -shared-libasan \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:     --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-SHARED-ASAN-LINUX %s
+//
+// CHECK-SHARED-ASAN-LINUX: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-SHARED-ASAN-LINUX-NOT: "-lc"
+// CHECK-SHARED-ASAN-LINUX-NOT: libclang_rt.asan-i386.a"
+// CHECK-SHARED-ASAN-LINUX: "-whole-archive" "{{.*}}libclang_rt.asan-preinit-i386.a" "-no-whole-archive"
+// CHECK-SHARED-ASAN-LINUX: libclang_rt.asan-i386.so"
+// CHECK-SHARED-ASAN-LINUX-NOT: "-lpthread"
+// CHECK-SHARED-ASAN-LINUX-NOT: "-lrt"
+// CHECK-SHARED-ASAN-LINUX-NOT: "-ldl"
+// CHECK-SHARED-ASAN-LINUX-NOT: "-export-dynamic"
+// CHECK-SHARED-ASAN-LINUX-NOT: "--dynamic-list"
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.so -shared 2>&1 \
+// RUN:     -target i386-unknown-linux -fsanitize=address -shared-libasan \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:     --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-DSO-SHARED-ASAN-LINUX %s
+//
+// CHECK-DSO-SHARED-ASAN-LINUX: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-DSO-SHARED-ASAN-LINUX-NOT: "-lc"
+// CHECK-DSO-SHARED-ASAN-LINUX-NOT: libclang_rt.asan-i386.a"
+// CHECK-DSO-SHARED-ASAN-LINUX-NOT: "libclang_rt.asan-preinit-i386.a"
+// CHECK-DSO-SHARED-ASAN-LINUX: libclang_rt.asan-i386.so"
+// CHECK-DSO-SHARED-ASAN-LINUX-NOT: "-lpthread"
+// CHECK-DSO-SHARED-ASAN-LINUX-NOT: "-lrt"
+// CHECK-DSO-SHARED-ASAN-LINUX-NOT: "-ldl"
+// CHECK-DSO-SHARED-ASAN-LINUX-NOT: "-export-dynamic"
+// CHECK-DSO-SHARED-ASAN-LINUX-NOT: "--dynamic-list"
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     -target i386-unknown-freebsd -fsanitize=address \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:     --sysroot=%S/Inputs/basic_freebsd_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-FREEBSD %s
 //
 // CHECK-ASAN-FREEBSD: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-ASAN-FREEBSD-NOT: "-lc"
 // CHECK-ASAN-FREEBSD: freebsd{{/|\\+}}libclang_rt.asan-i386.a"

 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     -target armv7l-linux-gnueabi -fsanitize=address \
 // RUN:     --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ARMv7 %s
 //
 // CHECK-ASAN-ARMv7: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-ASAN-ARMv7-NOT: "-lc"
 // CHECK-ASAN-ARMv7: libclang_rt.asan-arm.a"
-//
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     -target arm-linux-androideabi -fsanitize=address \
 // RUN:     --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ANDROID %s
 //
 // CHECK-ASAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-NOT: "-lc"