[compiler-rt] Use macros to simplify weak alias for Windows and add some documentation.

[mpividori]

Summary

I create some useful macros for dealing with pragma directives in Windows.
Also, after some experience with "alternatename" pragma, I decided to add some information for future users.
Thanks.

[kubamracek]

lib/ubsan/ubsan_flags.cc

20 ↗

(On Diff #83841)

I think this needs to be included only on Windows, because sanitizer_win_defs.h errors out if _MCS_VER is not defined.

[rnk]

Thanks for the refactoring! I'd been thinking of doing it for a while.

lib/asan/asan_globals_win.h

1 ↗

(On Diff #83841)

I think with your change, this header is no longer needed. We can replace all instances of ASAN_LINK_GLOBALS_WIN() with WIN_FORCE_LINK(__asan_dso_reg_hook).

lib/sanitizer_common/sanitizer_win_defs.h

17 ↗

(On Diff #83841)

The existing pattern in sanitizer_linux.h and sanitizer_mac.h is to allow the header to be included everywhere but ifdef out the contents. I think this reduced ifdefs and is in general consistent with how Kostya likes to do things in the sanitizer libraries. Let's do that. So, please change this to #if SANITIZER_WINDOWS.

lib/ubsan/ubsan_flags.cc

20 ↗

(On Diff #83841)

Let's fix the header to make it easy to include instead.

[mpividori]

@kubabrecka , you are right. Sorry, I was going to test on linux before submitting the commit to svn.

[mpividori]

@rnk Ok. Yes, I agree.

[mpividori]

Done.

[rnk]

lgtm

[mpividori]

Hi, also I think we could create a general macro, like:

#if SANITIZER_WINDOWS
#define WEAK_ALIAS(Name, Default)  \
  __pragma(comment(linker, "/alternatename:" #Name "=" #Default))
#else
#define STRINGIFY(A) #A
#define WEAK_ALIAS(Name, Default) \
  _Pragma( STRINGIFY(weak Name =  Default) )
#endif

And always use that macro when need to support Windows. For example:

int some_fun_def() {
   .. // Default impl.
}
WEAK_ALIAS(some_fun, some_fun_def)

This definitely simplifies the code, but has the disadvantage that we always need to define a default function, which is not strictly necessary for weak functions in non-windows systems, and as they have the same visibility, we would also be exporting the default functions (unless we find some way to avoid this).

Another option is to define another macro:

#if SANITIZER_WINDOWS
#define WEAK(ReturnType, Function) \ 
   WEAK_ALIAS(Function, Function##_def)
   ReturnType Function##_def
#else
#define WEAK(function) \ 
   ReturnType __attribute__((weak)) Function
#endif

Then we can use:

WEAK(int, some_fun) {
   .. // Default impl.
}

What do you think?
Thanks.

[rnk]

Defining a cross-platform weak mechanism seems like a great idea, even if we can only use it when we have default implementations. It greatly simplify our Windows porting efforts. Adding new weak functions is one of the more common ways to break the WinASan build.

[rnk]

... but this is a great incremental improvement, so let's commit this first.

[mpividori]

Hi, I updated the diff to use the macro STRINGIFY() instead of directly # , so we ensure we expand the parameters before stringifying.

[mpividori]

Ready to land?

[rnk]

Yep, looks good.

compiler-rt/trunk/lib/asan/asan_globals_win.h

-//===-- asan_globals_win.h --------------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//
-// Interface to the Windows-specific global management code. Separated into a
-// standalone header to allow inclusion from asan_win_dynamic_runtime_thunk,
-// which defines symbols that clash with other sanitizer headers.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef ASAN_GLOBALS_WIN_H
-#define ASAN_GLOBALS_WIN_H
-
-#if !defined(_MSC_VER)
-#error "this file is Windows-only, and uses MSVC pragmas"
-#endif
-
-#if defined(_WIN64)
-#define SANITIZER_SYM_PREFIX
-#else
-#define SANITIZER_SYM_PREFIX "_"
-#endif
-
-// Use this macro to force linking asan_globals_win.cc into the DSO.
-#define ASAN_LINK_GLOBALS_WIN() \
-  __pragma(                     \
-      comment(linker, "/include:" SANITIZER_SYM_PREFIX "__asan_dso_reg_hook"))
-
-#endif // ASAN_GLOBALS_WIN_H

compiler-rt/trunk/lib/asan/asan_win.cc

 
 #include "sanitizer_common/sanitizer_platform.h"
 #if SANITIZER_WINDOWS
 #define WIN32_LEAN_AND_MEAN
 #include <windows.h>
 
 #include <stdlib.h>
 
-#include "asan_globals_win.h"
 #include "asan_interceptors.h"
 #include "asan_internal.h"
 #include "asan_report.h"
 #include "asan_stack.h"
 #include "asan_thread.h"
 #include "asan_mapping.h"
 #include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_mutex.h"
+#include "sanitizer_common/sanitizer_win_defs.h"
 
 using namespace __asan;  // NOLINT
 
 extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE
 int __asan_should_detect_stack_use_after_return() {
   __asan_init();
   return __asan_option_detect_stack_use_after_return;
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 uptr __asan_get_shadow_memory_dynamic_address() {
   __asan_init();
   return __asan_shadow_memory_dynamic_address;
 }
 
-// -------------------- A workaround for the absence of weak symbols ----- {{{
-// We don't have a direct equivalent of weak symbols when using MSVC, but we can
-// use the /alternatename directive to tell the linker to default a specific
-// symbol to a specific value, which works nicely for allocator hooks and
-// __asan_default_options().
 void __sanitizer_default_malloc_hook(void *ptr, uptr size) { }
 void __sanitizer_default_free_hook(void *ptr) { }
 const char* __asan_default_default_options() { return ""; }
 const char* __asan_default_default_suppressions() { return ""; }
 void __asan_default_on_error() {}
-// 64-bit msvc will not prepend an underscore for symbols.
-#ifdef _WIN64
-#pragma comment(linker, "/alternatename:__sanitizer_malloc_hook=__sanitizer_default_malloc_hook")  // NOLINT
-#pragma comment(linker, "/alternatename:__sanitizer_free_hook=__sanitizer_default_free_hook")      // NOLINT
-#pragma comment(linker, "/alternatename:__asan_default_options=__asan_default_default_options")    // NOLINT
-#pragma comment(linker, "/alternatename:__asan_default_suppressions=__asan_default_default_suppressions")    // NOLINT
-#pragma comment(linker, "/alternatename:__asan_on_error=__asan_default_on_error")                  // NOLINT
-#else
-#pragma comment(linker, "/alternatename:___sanitizer_malloc_hook=___sanitizer_default_malloc_hook")  // NOLINT
-#pragma comment(linker, "/alternatename:___sanitizer_free_hook=___sanitizer_default_free_hook")      // NOLINT
-#pragma comment(linker, "/alternatename:___asan_default_options=___asan_default_default_options")    // NOLINT
-#pragma comment(linker, "/alternatename:___asan_default_suppressions=___asan_default_default_suppressions")    // NOLINT
-#pragma comment(linker, "/alternatename:___asan_on_error=___asan_default_on_error")                  // NOLINT
-#endif
-// }}}
+
+WIN_WEAK_ALIAS(__sanitizer_malloc_hook, __sanitizer_default_malloc_hook)
+WIN_WEAK_ALIAS(__sanitizer_free_hook, __sanitizer_default_free_hook)
+WIN_WEAK_ALIAS(__asan_default_options, __asan_default_default_options)
+WIN_WEAK_ALIAS(__asan_default_suppressions, __asan_default_default_suppressions)
+WIN_WEAK_ALIAS(__asan_on_error, __asan_default_on_error)
 }  // extern "C"
 
 // ---------------------- Windows-specific interceptors ---------------- {{{
 INTERCEPTOR_WINAPI(void, RtlRaiseException, EXCEPTION_RECORD *ExceptionRecord) {
   CHECK(REAL(RtlRaiseException));
   // This is a noreturn function, unless it's one of the exceptions raised to
   // communicate with the debugger, such as the one from OutputDebugString.
   if (ExceptionRecord->ExceptionCode != DBG_PRINTEXCEPTION_C)
static void NTAPI asan_thread_init(void *module, DWORD reason, void *reserved) {
   if (reason == DLL_PROCESS_ATTACH) __asan_init();
 }
 
 #pragma section(".CRT$XLAB", long, read)  // NOLINT
 __declspec(allocate(".CRT$XLAB")) void (NTAPI *__asan_tls_init)(void *,
     unsigned long, void *) = asan_thread_init;
 #endif
 
-ASAN_LINK_GLOBALS_WIN()
+WIN_FORCE_LINK(__asan_dso_reg_hook)
 
 // }}}
 }  // namespace __asan
 
 #endif  // SANITIZER_WINDOWS

compiler-rt/trunk/lib/asan/asan_win_dll_thunk.cc

 // See https://github.com/google/sanitizers/issues/209 for the details.
 //===----------------------------------------------------------------------===//
 
 // Only compile this code when building asan_dll_thunk.lib
 // Using #ifdef rather than relying on Makefiles etc.
 // simplifies the build procedure.
 #ifdef ASAN_DLL_THUNK
 #include "asan_init_version.h"
-#include "asan_globals_win.h"
 #include "interception/interception.h"
 #include "sanitizer_common/sanitizer_platform_interceptors.h"
+#include "sanitizer_common/sanitizer_win_defs.h"
 
 #ifdef _M_IX86
 #define WINAPI __stdcall
 #else
 #define WINAPI
 #endif
 
 // ---------- Function interception helper functions and macros ----------- {{{1
static void WINAPI asan_thread_init(void *mod, unsigned long reason,
                                    void *reserved) {
   if (reason == /*DLL_PROCESS_ATTACH=*/1) __asan_init();
 }
 
 #pragma section(".CRT$XLAB", long, read)  // NOLINT
 __declspec(allocate(".CRT$XLAB")) void (WINAPI *__asan_tls_init)(void *,
     unsigned long, void *) = asan_thread_init;
 
-ASAN_LINK_GLOBALS_WIN()
+WIN_FORCE_LINK(__asan_dso_reg_hook)
 
 #endif // ASAN_DLL_THUNK

compiler-rt/trunk/lib/asan/asan_win_dynamic_runtime_thunk.cc

 //  - installing a custom SEH handler
 //
 //===----------------------------------------------------------------------===//
 
 // Only compile this code when building asan_dynamic_runtime_thunk.lib
 // Using #ifdef rather than relying on Makefiles etc.
 // simplifies the build procedure.
 #ifdef ASAN_DYNAMIC_RUNTIME_THUNK
-#include "asan_globals_win.h"
+#include "sanitizer_common/sanitizer_win_defs.h"
 #define WIN32_LEAN_AND_MEAN
 #include <windows.h>
 
 // First, declare CRT sections we'll be using in this file
 #pragma section(".CRT$XIB", long, read)  // NOLINT
 #pragma section(".CRT$XID", long, read)  // NOLINT
 #pragma section(".CRT$XCAB", long, read)  // NOLINT
 #pragma section(".CRT$XTW", long, read)  // NOLINT

 static int SetSEHFilter() { return __asan_set_seh_filter(); }
 
 // Unfortunately, putting a pointer to __asan_set_seh_filter into
 // __asan_intercept_seh gets optimized out, so we have to use an extra function.
 __declspec(allocate(".CRT$XCAB")) int (*__asan_seh_interceptor)() =
     SetSEHFilter;
 }
 
-ASAN_LINK_GLOBALS_WIN()
+WIN_FORCE_LINK(__asan_dso_reg_hook)
 
 #endif // ASAN_DYNAMIC_RUNTIME_THUNK

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

 #include "sanitizer_common.h"
 #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"
+#include "sanitizer_win_defs.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__) && \

 
 #if !SANITIZER_GO
 // Workaround to implement weak hooks on Windows. COFF doesn't directly support
 // weak symbols, but it does support /alternatename, which is similar. If the
 // user does not override the hook, we will use this default definition instead
 // of null.
 extern "C" void __sanitizer_print_memory_profile(int top_percent) {}
 
-#ifdef _WIN64
-#pragma comment(linker, "/alternatename:__sanitizer_print_memory_profile=__sanitizer_default_print_memory_profile") // NOLINT
-#else
-#pragma comment(linker, "/alternatename:___sanitizer_print_memory_profile=___sanitizer_default_print_memory_profile") // NOLINT
-#endif
+WIN_WEAK_ALIAS(__sanitizer_print_memory_profile,
+               __sanitizer_default_print_memory_profile)
 #endif
 
 #endif  // _WIN32

compiler-rt/trunk/lib/sanitizer_common/sanitizer_win_defs.h

+//===-- sanitizer_win_defs.h ------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Common definitions for Windows-specific code.
+//
+//===----------------------------------------------------------------------===//
+#ifndef SANITIZER_WIN_DEFS_H
+#define SANITIZER_WIN_DEFS_H
+
+#include "sanitizer_platform.h"
+#if SANITIZER_WINDOWS
+
+#if defined(_WIN64)
+#define WIN_SYM_PREFIX
+#else
+#define WIN_SYM_PREFIX "_"
+#endif
+
+// Intermediate macro to ensure the parameter is expanded before stringified.
+#define STRINGIFY(A) #A
+
+// ----------------- A workaround for the absence of weak symbols --------------
+// We don't have a direct equivalent of weak symbols when using MSVC, but we can
+// use the /alternatename directive to tell the linker to default a specific
+// symbol to a specific value.
+// Take into account that the function will be marked as UNDEF in the symbol
+// table of the resulting object file, even if we provided a default value, and
+// the linker won't find the default implementation until it links with that
+// object file.
+// So, suppose we provide a default implementation "fundef" for "fun", and this
+// is compiled into the object file "test.obj".
+// If we have some code with references to "fun" and we link that code with
+// "test.obj", it will work because the linker always link object files.
+// But, if "test.obj" is included in a static library, like "test.lib", then the
+// liker will only link to "test.obj" if necessary. If we only included the
+// definition of "fun", it won't link to "test.obj" (from test.lib) because
+// "fun" appears as UNDEF, so it doesn't resolve the symbol "fun", and this will
+// result in a link error.
+// So, a workaround is to force linkage with the modules that include weak
+// definitions, with the following macro: WIN_FORCE_LINK()
+
+#define WIN_WEAK_ALIAS_(Name, Default)                                         \
+  __pragma(comment(linker, "/alternatename:" WIN_SYM_PREFIX STRINGIFY(Name) "="\
+                                             WIN_SYM_PREFIX STRINGIFY(Default)))
+
+#define WIN_WEAK_ALIAS(Name, Default)                                          \
+  WIN_WEAK_ALIAS_(Name, Default)
+
+#define WIN_FORCE_LINK(Name)                                                   \
+  __pragma(comment(linker, "/include:" WIN_SYM_PREFIX STRINGIFY(Name)))
+
+#endif // SANITIZER_WINDOWS
+#endif // SANITIZER_WIN_DEFS_H

compiler-rt/trunk/lib/ubsan/ubsan_flags.cc

 //===----------------------------------------------------------------------===//
 
 #include "ubsan_platform.h"
 #if CAN_SANITIZE_UB
 #include "ubsan_flags.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_flag_parser.h"
+#include "sanitizer_common/sanitizer_win_defs.h"
 
 namespace __ubsan {
 
 const char *MaybeCallUbsanDefaultOptions() {
   return (&__ubsan_default_options) ? __ubsan_default_options() : "";
 }
 
 Flags ubsan_flags;

 
 #if !SANITIZER_SUPPORTS_WEAK_HOOKS
 SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
 const char *__ubsan_default_options() { return ""; }
 #endif
 
 #if SANITIZER_WINDOWS
 const char *__ubsan_default_default_options() { return ""; }
-# ifdef _WIN64
-#  pragma comment(linker, "/alternatename:__ubsan_default_options=__ubsan_default_default_options")
-# else
-#  pragma comment(linker, "/alternatename:___ubsan_default_options=___ubsan_default_default_options")
-# endif
+
+WIN_WEAK_ALIAS(__ubsan_default_options, __ubsan_default_default_options)
 #endif
 
 }  // extern "C"
 
 #endif  // CAN_SANITIZE_UB