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 ↗

(On Diff #14844)

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 ↗

(On Diff #14844)

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 ↗

(On Diff #14844)

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 ↗

(On Diff #14844)

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 ↗

(On Diff #14860)

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

lib/Driver/Tools.cpp

3587 ↗

(On Diff #14860)

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 ↗

(On Diff #14877)

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).

cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_pch_not_first_include : Warning<
   "precompiled header '%0' was ignored because '%1' is not first '-include'">;
 def warn_missing_sysroot : Warning<"no such sysroot directory: '%0'">,
   InGroup<DiagGroup<"missing-sysroot">>;
 def warn_debug_compression_unavailable : Warning<"cannot compress debug sections (zlib not installed)">,
   InGroup<DiagGroup<"debug-compression-unavailable">>;
 
 def note_drv_command_failed_diag_msg : Note<
   "diagnostic msg: %0">;
-def note_drv_t_option_is_global :
-  Note<"The last /TC or /TP option takes precedence over earlier instances">;
-  
+def note_drv_t_option_is_global : Note<
+  "The last /TC or /TP option takes precedence over earlier instances">;
+def note_drv_address_sanitizer_debug_runtime : Note<
+  "AddressSanitizer doesn't support linking with debug runtime libraries yet">;
 def err_analyzer_config_no_value : Error<
   "analyzer-config option '%0' has a key but no value">;
 def err_analyzer_config_multiple_values : Error<
   "analyzer-config option '%0' should contain only one '='">;
 
 def err_drv_modules_validate_once_requires_timestamp : Error<
   "option '-fmodules-validate-once-per-build-session' requires "
   "'-fbuild-session-timestamp=<seconds since Epoch>' or '-fbuild-session-file=<file>'">;
 
 def warn_drv_invoking_fallback : Warning<"falling back to %0">,
   InGroup<Fallback>;
 }

cfe/trunk/lib/Driver/SanitizerArgs.cpp

if (Arg *A =
             Args.getLastArg(options::OPT_fsanitize_address_field_padding)) {
         StringRef S = A->getValue();
         // Legal values are 0 and 1, 2, but in future we may add more levels.
         if (S.getAsInteger(0, AsanFieldPadding) || AsanFieldPadding < 0 ||
             AsanFieldPadding > 2) {
           D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << S;
         }
     }
+
+    if (Arg *WindowsDebugRTArg =
+            Args.getLastArg(options::OPT__SLASH_MTd, options::OPT__SLASH_MT,
+                            options::OPT__SLASH_MDd, options::OPT__SLASH_MD,
+                            options::OPT__SLASH_LDd, options::OPT__SLASH_LD)) {
+      switch (WindowsDebugRTArg->getOption().getID()) {
+      case options::OPT__SLASH_MTd:
+      case options::OPT__SLASH_MDd:
+      case options::OPT__SLASH_LDd:
+        D.Diag(diag::err_drv_argument_not_allowed_with)
+            << WindowsDebugRTArg->getAsString(Args)
+            << lastArgumentForKind(D, Args, NeedsAsanRt);
+        D.Diag(diag::note_drv_address_sanitizer_debug_runtime);
+      }
+    }
   }
 
   // Parse -link-cxx-sanitizer flag.
   LinkCXXRuntimes =
       Args.hasArg(options::OPT_fsanitize_link_cxx_runtime) || D.CCCIsCXX();
 }
 
 void SanitizerArgs::addArgs(const llvm::opt::ArgList &Args,

cfe/trunk/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
+// RUN: %clang_cl -fsanitize=address -c -MD -MDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
+// RUN: %clang_cl -fsanitize=address -c -MT -MTd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
+// RUN: %clang_cl -fsanitize=address -c -LD -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
+// RUN: %clang_cl -fsanitize=address -c -MTd -MT -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-RELEASERTL
+// RUN: %clang_cl -fsanitize=address -c -MDd -MD -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-RELEASERTL
+// RUN: %clang_cl -fsanitize=address -c -LDd -LD -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-RELEASERTL
+// CHECK-ASAN-RELEASERTL-NOT: error: invalid argument