[PATCH] D118162: [tblgen] Disable lsan weak hook when building with msvc

Wed Jan 26 06:05:00 PST 2022

aaron.ballman added a comment.

In D118162#3272302 <https://reviews.llvm.org/D118162#3272302>, @pgousseau wrote:

> In D118162#3272215 <https://reviews.llvm.org/D118162#3272215>, @aaron.ballman wrote:
>
>> I'm a bit confused as to why this change is needed. My understanding is that `__SANITIZE_ADDRESS__` is only defined when the user passes `-fsanitize=address` (so they explicitly opted in). So isn't the issue here that the user asked for something unsupported and the solution is for them to not pass that compiler argument on Windows? Alternatively, should the fix be to require `__has_feature(leak_sanitizer)` to return true instead of using `||`?
>>
>> (I'm a bit worried that we're carving out an exception for something that's feature testable and when the feature eventually gets supported on Windows, nobody will remember to go remove the special exceptions.)
>
> Thank you for reviewing!

My pleasure, thanks for working on this!

> cl.exe does not support __has_feature unfortunately.

True, but we have a fallback on line 288 that defaults to the feature not being supported. So the only way to hit this problem is when `__SANITIZE_ADDRESS__` is defined.

> I have not tried but maybe an alternative would be to check the output of an instrumented exe with ASAN_OPTIONS=detect_leaks=1 during cmake? WDYT?
> eg:
>
>   cl.exe -fsanitize=address t.c /w /nologo /link /DEBUG && env ASAN_OPTIONS=detect_leaks=1 t.exe
>   t.c
>      Creating library t.lib and object t.exp
>   
>   ==28436==AddressSanitizer: detect_leaks is not supported on this platform.

Oh, I was forgetting that cl would set `__SANITIZE_ADDRESS__`, I am following along better now. I don't think we need to do anything as fancy as detecting during CMake, but that sounds like it would work (assuming there's no oddities like localized diagnostics that depend on the system language settings or something like that).

Personally, I think I'm fine either way. Should the test be for `_WIN32` or for `_MSC_VER` though? (I don't know how mingw or cygwin change things.)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118162/new/

https://reviews.llvm.org/D118162