clang-cl: Diagnose the usage of ASAN with a debug runtime library

[• ehsan]

Summary

AddressSanitizer currently doesn't support this configuration, and binaries
built with it will just get into an infinite loop during startup.

[samsonov]

lib/Driver/Tools.cpp

3590

Why not use err_drv_argument_not_allowed_with here and show that "-fsanitize=address not allowed with /MTd" or smth. like that?

[• ehsan]

lib/Driver/Tools.cpp

3590

Well, technically there is nothing wrong with using these two arguments together. I tried to make it clear in the diagnostic that this is something that is just not supported yet. Do you think that's reasonable?

[samsonov]

Then I'd vote for more generic driver error type: err_drv_argument_not_supported_with (with a slightly different wording than err_drv_argument_not_allowed_with).

[timurrrr]

I'd defer to Hans, he knows this code better.

I'm fine with the general direction.

include/clang/Basic/DiagnosticDriverKinds.td

89

The tool is called "AddressSanitizer", not "address sanitizer".

I'd prefer something line

"AddressSanitizer doesn't support linking with debug runtime libraries yet"

[timurrrr]

test/Driver/fsanitize.c

161

I think you can do something like

// RUN: ... -MDd ... | FileCheck %s -check-prefix=CHECK-ASAN-DEBUG-RTL
// RUN: ... -MTd ... | FileCheck %s -check-prefix=CHECK-ASAN-DEBUG-RTL
// RUN: ... -LDd ... | FileCheck %s -check-prefix=CHECK-ASAN-DEBUG-RTL
// CHECK-ASAN-DEBUG-RTL: <warning message>

// RUN: ...
// RUN: ...
// RUN: ...
// CHECK-ASAN-RELEASE-RTL-NOTE: <warning message>

[• ehsan]

Addressed the review comments.

[samsonov]

include/clang/Basic/DiagnosticDriverKinds.td

88

Please put this note next to another notes defined in this file.

lib/Driver/Tools.cpp

3587

Generally we parse sanitizer arguments and print the error diagnostics in SanitizerArgs constructor (lib/Driver/SanitizerArgs.cpp). For example, that file is the place where we form error messages about incompatible sanitizer runtimes. I think, it makes sense to put these checks there as well to keep it consistent. You can also use "SanitizerArgs::lastArgumentForKind" method there.

[hans]

I'd defer to Hans, he knows this code better.

Seems like samsonov is on it.

[• ehsan]

Addressed the review comments.

[timurrrr]

Ah, right.
Please stay in the loop for windows-specific stuff.
14 окт. 2014 г. 22:15 пользователь "Hans Wennborg" <[e-mail]>
написал:

I'd defer to Hans, he knows this code better.

Seems like samsonov is on it.

http://reviews.llvm.org/D5764

[samsonov]

lib/Driver/SanitizerArgs.cpp

178 ↗

(On Diff #14877)

Can you use the following syntax here?

if (Arg *WindowsDebugRTArg = Args.getLastArg(options::OPT__SLASH_MTd, options::OPT__SLASH_MDd,
    options::OPT__SLASH_LDd)) {
  D.Diag(...) << ....;
}

Also, what is MSVC/clang-cl semantics if I pass "/MTd /MT"? Will it pick up the last flag (/MT), or try to link both runtimes?
In the first case we don't need to produce the error.

test/Driver/fsanitize.c

167

This line is enough here.

[• ehsan]

lib/Driver/SanitizerArgs.cpp

178 ↗

(On Diff #14877)

When passing "/MTd /MT", /MTd will be ignored. clang-cl honors that too. I should actually add a test case for that too.

[• ehsan]

Addressed the review comments.

[rnk]

lib/Driver/SanitizerArgs.cpp

178 ↗

(On Diff #14877)

MSVC generally implements "last flag wins" for conflicting flags, with some exceptions for things like /EH. They warn on it, but we don't because it's been the behavior in gcc for a long time.

[samsonov]

LGTM. Please run the patch (.cpp file change) through clang-format-diff before submitting.

[• ehsan]

Closed by commit rL219744 (authored by @ehsan).

include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_force_crash : Error<
   "failing because environment variable '%0' is set">;
 def err_drv_invalid_mfloat_abi : Error<
   "invalid float ABI '%0'">;
 def err_drv_invalid_libcxx_deployment : Error<
   "invalid deployment target for -stdlib=libc++ (requires %0 or later)">;
 def err_drv_malformed_sanitizer_blacklist : Error<
   "malformed sanitizer blacklist: '%0'">;
+def note_drv_address_sanitizer_debug_runtime : Note<
+  "AddressSanitizer doesn't support linking with debug runtime libraries yet">;
 
 def err_target_unsupported_arch
   : Error<"the target architecture '%0' is not supported by the target '%1'">;
 
 def err_drv_I_dash_not_supported : Error<
   "'%0' not supported, please use -iquote instead">;
 def err_drv_unknown_argument : Error<"unknown argument: '%0'">;
 def err_drv_invalid_value : Error<"invalid value '%1' in '%0'">;

lib/Driver/Tools.cpp

   // AltiVec language extensions aren't relevant for assembling.
   if (!isa<PreprocessJobAction>(JA) || 
       Output.getType() != types::TY_PP_Asm)
     Args.AddLastArg(CmdArgs, options::OPT_faltivec);
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_show_template_tree);
   Args.AddLastArg(CmdArgs, options::OPT_fno_elide_type);
 
   const SanitizerArgs &Sanitize = getToolChain().getSanitizerArgs();
+  if (Sanitize.needsAsanRt()) {
+    Arg *DebugRTArg = nullptr;
+    if (Args.hasArg(options::OPT__SLASH_MTd))
+      DebugRTArg = Args.getLastArg(options::OPT__SLASH_MTd);
+    if (Args.hasArg(options::OPT__SLASH_MDd))
+      DebugRTArg = Args.getLastArg(options::OPT__SLASH_MDd);
+    if (Args.hasArg(options::OPT__SLASH_LDd))
+      DebugRTArg = Args.getLastArg(options::OPT__SLASH_LDd);
+    if (DebugRTArg) {
+      D.Diag(diag::err_drv_argument_not_allowed_with)
+        << DebugRTArg->getAsString(Args)
+        << Args.getLastArg(options::OPT_fsanitize_EQ)->getAsString(Args);
+      D.Diag(diag::note_drv_address_sanitizer_debug_runtime);
+    }
+  }
   Sanitize.addArgs(Args, CmdArgs);
 
   if (!Args.hasFlag(options::OPT_fsanitize_recover,
                     options::OPT_fno_sanitize_recover,
                     true))
     CmdArgs.push_back("-fno-sanitize-recover");
 
   if (Args.hasFlag(options::OPT_fsanitize_undefined_trap_on_error,

test/Driver/fsanitize.c

 // CHECK-TSAN-MSAN-MSAN-DARWIN: unsupported option '-fsanitize=thread' for target 'x86_64-apple-darwin10'
 // CHECK-TSAN-MSAN-MSAN-DARWIN-NOT: unsupported option
 
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=function %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FSAN-DARWIN
 // CHECK-FSAN-DARWIN: unsupported option '-fsanitize=function' for target 'x86_64-apple-darwin10'
 
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=function -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FSAN-UBSAN-DARWIN
 // CHECK-FSAN-UBSAN-DARWIN: unsupported option '-fsanitize=function' for target 'x86_64-apple-darwin10'
+
+// RUN: %clang_cl -fsanitize=address -c -MDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
+// RUN: %clang_cl -fsanitize=address -c -MTd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
+// RUN: %clang_cl -fsanitize=address -c -LDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
+// CHECK-ASAN-DEBUGRTL: error: invalid argument
+// CHECK-ASAN-DEBUGRTL: not allowed with '-fsanitize=address'
+// CHECK-ASAN-DEBUGRTL: note: AddressSanitizer doesn't support linking with debug runtime libraries yet
+
+// RUN: %clang_cl -fsanitize=address -c -MT -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-RELEASERTL
+// RUN: %clang_cl -fsanitize=address -c -MD -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-RELEASERTL
+// RUN: %clang_cl -fsanitize=address -c -LD -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-RELEASERTL
+// CHECK-ASAN-RELEASERTL-NOT: error: invalid argument
+// CHECK-ASAN-RELEASERTL-NOT: not allowed with '-fsanitize=address'
+// CHECK-ASAN-RELEASERTL-NOT: note: AddressSanitizer doesn't support linking with debug runtime libraries yet