Looks sensible to me, modulo the option description.

This commit also broke compilation of some files for me, for ARM, where compilation now doesn't finish (files that normally compile within a second now doesn't finish after half an hour. See https://bugs.llvm.org/show_bug.cgi?id=41160 for details and a reproduction sample.

In D59530#1434691, @aganea wrote:

In D59530#1434688, @mstorsjo wrote:

Aren't all of /implib, /pdband /out output files? In that sense it doesn't matter where they are written in the repro case (and writing them locally instead of somewhere else with an absolute path probably is more convenient?).

Yes, however our build system generates cmd-lines with full or relative paths for these options, which makes the response unusable, unless you edit it (because the paths aren't there on the reproducer's machine)

Are you suggesting that we strip those options from their path, and only kept the filename? That would be a good alternative.

Aren't all of /implib, /pdband /out output files? In that sense it doesn't matter where they are written in the repro case (and writing them locally instead of somewhere else with an absolute path probably is more convenient?).

Sounds ok to me

This broke compilation of libcxx for armv7 mingw for me, see https://bugs.llvm.org/show_bug.cgi?id=41081 for details.

No objection from the mingw perspective. Sanitizers on mingw are only supported by clang+lld, not by gcc/ld.bfd, and then it's built by clang in mingw mode with -fms-extensions, so most msvc specific extensions are available. Additionally, I haven't even yet a full setup for running compiler-rt/libcxx runtime tests, so I'm not sure if there are other things that need to be fixed wrt mingw in the tests.

In D59025#1420036, @santagada wrote:

Well, link.exe seems to get "confused" it doesn't print any error messages and produce a very big file that is not really an executable. Without any messages or symbols for it its a bit difficult to see what is happening. As I mostly did the same as the gnu-link option does, I guess those obj files should have the same problem.

Changed to do the same thing with a regex.

Factorized the condition to a macro. As sanitizer_common.h is inclued only later, I had to move the include of traceloggingprovider.h out from the standard include block into a later conditional.

In D58765#1414329, @mcgov wrote:

@rnk that suggestion works. I'll start testing mingw along with msvc before I commit in the future.

In D58765#1414310, @rnk wrote:

Should this be factored into some general define in some header (which one?) like SANITIZER_WIN_TRACE or so, to avoid duplicating the condition all over the place?

I just noticed that this breaks clang-cl builds of asan because clang-cl doesn't support some weird pre-processor patterns used by TraceLoggingProvider.h:
https://bugs.llvm.org/show_bug.cgi?id=32021

In D58767#1413871, @rnk wrote:

I'd prefer to keep the same /Zi vs. /Z7 setting in the standalone build. Does the standalone build not have access to the installed LLVM cmake modules?

It turns out that the fix for Windows in rL355052 didn't turn out to help either: http://lab.llvm.org:8011/builders/sanitizer-windows/builds/42624/steps/stage%201%20build/logs/stdio

LGTM when rebased on top of D58380.

Changed the condition as requested.

This broke compilation with GCC 5.4 on Ubuntu 16.04:

I tested and it seems good, I can build the whole mingw-w64 runtime again with this applied.

This change broke compiling the mingw-w64 runtime, which uses GCC_ASM_FLAG_OUTPUTS to enable newer forms of inline assembly. See PR40737 for details.

Please add a check for this new option in COFF/COFFObjCopy.cpp for the "Option not supported by llvm-objcopy for COFF" error, to avoid giving the illusion that the option is handled with that backend.

In D58107#1396063, @davezarzycki wrote:

This change breaks building/testing the compiler with CLANG_DEFAULT_LINKER set to lld. Was this intentional? What should people do if they want to use CLANG_DEFAULT_LINKER and run the test suite?

Thanks, looks good to me now. I'll leave someone else to do the overall approval though.

LGTM

In D57625#1386061, @rnk wrote:

Do we know when most of these were added? Did the all arrive in more or less the same version? It would be nice to make this conditional on a new enough environment version. That gives users a way to indicate that they are targetting an old CRT if they need to.

Thanks, this looks much more readable!

LGTM

In D57625#1384112, @evandro wrote:

In D57625#1382005, @mstorsjo wrote:

(And it also works with mingw which normally uses an ancient msvcrt.)

Indeed, this change only affects Windows proper, as both Cygwin and MinGW were already excluded from this selection of supported math functions.