[PATCH] D28596: [compiler-rt] General definition for weak functions.

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl accepted this revision.
alekseyshl added a comment.

LGTM with a few minor comments.



================
Comment at: lib/asan/asan_allocator.cc:961
 // Provide default (no-op) implementation of malloc hooks.
-extern "C" {
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_malloc_hook(void *ptr, uptr size) {
+WEAK_DEF(void, __sanitizer_malloc_hook, void *ptr, uptr size) {
   (void)ptr;
----------------
Since all we have now is this macro, can we be a bit more explicit about the fact that it's an interface function?
SANITIZER_INTERFACE_WEAK_DEF, for example?


================
Comment at: lib/sanitizer_common/sanitizer_internal_defs.h:17
 #include "sanitizer_platform.h"
+#include "sanitizer_win_defs.h"
 
----------------
It does not feel right, unconditionally including win specific header in the generic module. I realize that within sanitizer_win_defs.h there is a #if SANITIZER_WINDOWS guard, but still. Can you move it into #if SANITIZER_WINDOWS block, where it is actually needed?


================
Comment at: lib/sanitizer_common/sanitizer_win_defs.h:62
+
+// In Windows, we can't define weak functions, but we can use WIN_WEAK_ALIAS()
+// which defines an alias to a default implementation, and only works when
----------------
We cannot define weak functions on Windows,


================
Comment at: lib/sanitizer_common/sanitizer_win_defs.h:93
+// some differences:
+//  + Always a default implementation must me provided.
+//  + When linking dynamically with a library (dll), weak functions are
----------------
A default implementation must always be provided


================
Comment at: lib/sanitizer_common/sanitizer_win_defs.h:99
+//  implementation in a different unit is required, the strong definition must
+//  be exported and interception can be used from the rest of the units.
+
----------------
I think your summary explains it in a bit more coherent way, can you expand this comment?


https://reviews.llvm.org/D28596