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

Marcos Pividori via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 20 17:39:52 PST 2017


mpividori added inline comments.


================
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) {
----------------
aizatsky wrote:
> WEAK_* macros specify `extern "C"`, right? Let's remove this and similar usages then.
@aizatsky Ok, I can do that.


================
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) {
----------------
aizatsky wrote:
> I'm not sure what's up with !SANITIZER_GO here, do you know?
@aizatsky I don't know, I think we can remove it.
As far as I understand, `SANITIZER_GO` is a flag that we provide from the command line.
For `SANITIZER_GO`  we ignore weak symbols in `sanitizer_internal_defs.h`.


================
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) {}
----------------
aizatsky wrote:
> 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__`. 
@aizatsky Yes, I can do that. Do you think that way it is easier to understand? I can see for the `INTERCEPTOR()` macro, we always make it explicit and write `void`.


================
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)
----------------
aizatsky wrote:
> 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. 
@aizatsky I agree, I will add more documentation.


================
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:
> 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? 
@aizatsky as I don't understand the purpose of `TSAN_EXTERNAL_HOOKS` and `SANITIZER_GO`, I don't modify this implementation. If you prefer I can do that, maybe in a different diff? Also I want to add more changes, for example remove all the conditions like:
```
  if (weak_function)
    weak_function()
```
Which is not necessary anymore, since we always provide a default implementation. Weak functions are always defined. 


https://reviews.llvm.org/D28596





More information about the llvm-commits mailing list