This is an archive of the discontinued LLVM Phabricator instance.

[CMake] [MinGW] Build address sanitizer for MinGW
ClosedPublic

Authored by mstorsjo on Sep 10 2018, 12:35 PM.

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 10 2018, 12:35 PM
Herald added subscribers: Restricted Project, mgorny, kubamracek. · View Herald TranscriptSep 10 2018, 12:35 PM
rnk added a comment.Sep 10 2018, 1:27 PM

Does it build with GCC, clang, or both?

In D51885#1229529, @rnk wrote:

Does it build with GCC, clang, or both?

I've only tested with clang. In general I would think that GCC mostly supports most of the same things with -fms-extensions, but I haven't tried (I don't know what the state is of actually instrumenting for asan for windows targets in gcc either). But the thing that most certainly won't work, are the /include: and /alternatename: options in embedded linker directives, if attempting to link with ld.bfd.

I should add more reviewers, since adding a mingw port is not a small matter. I *think* that some of the sanitizer_common code already builds with mingw gcc for tsan-go, right? So, adding support for building with gcc on Windows should not be too onerous for the project as a whole. At least, that's what I think. Other folks can chime in.

In D51885#1229529, @rnk wrote:

Does it build with GCC, clang, or both?

I've only tested with clang. In general I would think that GCC mostly supports most of the same things with -fms-extensions, but I haven't tried (I don't know what the state is of actually instrumenting for asan for windows targets in gcc either).

It turns out that GCC actually doesn't support this syntax at all (at least not in a 5.x version). So without that, building with GCC would require much more invasive changes.

Additionally, Clang provides __debugbreak() as a builtin when building with -fms-extensions, but GCC doesn't (mingw provides it as an inline function in a header unless the compiler has got it as a builtin).

mstorsjo added inline comments.Sep 11 2018, 1:51 PM
cmake/config-ix.cmake
517

I guess this should be extended into something that allows building it with mingw+clang, but not with mingw+gcc, in case there are existing cases where this is project is built in that configuration, since this would otherwise break it by trying to build something that isn't supported?

mstorsjo updated this revision to Diff 165139.Sep 12 2018, 1:19 PM

Updated the check, to only allow building for mingw if the compiler is clang. This should make sure not to break existing mingw builds with gcc/binutils, if that's something that people do. It won't catch the case of building with clang but linking with binutils though...

Ping @kcc and others

rnk accepted this revision.Sep 25 2018, 8:41 AM

Ping @kcc and others

@kcc is here at CppCon. I think we should go forward with this. As you've explained, we're mostly adding support for the mingw headers and libs, not a new compiler, and that lowers the cost.

This revision is now accepted and ready to land.Sep 25 2018, 8:41 AM
In D51885#1245163, @rnk wrote:

Ping @kcc and others

@kcc is here at CppCon. I think we should go forward with this. As you've explained, we're mostly adding support for the mingw headers and libs, not a new compiler, and that lowers the cost.

Great, thanks! Yes, it's just a minor tweak of the existing windows support, so it shouldn't require any larger commitment. There were two other patches still unapproved in the same series; I bumped one at the same time as this, I'll bump the other one as well.

This revision was automatically updated to reflect the committed changes.