fix warning about noreturn in sanitizer_win's internal__exit()

[inglorion]

Summary

A previous fix used __assume(0), but not all compilers know that control will
not pass that. This patch uses a macro which works in more compilers.

[rnk]

lib/sanitizer_common/sanitizer_win.cc

41

We don't want to do that, because it can re-enter into internal__exit() and then it would stack overflow. Defining it to nothing is OK. It just means that compilers that are not gcc, clang, or msvc will have false positive warnings. TerminateProcess will never return.

Similarly, this is why we can't call sanitizer_common's normal UNREACHABLE macro. It would re-enter. I guess we could call ExitProcess or something that is marked noreturn, but it would run finalizers and that's bad.

[inglorion]

lib/sanitizer_common/sanitizer_win.cc

41

Right, I knew about UNREACHABLE re-entering, which is why I didn't just use that. I'll change the fallback implementation to be empty and add a comment with what you wrote so the next reader doesn't make the same mistake I did.

[inglorion]

Removed code that terminates the process and added a comment explaining why such code is incorrect.

[inglorion]

With this change, compiler-rt builds without warnings when lld is used as the linker (MSVC's link.exe still emits one warning because it doesn't support weak symbols).

[rnk]

lgtm, thanks! Eventually we should teach clang to pattern match assume(0) -> builtin_unreachable.

[alekseyshl]

compiler-rt/trunk/lib/sanitizer_common/sanitizer_win.cc

40 ↗

(On Diff #83166)

At least sanitizer-x86_64-linux-autoconf bot failed lib/CMakeFiles/SanitizerLintCheck with "Lines should be <= 80 characters long" error.

lib/sanitizer_common/sanitizer_win.cc

 #include "sanitizer_dbghelp.h"
 #include "sanitizer_libc.h"
 #include "sanitizer_mutex.h"
 #include "sanitizer_placement_new.h"
 #include "sanitizer_procmaps.h"
 #include "sanitizer_stacktrace.h"
 #include "sanitizer_symbolizer.h"
 
+// A macro to tell the compiler that this part of the code cannot be reached,
+// if the compiler supports this feature. Since we're using this in
+// code that is called when terminating the process, the expansion of the
+// macro should not terminate the process to avoid infinite recursion.
+#if defined(__clang__)
+# define BUILTIN_UNREACHABLE() __builtin_unreachable()
+#elif defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5))
+# define BUILTIN_UNREACHABLE() __builtin_unreachable()
+#elif defined(_MSC_VER)
+# define BUILTIN_UNREACHABLE() __assume(0)
+#else
+# define BUILTIN_UNREACHABLE()
+#endif
+
 namespace __sanitizer {
 
 #include "sanitizer_syscall_generic.inc"
 
 // --------------------- sanitizer_common.h
 uptr GetPageSize() {
   SYSTEM_INFO si;
   GetSystemInfo(&si);

 
 void internal__exit(int exitcode) {
   // ExitProcess runs some finalizers, so use TerminateProcess to avoid that.
   // The debugger doesn't stop on TerminateProcess like it does on ExitProcess,
   // so add our own breakpoint here.
   if (::IsDebuggerPresent())
     __debugbreak();
   TerminateProcess(GetCurrentProcess(), exitcode);
-  __assume(0);
+  BUILTIN_UNREACHABLE();
 }
 
 uptr internal_ftruncate(fd_t fd, uptr size) {
   UNIMPLEMENTED();
 }
 
 uptr GetRSS() {
   return 0;