[tblgen] Disable lsan weak hook when building with msvc

[pgousseau]

Summary

This change is to fix a link time error when building llvm with msvc.

MSVC's implementation does not support weak hook or lsan so this change disables lsan's weak hook definition.
Only GCC supports LSan.

Tested with visual studio 2019 v16.9.6

[aaron.ballman]

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.)

[pgousseau]

In 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!

cl.exe does not support __has_feature unfortunately.

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.

[aaron.ballman]

In D118162#3272302, @pgousseau wrote:

In 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.)

[rnk]

llvm/utils/TableGen/TableGen.cpp

293

It seems to me that neither GCC nor MSVC support __has_feature, but they both set __SANITIZE_ADDRESS__. Only GCC supports LSan. I would prefer to phrase this positively: if we are using GCC ASan, set up this LSan hook. So, I prefer the condition (defined(__SANITIZE_ADDRESS__) && defined(__GNUC__)). I don't know if GCC supports ASan/LSan on Windows as part of mingw, but I don't see any reason to rule it out. If we just want to opt MSVC out and use LSan with any other hypothetical non-gnu, non-MSVC compiler, we could spell this !defined(_MSC_VER).

[pgousseau]

Updating patch following comments, using __GNUC__ instead of !_WIN32 which was wrong.

[pgousseau]

llvm/utils/TableGen/TableGen.cpp

293

Thanks yes !_WIN32 was wrong, I have updated to use __GNUC__

[aaron.ballman]

LGTM!

llvm/utils/TableGen/TableGen.cpp

int main(int argc, char **argv) {
 
   return TableGenMain(argv[0], &LLVMTableGenMain);
 }
 
 #ifndef __has_feature
 #define __has_feature(x) 0
 #endif
 
-#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__) ||       \
+#if __has_feature(address_sanitizer) ||                                        \
+    (defined(__SANITIZE_ADDRESS__) && defined(__GNUC__)) ||                    \
     __has_feature(leak_sanitizer)
 
 #include <sanitizer/lsan_interface.h>
 // Disable LeakSanitizer for this binary as it has too many leaks that are not
 // very interesting to fix. See compiler-rt/include/sanitizer/lsan_interface.h .
 LLVM_ATTRIBUTE_USED int __lsan_is_turned_off() { return 1; }
 
 #endif