This is an archive of the discontinued LLVM Phabricator instance.

[sanitizers] Don't use Windows Trace Logging on MinGW
ClosedPublic

Authored by mstorsjo on Feb 28 2019, 2:41 AM.

Details

Summary

mingw-w64 currently is lacking the headers for this feature.

Make the include lowercase at the same time. We consistently use lowercase for windows header includes, as windows itself is case insensitive, the SDK headers (in general, not necessarily considering this particular header) aren't consistent among themselves about what the proper canonical capitalization for headers are, and MinGW uses all lowercase names for the headers (as it is often used on case sensitive filesystems).

In case mingw-w64 later gets this header, we can revert this (but keep the include lowercased).

Diff Detail

Event Timeline

mstorsjo created this revision.Feb 28 2019, 2:41 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 28 2019, 2:41 AM
Herald added subscribers: Restricted Project, kubamracek. · View Herald Transcript
rnk added a comment.Feb 28 2019, 3:20 PM

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

So, I think we should go ahead and make SANITIZER_WIN_TRACE and add defined(__clang__) to the list of conditions. I think the simplest check is probably #if defined(_MSC_VER) && !defined(__clang__) because that rules out mingw.

mcgov added a comment.EditedFeb 28 2019, 3:54 PM

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

Are you alright with the definition of that macro

#define SANITIZER_WIN_TRACE defined(_MSC_VER) && ! defined(__clang__)

in sanitizer_common.h?

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

Is this really still the case? For me at least, it builds fine with clang-cl, with TraceLoggingProvider.h from WinSDK 10.0.17763.0.

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

Are you alright with the definition of that macro

#define SANITIZER_WIN_TRACE defined(_MSC_VER) && ! defined(__clang__)

I'd suggest the form

#if defined(_MSC_VER) && !defined(__clang__)
#define SANITIZER_WIN_TRACE 1
#else
#define SANITIZER_WIN_TRACE 0
#endif

because expanding defined() from a macro is undefined behaviour (clang has got a warning for it, -Wexpansion-to-defined, which is enabled as part of -Wall).

But as current clang with current WinSDK headers work (I'm not sure which one of them have changed wrt to this issue), perhaps we could just keep it with checking _MSC_VER (or SANITIZER_WINDOWS && !defined(__MINGW32__), but only checking _MSC_VER is more straightforward).

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

Also do note that building sanitizers for mingw isn't entirely trivial; it requires both Clang and LLD, so normal mingw setups won't work. Prebuilt toolchains (with all defaults set up in the right way) are available at https://github.com/mstorsjo/llvm-mingw/releases in case that helps you get set up.

As this is a rather niche setup, I wouldn't expect you to test it all the time though - if anything breaks, I notice soon and hopefully can resolve it after the fact.

mstorsjo updated this revision to Diff 188855.Mar 1 2019, 12:37 AM
mstorsjo edited the summary of this revision. (Show Details)

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.

mcgov accepted this revision.Mar 1 2019, 9:43 AM

Looks good! Thank you for the pointer to the mingw kit, also.

This revision is now accepted and ready to land.Mar 1 2019, 9:43 AM
This revision was automatically updated to reflect the committed changes.