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

[Mike Aizatsky via Phabricator via llvm-commits]

aizatsky requested changes to this revision.
aizatsky added a comment.
This revision now requires changes to proceed.

I think we can clean this up a bit.



================
Comment at: lib/asan/asan_allocator.cc:960
 // Provide default (no-op) implementation of malloc hooks.
 extern "C" {
+WEAK_DEF(void, __sanitizer_malloc_hook, void *ptr, uptr size) {
----------------
WEAK_* macros specify `extern "C"`, right? Let's remove this and similar usages then.


================
Comment at: lib/sanitizer_common/sanitizer_common.cc:508
 
-#if !SANITIZER_GO && !SANITIZER_SUPPORTS_WEAK_HOOKS
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_print_memory_profile(int top_percent) {
+#if !SANITIZER_GO
+WEAK_DEF(void, __sanitizer_print_memory_profile, int top_percent) {
----------------
I'm not sure what's up with !SANITIZER_GO here, do you know?


================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep.cc:1021
 // Default empty implementations (weak). Users should redefine them.
-#if !SANITIZER_WINDOWS  // weak does not work on Windows.
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_cmp() {}
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_cmp1() {}
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_cmp2() {}
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_cmp4() {}
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_cmp8() {}
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_switch() {}
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_div4() {}
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_div8() {}
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_gep() {}
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_cov_trace_pc_indir() {}
-#endif  // !SANITIZER_WINDOWS
+WEAK_DEF(void, __sanitizer_cov_trace_cmp, void) {}
+WEAK_DEF(void, __sanitizer_cov_trace_cmp1, void) {}
----------------
Can we get rid of `void` when defining a no-op weak function?

```
WEAK_INTERFACE_DECL(const char *, __ubsan_default_options);
```

It doesn't look like we'd need `##__VA_ARGS__`. 


================
Comment at: lib/sanitizer_common/sanitizer_internal_defs.h:24
 // Only use SANITIZER_*ATTRIBUTE* before the function return type!
 #if SANITIZER_WINDOWS
 # define SANITIZER_INTERFACE_ATTRIBUTE __declspec(dllexport)
----------------
Let's create a top-level weak symbols section documentation here that doesn't mention any platform specifics. It should document intent (weak symbols multi-platform support), top-level macros, their correct usage and give an example.

All platform-specific documentation should be next to implementation like now. 


================
Comment at: lib/sanitizer_common/sanitizer_printf.cc:218
 // Can be overriden in frontend.
 #if SANITIZER_SUPPORTS_WEAK_HOOKS
+WEAK_DEF(void, OnPrint, const char *str) {
----------------
do you still neeed this #if?


================
Comment at: lib/sanitizer_common/sanitizer_printf.cc:218
 // Can be overriden in frontend.
 #if SANITIZER_SUPPORTS_WEAK_HOOKS
+WEAK_DEF(void, OnPrint, const char *str) {
----------------
aizatsky wrote:
> do you still neeed this #if?
Now that you are introducing these macros, should we continue to have SANITIZER_SUPPORTS_WEAK_HOOKS visible to the code? 


https://reviews.llvm.org/D28596