Add asan support for MSVC debug runtimes

[lo1ol]

Summary

Use special debug asan versions (prefixed with asan_dbg) for MSVC debug runtimes (/MTd, /MDd, /LDd).

Resolves: https://github.com/llvm/llvm-project/compare/main...lo1ol:asan_dbg?expand=1

[rnk]

I'm hesitant to do this. The main reason we disabled the use of the debug runtimes was that the debug runtimes interfered with malloc interception, which leads to crashes and a generally poor user experience. I'd like some confirmation beyond just your word that this is addressed.

After that, this is kind of scary because we don't build and ship these "dbg" variants of the ASan runtime (so far as I know). They are only provided as part of the MSVC CRT. Is LLVM ASan actually compatible with the ASan runtimes provided by MSVC? I'm guessing the answer that things appear to work, but so far as I know, there is no usptream support for this. If things break, there's nobody who will fix them. I have reached out to Microsoft to ask them to help contribute upstream, but so far I haven't seen major contributions.

This change is one small step towards encouraging users to combine clang-cl with the Microsoft provided ASan runtimes, and I'm not sure we want to guide users in that direction. We only promise interop with compiler runtime libraries that we vend.

Other stakeholders: +@cbezault @vitalybuka

clang/lib/Driver/SanitizerArgs.cpp

829

This is the only usage of this note, and if you remove it, the note should be removed as well.

[cbezault]

Imo I agree that this shouldn’t be merged until the debug variants of the asan runtime are getting built publicly.

[lo1ol]

Ok, I get current situation.

Sorry for my late answer. I made some investigation yesterday and compared msvc and clang versions of asan. In my tests msvc version seems more stable.

All my tests is written via catch2 framework (v2.13.8). So, first example is:

#define CATCH_CONFIG_MAIN

#include <catch2/catch.hpp>

#include <iostream>

TEST_CASE( "kek" ) {
        for (int i=0; i < 10; ++i) {
                printf("lol ke\n");
                free(malloc(4472));
        }

}

If compiles it via msvc version of asan, everything is ok:

# copy  MSVC\14.29.30133\lib\x64 to Llvm\x64\lib\clang\12.0.0\lib\windows before
$ clang-cl kek.cpp -I ..\Catch2\single_include /MT -fsanitize=address
$ kek.exe
lol ke
lol ke
lol ke
lol ke
lol ke
lol ke
lol ke
lol ke
lol ke
lol ke
===============================================================================
test cases: 1 | 1 passed
assertions: - none -

But version for clang's toolchain has strange behavior:

# With original content of Llvm\x64\lib\clang\12.0.0\lib\windows
$ clang-cl kek.cpp -I ..\Catch2\single_include /MT -fsanitize=address
$ kek.exe
lol ke
lol ke
lol ke
lol ke
lol ke

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...

Nevertheless, I found some bug which presents inside both versions

#define CATCH_CONFIG_MAIN

#include <catch2/catch.hpp>

#include <iostream>
#include <future>

TEST_CASE( "kek" ) {
        auto result = std::async(std::launch::async, []() {
                        std::cerr << "kek " << std::endl;
                });

        result.get();
}

The output is same for both versions:

$ clang-cl kek.cpp -I ..\Catch2\single_include /MT -fsanitize=address
$ kek.exe

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...

I don't have a time to discover a root of problem. But msvc's version of asan seems to me more stable and compatible with clang's asan. So, may be there is point to distribute same version of asan for both toolchains.

If you want, I may create an issue for founded bugs.

[DanWillans]

Hi all,

Has there been any progress on this? I want to run clang-tidy on some code being compiled in debug with MSVC but I hit the clang-diagnostic-error note 'D.Diag(clang::diag::note_drv_address_sanitizer_debug_runtime)'. Later versions of MSVC support ASan with debug runtime now. Maybe I'm doing something wrong?

[rnk]

+ @mstorsjo @thieta

Since we were talking about the complexity involved in choosing the correct ASan runtime, there is actually even more involved.

clang/lib/Driver/SanitizerArgs.cpp

if (Arg *A =
         // Legal values are 0 and 1, 2, but in future we may add more levels.
         if ((S.getAsInteger(0, AsanFieldPadding) || AsanFieldPadding < 0 ||
              AsanFieldPadding > 2) &&
             DiagnoseErrors) {
           D.Diag(clang::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:
-        if (DiagnoseErrors) {
-          D.Diag(clang::diag::err_drv_argument_not_allowed_with)
-              << WindowsDebugRTArg->getAsString(Args)
-              << lastArgumentForMask(D, Args, SanitizerKind::Address);
-          D.Diag(clang::diag::note_drv_address_sanitizer_debug_runtime);
-        }
-      }
-    }
-
     AsanUseAfterScope = Args.hasFlag(
         options::OPT_fsanitize_address_use_after_scope,
         options::OPT_fno_sanitize_address_use_after_scope, AsanUseAfterScope);
 
     AsanPoisonCustomArrayCookie = Args.hasFlag(
         options::OPT_fsanitize_address_poison_custom_array_cookie,
         options::OPT_fno_sanitize_address_poison_custom_array_cookie,
         AsanPoisonCustomArrayCookie);

clang/lib/Driver/ToolChains/MSVC.cpp

if (!Args.hasArg(options::OPT_shared))
           Args.MakeArgString(std::string("-wholearchive:") +
                              TC.getCompilerRTArgString(Args, "fuzzer")));
     CmdArgs.push_back(Args.MakeArgString("-debug"));
     // Prevent the linker from padding sections we use for instrumentation
     // arrays.
     CmdArgs.push_back(Args.MakeArgString("-incremental:no"));
   }
 
+  bool DebugRuntime = Args.hasArg(options::OPT__SLASH_MTd) ||
+                      Args.hasArg(options::OPT__SLASH_MDd) ||
+                      Args.hasArg(options::OPT__SLASH_LDd);
+
+  std::string AsanPrefix = DebugRuntime ? "asan_dbg" : "asan";
+  std::string AsanCXX = DebugRuntime ? "asan_cxx_dbg" : "asan_cxx";
+
   if (TC.getSanitizerArgs(Args).needsAsanRt()) {
     CmdArgs.push_back(Args.MakeArgString("-debug"));
     CmdArgs.push_back(Args.MakeArgString("-incremental:no"));
     if (TC.getSanitizerArgs(Args).needsSharedRt() ||
         Args.hasArg(options::OPT__SLASH_MD, options::OPT__SLASH_MDd)) {
-      for (const auto &Lib : {"asan_dynamic", "asan_dynamic_runtime_thunk"})
-        CmdArgs.push_back(TC.getCompilerRTArgString(Args, Lib));
+      for (const auto &Lib : {"_dynamic", "_dynamic_runtime_thunk"})
+        CmdArgs.push_back(TC.getCompilerRTArgString(Args, AsanPrefix + Lib));
       // Make sure the dynamic runtime thunk is not optimized out at link time
       // to ensure proper SEH handling.
       CmdArgs.push_back(Args.MakeArgString(
           TC.getArch() == llvm::Triple::x86
               ? "-include:___asan_seh_interceptor"
               : "-include:__asan_seh_interceptor"));
       // Make sure the linker consider all object files from the dynamic runtime
       // thunk.
-      CmdArgs.push_back(Args.MakeArgString(std::string("-wholearchive:") +
-          TC.getCompilerRT(Args, "asan_dynamic_runtime_thunk")));
+      CmdArgs.push_back(Args.MakeArgString(
+          std::string("-wholearchive:") +
+          TC.getCompilerRT(Args, AsanPrefix + "_dynamic_runtime_thunk")));
     } else if (DLL) {
-      CmdArgs.push_back(TC.getCompilerRTArgString(Args, "asan_dll_thunk"));
+      CmdArgs.push_back(
+          TC.getCompilerRTArgString(Args, AsanPrefix + "_dll_thunk"));
     } else {
-      for (const auto &Lib : {"asan", "asan_cxx"}) {
+      for (const auto &Lib : {AsanPrefix, AsanCXX}) {
         CmdArgs.push_back(TC.getCompilerRTArgString(Args, Lib));
         // Make sure the linker consider all object files from the static lib.
         // This is necessary because instrumented dlls need access to all the
         // interface exported by the static lib in the main executable.
         CmdArgs.push_back(Args.MakeArgString(std::string("-wholearchive:") +
             TC.getCompilerRT(Args, Lib)));
       }
     }