[Driver] [Darwin] Add sanitizer libraries even if -nodefaultlibs is passed

[beanz]

Summary

The -nodefaultlibs and -nostdlib flags suppress all the runtime libraries that the driver puts on the link line. This feels wrong. If a user specifies "-fsanitize=<blah>" I think it is expected that the sanitizer library would be included on the link line.

[bruno]

Regardless of the way we decide to go with this, it would be nice if a driver level warning is used to tell users about any non-obvious assumed behavior when these flags are used together.

[beanz]

I've sent out an additional patch that adds a warning for using -nostdlibs and -fsanitize (See: D24091)

[zaks.anna]

-nostdlib is often used to build parts of libsystem. It's worth noting that ASan and TSan are not supported for use on libsystem on darwin (and elsewhere?), though some subcomponents of it can be sanitized. I am not sure how this relates to UBSan.

The user experience of not passing the liker flag is confusing. One proposed solution is to have the clang driver error out when -fsanitize=address and -nostdlib are used together. I am not sure if the warning is sufficient. The error/warning should say that this combination is unsupported at least on Darwin.

Chris, what is the driver behind this?

Kuba, what do you think?

No tests?

[beanz]

@zaks.anna, the driver behind this is supporting building libcxx with sanitizers. The drivers for GNUTools and FreeBSD already support this workflow.

I have updates to the test in D24091, which also cover this code. I'll push the updated test shortly.

[gribozavr]

If a user specifies "-fsanitize=<blah>" I think it is expected that the sanitizer library would be included on the link line.

I deeply disagree.

If the user asks for -nodefaultlibs, we should not add any libraries ourselves, because (a) this is what they asked for, and (b) they probably know what they are doing if they are using such a low-level flag. (And they are probably doing something deeply system-specific and they can handle adding more low-level compiler options.)

There are issues with the behavior you are proposing. If -nodefaultlibs adds some runtime libs, but not others, it is:

• confusing,
• inconsistent -- users can't predict the behavior of a given set of compiler options. How would you document -nodefaultlibs with the semantics that you propose without explicitly explaining this special-case interaction with sanitizers? If you can't, then it means that the design is not the right, intuitive one.
• The proposed semantics lead to an immediate feature request for a -really-nodefaultlibs option that does not add sanitizer libraries,
• The proposed semantics add to an existing mess of the random special-case driver behavior.

So the question becomes, what are you trying to do? If you want to allow users to easily link in the sanitizer runtime even if they specify -nodefaultlibs, I think the user should explicitly opt into that via another driver flag (e.g., clang -nodefaultlibs -fsanitize=address -flink-sanitizer-runtime=address). Then the command line is explicit about the intent, and each flag has the obvious meaning.

[beanz]

To address both @bogner and @gribozavr...

The problem is we're wildly inconsistent on what -nodefaultlibs and -nostdlibs mean.

For example, on GNUTools or FreeBSD -fsanitize *always* adds sanitizer libraries regardless of the presence of the -no* flags. On Darwin we're even inconsistent among the sanitizers. The safestack library is added regardless of the flags, but the other sanitizers are not.

To answer @bogner's question about the PGO libraries, they are universally always added regardless of the -no* flags. That behavior is at least consistent across all driver implementations.

Agnostic to the discussion about a warning, I feel that this change (whether you agree with it or not) is an improvement because it at least makes the behavior of -fsanitize and the -no* flags consistent across all Driver implementations. That is significantly better than where we are today.

And in case you want to argue that maybe we should change the other drivers to match Darwin, @filcab pointed out the last attempt at doing that was in rL218541, and it was later reverted (rL220455) because we rely on that behavior in libcxx. The path of least resistance to consistency is to make Darwin behave like GNUTools and FreeBSD.

[zaks.anna]

I've added kcc as a reviewer to see what his opinion is.

The way I see this, is that the sanitizer flags and the -nodefaultlibs and -nostdlib flags are not fully compatible since sanitizers will not work for some users who explicitly pass the "-no*" flags.

libcxx happens to work, however, it's just one of the users and it has a much better representation on this list since it's an LLVM project. I do not think this is a blocker for sanitizing libcxx because we could change the build system to explicitly link in the ASan library, correct?

In any case, we would need some type of diagnostic in the driver to notify the user about the incompatibility. We just need to decide which default to pick.

[kcc]

Sorry, I won't have a chance to look at it before late next week.
Adding two more folks in case they have ideas.

[beanz]

@zaks.anna, changing the build system to explicitly select which sanitizer runtime to use is more complicated than it might seem. The sanitizer runtime libraries are named differently by toolchain, so correctly selecting a library would involve either duplicating complicated compiler driver logic, or invoking the compiler to get it to report which library it would use. Both of which are fragile.

If the build system needs to work around this, it can, but it would be much better if we didn't need to.

[eugenis]

I would also expect -nodefaultlibs and -nostdlib to remove all standard libraries from the link command line, including the sanitizer ones.

I like the idea of -flink-sanitizer-runtime=address, but may be without "address" - the set of sanitizer runtime libraries can be found from -fsanitize=* flags.

Alternatively, we could extend (or add something like) -print-libgcc-name to report the set of sanitizer link flags (there may be multiple libraries and possible other flags).

[vitalybuka]

I have no strong opinion on that. There is already exception for startfiles, so maybe sanitizes are OK as well.

[beanz]

• Added new driver flag -flink-sanitizer-runtimes which forces linking sanitizer runtimes.

Additional cleanup will be required to support the GNUTools and FreeBSD drivers as well as making the Darwin behavior more consistent. I will update the patch with additional changes if this approach looks acceptable.

[rsmith]

My 2c: -nodefaultlibs means "don't link against any libraries I didn't explicitly tell you to". -fsanitize=* as a driver argument *when linking* is an explicit request to link against the sanitizer runtimes. So that should win. If you don't want to link against the sanitizer runtimes, link without the -fsanitize= flag, or append -fno-sanitize=all to the link line.

[zaks.anna]

-fsanitize=* as a driver argument *when linking* is an explicit request to link against the sanitizer runtimes.

Sanitizer users pass this option to the clang driver to get the runtime checking. Not all of them understand the implications and immediately realize that extra libraries will be linked in (which might be incompatible with other options they pass) or at least they are not thinking about it when using the feature. So I do not view this as an *explicit request* to link against the extra libraries. (I might be missing the point you are trying to convey by highlighting *when linking*. When this option is passed, we both compile and link differently. I view this as an option to turn on the sanitizers feature as a whole.)

We saw several occurrences of people passing this option to clang when building libsystem components and not realizing why there is a problem at link time. This is not a great experience, but happy linking and running into problems at run time would be worse.

[kubamracek]

Anyone who uses -nostdlib or -nodefaultlibs has to be aware that his program will be linked with less stuff than usual and has to expect linker error. If such a user doesn’t know that ASan and TSan require a dylib on macOS, he should be able to figure this out from the error/warning.

That being said, I am in favor for the current patch (don’t link sanitizer runtime automatically with -nostdlib, but do so with an explicit -flink-sanitizer-runtimes), plus an explicit warning when using -nostdlib and -fsanitize=... together without -flink-sanitizer-runtimes, which should suggest to use the new option.

Could we consider renaming -flink-sanitizer-runtimes to something that starts with -fsanitize? All current sanitizer flags start with -fsanitize, they never use the word sanitize/sanitizer in the middle. We also already have -fsanitize-link-c++-runtime.

[beanz]

• Updated with FreeBSD and GNUTools support for -flink-sanitizer-runtimes

[beanz]

There are basically two solutions being debated. Either make -fsanitize=... bypass -nostdlib -nodefaultlibs or support a flag that explicitly bypasses it.

Personally I think the flag is probably the better way to go because it is more explicit, and has less implied behavior. If we go with this solution I will add a check to libcxx to detect if the compiler supports the flag and add it to sanitizer builds before committing the support to clang. This should prevent any breakages.

@kubabrecka, I can understand where you're coming from about the option starting with -fsanitize, but I disagree for two reasons. First, I think that it is more important for the option to be concise and clear about what it means, and -flink-sanitizer-runtimes seems pretty clear to me. Second, the existing -fsanitize options all refer to the compiler inserted runtime checks, where as this refers to the driver behavior, so I think there is a difference between the intention of the flags that justifies them not conforming to the same format.

I also have a small problem with @rsmith's point about -fsanitize being an explicit request when linking. I get very nervous about the idea of a flag having different meaning when compiling from when linking. While I understand that it doesn't make sense for -fsanitize options to not imply linking the runtimes, I think there is a good argument that they're not an explicit request for the runtime libraries.

Either way I don't really care which set of patches we go with. At this point the current diff has a functional -flink-sanitizer-runtimes flag supported across all drivers, and the original patch modified the Darwin behavior to match FreeBSD and GNUTools.

Thoughts anyone?

[rsmith]

In D24048#537257, @beanz wrote:

@kubabrecka, I can understand where you're coming from about the option starting with -fsanitize, but I disagree for two reasons. First, I think that it is more important for the option to be concise and clear about what it means, and -flink-sanitizer-runtimes seems pretty clear to me. Second, the existing -fsanitize options all refer to the compiler inserted runtime checks, where as this refers to the driver behavior, so I think there is a difference between the intention of the flags that justifies them not conforming to the same format.

Your claim about the existing -fsanitize options seems off the mark -- at least -fsanitize= and fsanitize-link-c++-runtime already affect the driver linking behaviour, not just the compiler-inserted runtime checks. And more generally, we're trying to group our driver options based on prefixes, so -flink-sanitizer-runtimes is not appropriate on that basis.

I also have a small problem with @rsmith's point about -fsanitize being an explicit request when linking. I get very nervous about the idea of a flag having different meaning when compiling from when linking. While I understand that it doesn't make sense for -fsanitize options to not imply linking the runtimes, I think there is a good argument that they're not an explicit request for the runtime libraries.

I think it depends a lot on how you set up your build system. If you have one set of flags for compilation steps and one set of flags for link steps, then adding -fsanitize= to the link flags is pretty clearly requesting that you link against the sanitizer runtimes. This is, in fact, the documented meaning of passing -fsanitize= when linking (http://clang.llvm.org/docs/UsersManual.html#controlling-code-generation).

If, on the other hand, you have a common set of flags passed to both compilations and links, then perhaps you could argue that you're not explicitly opting in. However, as there are a large number of Clang compilation flags (-f flags in particular) that will cause the link to fail if they're passed to the driver when linking, someone will have needed to choose to add the flags to the "both compile and link" flag set to get into this state (rather than adding them just to the "compile" flag set, which should be the default choice), and again this seems like an explicit statement of intent.

I don't see the point of adding another flag to control this when we already have a perfectly good set of flags that already do the right thing -- that takes us three levels deep in flags overriding the behavior of other flags, and I don't see how it actually helps our users in any way.

[zaks.anna]

I don't see the point of adding another flag to control this when we already have a perfectly good set of
flags that already do the right thing -- that takes us three levels deep in flags overriding the behavior of
other flags, and I don't see how it actually helps our users in any way.

Going with silently linking the sanitizer runtimes when -nostdlib is passed will cause regressions in user experience. They will start getting inexplicable run time failures instead of build failures. Which solution do you propose to fix this problem?

This specific situation comes up often on internal mailing lists, so we should not go for a solution that regresses the behavior on Darwin.

include/clang/Driver/Options.td

def fsanitize_stats : Flag<["-"], "fsanitize-stats">,
                               HelpText<"Enable sanitizer statistics gathering.">;
 def fno_sanitize_stats : Flag<["-"], "fno-sanitize-stats">,
                                  Group<f_clang_Group>, Flags<[CC1Option]>,
                                  HelpText<"Disable sanitizer statistics gathering.">;
 def fsanitize_undefined_strip_path_components_EQ : Joined<["-"], "fsanitize-undefined-strip-path-components=">,
   Group<f_clang_Group>, Flags<[CC1Option]>, MetaVarName<"<number>">,
   HelpText<"Strip (or keep only, if negative) a given number of path components "
            "when emitting check metadata.">;
+def flink_sanitizer_runtimes : Flag<["-"], "flink-sanitizer-runtimes">,
+  Group<f_Group>, Flags<[DriverOption]>,
+  HelpText<"Force linking sanitizer runtime libraries even if -nostdlib or "
+  "-nodefaultlibs is passed.">;
 def funsafe_math_optimizations : Flag<["-"], "funsafe-math-optimizations">,
   Group<f_Group>;
 def fno_unsafe_math_optimizations : Flag<["-"], "fno-unsafe-math-optimizations">,
   Group<f_Group>;
 def fassociative_math : Flag<["-"], "fassociative-math">, Group<f_Group>;
 def fno_associative_math : Flag<["-"], "fno-associative-math">, Group<f_Group>;
 def freciprocal_math :
   Flag<["-"], "freciprocal-math">, Group<f_Group>, Flags<[CC1Option]>,

lib/Driver/ToolChains.h

public:
   /// Add the linker arguments to link the ARC runtime library.
   virtual void AddLinkARCArgs(const llvm::opt::ArgList &Args,
                               llvm::opt::ArgStringList &CmdArgs) const {}
 
   /// Add the linker arguments to link the compiler runtime library.
   virtual void AddLinkRuntimeLibArgs(const llvm::opt::ArgList &Args,
                                      llvm::opt::ArgStringList &CmdArgs) const;
 
+  virtual void
+  AddLinkSanitizerLibArgs(const llvm::opt::ArgList &Args,
+                          llvm::opt::ArgStringList &CmdArgs) const {}
+
   virtual void addStartObjectFileArgs(const llvm::opt::ArgList &Args,
                                       llvm::opt::ArgStringList &CmdArgs) const {
   }
 
   virtual void addMinVersionArgs(const llvm::opt::ArgList &Args,
                                  llvm::opt::ArgStringList &CmdArgs) const {}
 
   /// On some iOS platforms, kernel and kernel modules were built statically. Is
public:
   DarwinClang(const Driver &D, const llvm::Triple &Triple,
               const llvm::opt::ArgList &Args);
 
   /// @name Apple ToolChain Implementation
   /// {
 
   RuntimeLibType GetRuntimeLibType(const llvm::opt::ArgList &Args) const override;
 
+  void
+  AddLinkSanitizerLibArgs(const llvm::opt::ArgList &Args,
+                          llvm::opt::ArgStringList &CmdArgs) const override;
+
   void AddLinkRuntimeLibArgs(const llvm::opt::ArgList &Args,
                              llvm::opt::ArgStringList &CmdArgs) const override;
 
   void AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
                            llvm::opt::ArgStringList &CmdArgs) const override;
 
   void AddCCKextLibArgs(const llvm::opt::ArgList &Args,
                         llvm::opt::ArgStringList &CmdArgs) const override;

lib/Driver/ToolChains.cpp

void DarwinClang::AddLinkRuntimeLibArgs(const ArgList &Args,
   // Reject -static-libgcc for now, we can deal with this when and if someone
   // cares. This is useful in situations where someone wants to statically link
   // something like libstdc++, and needs its runtime support routines.
   if (const Arg *A = Args.getLastArg(options::OPT_static_libgcc)) {
     getDriver().Diag(diag::err_drv_unsupported_opt) << A->getAsString(Args);
     return;
   }
 
-  const SanitizerArgs &Sanitize = getSanitizerArgs();
-  if (Sanitize.needsAsanRt())
-    AddLinkSanitizerLibArgs(Args, CmdArgs, "asan");
-  if (Sanitize.needsUbsanRt())
-    AddLinkSanitizerLibArgs(Args, CmdArgs, "ubsan");
-  if (Sanitize.needsTsanRt())
-    AddLinkSanitizerLibArgs(Args, CmdArgs, "tsan");
-  if (Sanitize.needsStatsRt()) {
-    StringRef OS = isTargetMacOS() ? "osx" : "iossim";
-    AddLinkRuntimeLib(Args, CmdArgs,
-                      (Twine("libclang_rt.stats_client_") + OS + ".a").str(),
-                      /*AlwaysLink=*/true);
-    AddLinkSanitizerLibArgs(Args, CmdArgs, "stats");
-  }
-  if (Sanitize.needsEsanRt())
-    AddLinkSanitizerLibArgs(Args, CmdArgs, "esan");
+  AddLinkSanitizerLibArgs(Args, CmdArgs);
 
   // Otherwise link libSystem, then the dynamic runtime library, and finally any
   // target specific static runtime library.
   CmdArgs.push_back("-lSystem");
 
   // Select the dynamic runtime library and the target specific static library.
   if (isTargetWatchOSBased()) {
     // We currently always need a static runtime library for watchOS.
if (isTargetWatchOSBased()) {
     // the OS X builtins library.
     if (isMacosxVersionLT(10, 5))
       AddLinkRuntimeLib(Args, CmdArgs, "libclang_rt.10.4.a");
     else
       AddLinkRuntimeLib(Args, CmdArgs, "libclang_rt.osx.a");
   }
 }
 
+void DarwinClang::AddLinkSanitizerLibArgs(
+    const llvm::opt::ArgList &Args, llvm::opt::ArgStringList &CmdArgs) const {
+  const SanitizerArgs &Sanitize = getSanitizerArgs();
+  if (Sanitize.needsAsanRt())
+    AddLinkSanitizerLibArgs(Args, CmdArgs, "asan");
+  if (Sanitize.needsUbsanRt())
+    AddLinkSanitizerLibArgs(Args, CmdArgs, "ubsan");
+  if (Sanitize.needsTsanRt())
+    AddLinkSanitizerLibArgs(Args, CmdArgs, "tsan");
+  if (Sanitize.needsStatsRt()) {
+    StringRef OS = isTargetMacOS() ? "osx" : "iossim";
+    AddLinkRuntimeLib(Args, CmdArgs,
+                      (Twine("libclang_rt.stats_client_") + OS + ".a").str(),
+                      /*AlwaysLink=*/true);
+    AddLinkSanitizerLibArgs(Args, CmdArgs, "stats");
+  }
+  if (Sanitize.needsEsanRt())
+    AddLinkSanitizerLibArgs(Args, CmdArgs, "esan");
+}
+
 void Darwin::AddDeploymentTarget(DerivedArgList &Args) const {
   const OptTable &Opts = getDriver().getOpts();
 
   // Support allowing the SDKROOT environment variable used by xcrun and other
   // Xcode tools to define the default sysroot, by making it the default for
   // isysroot.
   if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
     // Warn if the path does not exist.

lib/Driver/Tools.cpp

void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
     if (getToolChain().getDriver().CCCIsCXX())
       getToolChain().AddCXXStdlibLibArgs(Args, CmdArgs);
 
     // link_ssp spec is empty.
 
     // Let the tool chain choose which runtime library to link.
     getMachOToolChain().AddLinkRuntimeLibArgs(Args, CmdArgs);
+  } else if (Args.hasArg(options::OPT_flink_sanitizer_runtimes)) {
+    getMachOToolChain().AddLinkSanitizerLibArgs(Args, CmdArgs);
   }
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles)) {
     // endfile_spec is empty.
   }
 
   Args.AddAllArgs(CmdArgs, options::OPT_T_Group);
   Args.AddAllArgs(CmdArgs, options::OPT_F);

test/Driver/nodefaultlib.c

 // RUN: %clang -target i686-pc-linux-gnu -### -nodefaultlibs %s 2>&1 | FileCheck -check-prefix=TEST1 %s
 // TEST1-NOT: start-group
 // TEST1-NOT: "-lgcc"
 // TEST1-NOT: "-lc"
 // TEST1: crtbegin
 // TEST1: crtend
 
 // RUN: %clang -target i686-pc-linux-gnu -stdlib=libc++ -nodefaultlibs -lstdc++ -### %s 2>&1 | FileCheck -check-prefix=TEST2 %s
 // TEST2-NOT: "-lc++"
 // TEST2: "-lstdc++"
+
+// RUN: %clang -target i686-apple-darwin -nodefaultlibs -fsanitize=address -### %s 2>&1 | FileCheck -check-prefix=DARWIN_SAN_LIB1 %s
+// DARWIN_SAN_LIB1-NOT: libclang_rt.asan_osx_dynamic.dylib
+
+// RUN: %clang -target i686-apple-darwin -nodefaultlibs -fsanitize=address -flink-sanitizer-runtimes -### %s 2>&1 | FileCheck -check-prefix=DARWIN_SAN_LIB2 %s
+// DARWIN_SAN_LIB2: libclang_rt.asan_osx_dynamic.dylib

test/Driver/nostdlib.c

 //
 // RUN: %clang -target x86_64-pc-windows-msvc -nostdlib --rtlib=compiler-rt -### %s 2>&1 | FileCheck %s -check-prefix CHECK-MSVC-NOSTDLIB
 // RUN: %clang -target x86_64-pc-windows-msvc --rtlib=compiler-rt -nostdlib -### %s 2>&1 | FileCheck %s -check-prefix CHECK-MSVC-NOSTDLIB
 //
 // CHECK-LINUX-NOSTDLIB: warning: argument unused during compilation: '--rtlib=compiler-rt'
 // CHECK-LINUX-NOSTDLIB: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-LINUX-NOSTDLIB-NOT: "{{.*}}/Inputs/resource_dir{{/|\\\\}}lib{{/|\\\\}}linux{{/|\\\\}}libclang_rt.builtins-i686.a"
 // CHECK-MSVC-NOSTDLIB: warning: argument unused during compilation: '--rtlib=compiler-rt'
+
+// RUN: %clang -target i686-apple-darwin -nostdlib -fsanitize=address -### %s 2>&1 | FileCheck -check-prefix=DARWIN_SAN_LIB1 %s
+// DARWIN_SAN_LIB1-NOT: libclang_rt.asan_osx_dynamic.dylib
+
+// RUN: %clang -target i686-apple-darwin -nostdlib -fsanitize=address -flink-sanitizer-runtimes -### %s 2>&1 | FileCheck -check-prefix=DARWIN_SAN_LIB2 %s
+// DARWIN_SAN_LIB2: libclang_rt.asan_osx_dynamic.dylib