[clang][MinGW] Add asan DLL lib before other libs and objects

[alvinhochun]

Summary

As stated in https://github.com/llvm/llvm-project/issues/61685, by
passing LLD the import lib of the asan DLL first, the asan DLL will be
listed as the first entry in the Import Directory Table, making it be
loaded first before other user DLLs. This allows asan to be initialized
as early as possible to increase its instrumentation coverage to include
other DLLs not built with asan.

This also avoids some false asan reports on realloc for memory
allocated during initialization of user DLLs being loaded earlier than
asan, because after this change they will be loaded later than asan.

[alvinhochun]

Trying to fix the test...

[mstorsjo]

I tested this, and this does fix the repro from the linked issue.

I wonder if we do want to move all of these before the app itself, or if it's enough to just move the reference to asan_dynamic-<arch>.dll.a earlier, and keep the rest as it was? Especially the whole-archive bit ends up linking a bunch of things statically, ordered way before the app itself. On one hand, I wonder if it would have subtle effects that we don't notice until way later, and we should only change as little as we need (moving the import library reference earlier), or if there are other potential benefits to moving the wholearchive-linked bits earlier too? (I'm wondering about things like constructor execution orders and such.)

clang/test/Driver/mingw-sanitizers.c

1–15

Hm, what does this do - some sort of more portable version of just touching a file to create it?

[alvinhochun]

In D146908#4226926, @mstorsjo wrote:

I tested this, and this does fix the repro from the linked issue.

Thanks for testing!

I wonder if we do want to move all of these before the app itself, or if it's enough to just move the reference to asan_dynamic-<arch>.dll.a earlier, and keep the rest as it was? Especially the whole-archive bit ends up linking a bunch of things statically, ordered way before the app itself. On one hand, I wonder if it would have subtle effects that we don't notice until way later, and we should only change as little as we need (moving the import library reference earlier), or if there are other potential benefits to moving the wholearchive-linked bits earlier too? (I'm wondering about things like constructor execution orders and such.)

That is a valid concern, especially if we want to backport this change to 16.x. I didn't think about this clearly before. With this in mind, I would change the patch to only add a duplicate of asan_dynamic-<arch>.dll.a early and keep the existing flags at their old position.

I can do some experiments with the new asan test setup I have and see if there are other benefits of moving the other flags. That will be for separate patches.

clang/test/Driver/mingw-sanitizers.c

1–15

Yeah that's just creating the file (though it would also overwrite the existing content). I believe lit has echo built-in but touch may not be available on Windows so this seems like a good idea to me.

[mstorsjo]

clang/test/Driver/mingw-sanitizers.c

1–15

Ah, I see. FWIW, the lit tests in general do assume a handful of unix utilities - we seem to have unconditional uses of touch in various tests. The lit tests check if these tools are available in path, and if they aren't, it tries to locate an installation of Git for Windows (which contains what's needed) and add that to the path internally: https://github.com/llvm/llvm-project/blob/llvmorg-17-init/llvm/utils/lit/lit/llvm/config.py#L29-L35

[alvinhochun]

clang/test/Driver/mingw-sanitizers.c

1–15

I suppose that works. Would you prefer to use touch here too?

[mstorsjo]

clang/test/Driver/mingw-sanitizers.c

1–15

I don't have a strong opinion, but with touch I know what is intended, while this stopped my reading, wondering what the flag to echo does and if this is meant to have any specific function other than creating the file.

[alvinhochun]

Updated per comments.

[mstorsjo]

LGTM overall.

I wondered if the modified test runs correctly on Windows (any test that touches paths is prone to break) but I see that it seems to have run successfully on both Windows and Linux in the premerge CI, so that's at least good. One minor question for the test.

clang/test/Driver/mingw-sanitizers.c

2

You're using %/t.a here while the file you touch is %t.a - I don't think I have seen %/t anywhere here before. I do see that lit seems to replace it though... Is this an intentional thing (what's the difference to %t though?) or is it a consistently copied typo?

[alvinhochun]

clang/test/Driver/mingw-sanitizers.c

2

Ah yes, that is intentional. It's documented on https://llvm.org/docs/CommandGuide/lit.html#substitutions to be "%t but \ is replaced by /". The reason for this is that, the previous Windows pre-merge check failed because when clang -### prints the command line it quotes and backslash-escape the arguments., FileCheck cannot match the INPUT path with double backslashes. Using forward slashes avoids this issue.

[mstorsjo]

clang/test/Driver/mingw-sanitizers.c

2

Oh, I see, thanks - TIL! That makes sense. I wonder why a grep for that didn't get almost any hits in the clang/test/Driver directory though - but maybe it's used more elsewhere?

clang/lib/Driver/ToolChains/MinGW.cpp

void tools::MinGW::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   Args.AddAllArgs(CmdArgs, options::OPT_e);
   // FIXME: add -N, -n flags
   Args.AddLastArg(CmdArgs, options::OPT_r);
   Args.AddLastArg(CmdArgs, options::OPT_s);
   Args.AddLastArg(CmdArgs, options::OPT_t);
   Args.AddAllArgs(CmdArgs, options::OPT_u_Group);
   Args.AddLastArg(CmdArgs, options::OPT_Z_Flag);
 
+  // Add asan_dynamic as the first import lib before other libs. This allows
+  // asan to be initialized as early as possible to increase its instrumentation
+  // coverage to include other user DLLs which has not been built with asan.
+  if (Sanitize.needsAsanRt() && !Args.hasArg(options::OPT_nostdlib) &&
+      !Args.hasArg(options::OPT_nodefaultlibs)) {
+    // MinGW always links against a shared MSVCRT.
+    CmdArgs.push_back(
+        TC.getCompilerRTArgString(Args, "asan_dynamic", ToolChain::FT_Shared));
+  }
+
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles)) {
     if (Args.hasArg(options::OPT_shared) || Args.hasArg(options::OPT_mdll)) {
       CmdArgs.push_back(Args.MakeArgString(TC.GetFilePath("dllcrt2.o")));
     } else {
       if (Args.hasArg(options::OPT_municode))
         CmdArgs.push_back(Args.MakeArgString(TC.GetFilePath("crt2u.o")));
       else
         CmdArgs.push_back(Args.MakeArgString(TC.GetFilePath("crt2.o")));

clang/test/Driver/mingw-sanitizers.c

-// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address 2>&1 | FileCheck --check-prefix=ASAN-I686 %s
-// ASAN-I686: "{{.*}}libclang_rt.asan_dynamic-i386.dll.a"
-// ASAN-I686: "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a"
-// ASAN-I686: "--require-defined" "___asan_seh_interceptor"
-// ASAN-I686: "--whole-archive" "{{.*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a" "--no-whole-archive"
-
-// RUN: %clang -target x86_64-windows-gnu %s -### -fsanitize=address 2>&1 | FileCheck --check-prefix=ASAN-X86_64 %s
-// ASAN-X86_64: "{{.*}}libclang_rt.asan_dynamic-x86_64.dll.a"
+// RUN: touch %t.a
+// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address -lcomponent %/t.a 2>&1 | FileCheck --check-prefixes=ASAN-ALL,ASAN-I686 -DINPUT=%/t.a %s
+// RUN: %clang -target x86_64-windows-gnu %s -### -fsanitize=address -lcomponent %/t.a 2>&1 | FileCheck --check-prefixes=ASAN-ALL,ASAN-X86_64 -DINPUT=%/t.a %s
+//
+// ASAN-ALL-NOT:"-l{{[^"]+"]}}"
+// ASAN-ALL-NOT:"[[INPUT]]"
+// ASAN-I686:   "{{[^"]*}}libclang_rt.asan_dynamic-i386.dll.a"
+// ASAN-X86_64: "{{[^"]*}}libclang_rt.asan_dynamic-x86_64.dll.a"
+// ASAN-ALL:    "-lcomponent"
+// ASAN-ALL:    "[[INPUT]]"
+// ASAN-I686:   "{{[^"]*}}libclang_rt.asan_dynamic-i386.dll.a"
+// ASAN-I686:   "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a"
+// ASAN-I686:   "--require-defined" "___asan_seh_interceptor"
+// ASAN-I686:   "--whole-archive" "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a" "--no-whole-archive"
+// ASAN-X86_64: "{{[^"]*}}libclang_rt.asan_dynamic-x86_64.dll.a"
 // ASAN-X86_64: "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-x86_64.a"
 // ASAN-X86_64: "--require-defined" "__asan_seh_interceptor"
-// ASAN-X86_64: "--whole-archive" "{{.*}}libclang_rt.asan_dynamic_runtime_thunk-x86_64.a" "--no-whole-archive"
+// ASAN-X86_64: "--whole-archive" "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-x86_64.a" "--no-whole-archive"
 
 // RUN: %clang -target x86_64-windows-gnu %s -### -fsanitize=vptr