This is an archive of the discontinued LLVM Phabricator instance.

[CMake] [MinGW] Enable use of LLVM_USE_SANITIZER in a MinGW environment
ClosedPublic

Authored by zero9178 on Jan 30 2021, 3:44 PM.

Details

Summary

Currently using LLVM_USE_SANITIZER with a MinGW target leads to a fatal configuration error due to an unsupported platform. MinGW targets on clang however implement a few sanitizers, currently ASAN and UBSAN.

This patch enables LLVM_USE_SANITIZER in a MinGW environment as well.

If accepted I'd need this patch to be committed for me with the email: markus.boeck02@gmail.com

Diff Detail

Event Timeline

zero9178 created this revision.Jan 30 2021, 3:44 PM
zero9178 requested review of this revision.Jan 30 2021, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2021, 3:44 PM
mstorsjo added a subscriber: rnk.

I guess this looks sensible to me, although I'm a bit unsure why we need to hardcode exactly which sets of sanitizers each target environment supports at the moment. I would kinda understand if we want to keep MSVC separate if those cases used different command line options, but it looks like they don't, so technically one could kinda merge all of the existing unix/msvc cases too. If one would try to use a sanitizer that isn't supported, the cmake configuration wouldn't error out immediately, but it'd be noticed directly when the compiler errors out on the unsupported options anyway.

What does @rnk think about it?

I can see it both ways - I like when CMake tells me if a configuration is not supported - but on the other hand this is probably a pretty easy to spot in the compiler error what's wrong. I guess the section could be replaced with actually trying to compile a snippet with TryCompile with the sanitizer flag instead and replace the specific flag check.

@rnk - what's your opinion on this?

rnk accepted this revision.Feb 8 2021, 12:49 PM

lgtm

llvm/cmake/modules/HandleLLVMOptions.cmake
803–804

Well, I guess there is prior art for this tortured conditional.

811

MSVC should probably get the same treatment, but that doesn't need to be part of this change.

This revision is now accepted and ready to land.Feb 8 2021, 12:49 PM
This revision was landed with ongoing or failed builds.Feb 8 2021, 1:05 PM
This revision was automatically updated to reflect the committed changes.