[compiler-rt] General definition for weak functions.

[mpividori]

Summary

Hi,

After spending some time to understand how "weak aliases" work on Windows, with the undocumented linker option: "alternatename", I came up with 2 general macros for defining weak functions: "WEAK()" and "WEAK_INTERFACE()".
I think this really simplifies the code and these changes are required for my next diffs that fix Sanitizer Coverage for Windows (I don't add these changes only for refactoring, I add these changes because they are necessary to make Sanitizer Coverage work on Windows, and port libFuzzer to Windows which is my final goal).

I provide 2 macros:

+ WEAK(ReturnType, Name, Parameters)  :  for declaring or defining weak functions.
+ WEAK_INTERFACE(ReturnType, Name, Parameters)  :  for declaring or defining weak functions that should be exported, for example for the interface of a library.

So, for example:
+ Declaring a weak function:

WEAK(bool, compare, (int a, int b))

+ Defining a weak function:

WEAK(bool, compare, (int a, int b)) { return a > b; }

In Windows, we don't have a direct equivalent of weak symbols, but we can use the macro "WIN_WEAK_ALIAS()" (defined in https://reviews.llvm.org/D28525) which defines an alias to a default implementation (using the pragma "alternatename"), and only works when linking statically.
To define a weak function "fun", we define a default implementation with a different name "fun__def" and we create a "weak alias" fun = fun__def.
Then, users can override it just defining "fun".
For example:
header.h:

WEAK(bool, compare, (int a, int b))

default.cc:

WEAK(bool, compare, (int a, int b)) { return 0; }

override.cc

extern "C" bool compare (int a, int b) { return a >= b; }

So, until this point, when linking statically, it works quite similar to weak symbols in linux, with the difference that we always need to provide a default implementation.

However, when exposing weak functions in the interface of a shared library on Windows (dll), it is a bit different. We only provide the default implementation (fun__def()).
Clients of that library, only need to include a header with the declaration of that function, which will define a "weak alias" fun = fun__def. So, by default clients will be using the default implementation imported from the dll, and they can override it by redefining the function. For example:
libAheader.h:

WEAK_INTERFACE(bool, compare, (int a, int b))

libAdefault.cc:

WEAK(bool, compare, (int a, int b)) { return 0; }

client.cc

#include "libAheader.h"
// We can use the default implementation from the library:
  compare(1, 2);
// Or we can override it:
  extern "C" bool compare (int a, int b) { return a >= b; }

So, when linking dynamically, it works different to linux.
If some unit overrides the function (redefining fun()), the rest of the units (other dlls, or main executable) don't have access to it.
So, for example, in the previous example. If the client redefines "compare()", the code of the library libA will continue using the default implementation.
When access to the implementation in a different unit is required, interception can be used. (I use this for asan dll, in next diffs)

So, in order to use weak functions in a portable way that works for Windows and Linux, we must:

+ Always provide a default implementation.
+ For dynamically libraries (dll), only provide the default implementation, and don't override it inside the library.

I think this doesn't impose any limitation to the current code for sanitizers. Generally, all we want to do is provide a default implementation that can be overrided by the user.
When providing static libraries, it works fine for both Linux and Windows. When users override the weak function, also the code in the library is updated to refer to that function (because all is statically linked together).
When providing shared libraries, for windows we need to use interception to get a pointer to the function defined by the user and override the default implementation in the dll.

For example, for asan in Windows, we provided 2 implementations, a static library for MT and a shared library for MD. Lets consider the case of shared library (MD).
Client's code is instrumented with some calls to __sanitizer_cov_trace_pc_guard(). By default, that calls will be aliased to use the imported default implementation fom asan dll: __sanitizer_cov_trace_pc_guard__def().
But clients can override it. So, when asan dll is initialized, it will check if the main executable exports the definition of __sanitizer_cov_trace_pc_guard(). If we find that function, then we override the default function in the dll with that pointer. So, all the client's dlls with instrumentation that import __sanitizer_cov_trace_pc_guard__def() from asan dll, will be using the function provided by the main executable.

Thanks,
Marcos

[rnk]

Looks good to me. I think this is the best we can do to abstract over the differences between ELF weak functions and COFF weak functions. We should wait for @kcc to comment though, since this impacts them significantly.

[rnk]

lib/sanitizer_common/sanitizer_internal_defs.h

74 ↗

(On Diff #84073)

We should consider removing this macro eventually and migrate all users over to the new API.

[mpividori]

lib/sanitizer_common/sanitizer_internal_defs.h

74 ↗

(On Diff #84073)

Exactly, but I was waiting for review on the new macro before migrating everything.

[rnk]

lib/sanitizer_common/sanitizer_internal_defs.h

42 ↗

(On Diff #84073)

After looking at D28597 and D28598, I realized how this usage of WIN_WEAK_ALIAS makes it so that every weak declaration now introduces a #pragma comment(linker, "/alternatename:..."). That seems bad. In particular, I don't like how including sanitizer_interface_internal.h in D28598 is used for the side effect of providing aliases for the sanitizer coverage callbacks.

It might be nicer to provide separate macros for declaring and defining weak functions. On Windows, weak function declarations wouldn't have anything special added. Then, at the definition, they would provide the pragma and the default definition.

60 ↗

(On Diff #84073)

typo on "dynamically"

[mpividori]

lib/sanitizer_common/sanitizer_internal_defs.h

42 ↗

(On Diff #84073)

@rnk, yes, it "looks bad". But works really well and I strongly think this is appropriate for this case, I will explain why:

"Windows "weak aliases" are not exposed on object files"

As I commented in the definition of WIN_WEAK_ALIAS(), if we define an alias: "fun = fun_def", the compiler doesn't include "fun" in the symbol table of the object file. (if we refer to "fun" inside that object file, it will include "fun" to the symbol table, but as UNDEF, which for the purpose of this explanation is the same)
So, when linking to an object file that includes the default definition ant the alias, it will works fine because the linker will consider that object file even if it doesn't resolve any symbol, and it will find the definition, but when the default definition is included in a static library, the linker could ignore the object file where "fun" is defined (because "fun" is not included in the symbol table, or is included as UNDEF) and result in unresolved symbol.
For example:

// definition.h
  int fun();
  int fun__def();
// definition.cc
extern "C" {
  WIN_WEAK_ALIAS(fun, fun__def)
  int fun__def() {
    return 1;
  }
}
// main.cc
#include "definition.h"
int main() {
  return fun();
}

This works fine:

clang-cl /c definition.cc
clang-cl main.cc definition.obj

This fails with unresolved symbol:

clang-cl /c definition.cc
lib definitions.obj
clang-cl main.cc definition.lib

So, I think it is OK to include the weak alias in the declaration, so everywhere you refer to "fun" you include the weak alias "fun = fun__def".
The linker should be able to remove that alias when "fun" is not used. And we avoid the problem of dealing with static libraries.
For example:

// definition.h
  int fun();
  int fun__def();
  WIN_WEAK_ALIAS(fun, fun__def)
// definition.cc
extern "C" {
  int fun__def() {
    return 1;
  }
}
// main.cc
#include "definition.h"
int main() {
  return fun();
}

This will works fine:

clang-cl /c definition.cc
lib definitions.obj
clang-cl main.cc definition.lib

Would you agree?

[mpividori]

@rnk I updated the diff to add suggested changes. Would you agree? (In future, if we have link errors with weak symbols we must remember to use WIN_FORCE_LINK() to force linking to the object defining a weak symbol, but now, fortunately, it is not necessary, since weak symbols are defined in modules that define other non-weak symbols, so they are always included by the linker).

[aizatsky]

I don't like that we introduce macros making function definition ugly and opaque. All this to implement one platform hacks. I would prefer to have current version cleaned up to eliminate ifdefs. Something like:

SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
const char *__ubsan_default_options() { return ""; }

WIN_WEAK_FN(__ubsan_default_options);

Is such clenup possible? What properties does proposed solution have that cleaned up one doesn't?

[mpividori]

@aizatsky I updated the code 2 minutes before your comment.

I think the two macros that I am defining: WEAK_DEF() and WEAK_INTERFACE_DECL() are very simple to understand and simplify the code removing redundant code.

The solution that you propose has some disadvantages:

• For Linux, we don't need to define a different function for the default implementation. So, with your proposed solution we have 2 approaches:

+) or we define the default function and we create a weak alias in linux too:

  SANITIZER_INTERFACE_ATTRIBUTE const char *__ubsan_options__def() { return ""; }
#ifdef SANITIZER_WINDOWS
  SANITIZER_INTERFACE_ATTRIBUTE const char *__ubsan_options();
  WIN_WEAK_ALIAS(__ubsan_options, __ubsan_options__def);
#else
  SANITIZER_INTERFACE_ATTRIBUTE const char *__ubsan_options()  __attribute__ ((weak, alias ("___ubsan_options__def")));;
#endif

(I guess weak aliases also work for APPLE)
We could generalize the weak alias in a macro to avoid the #ifdefs .

+) Or we can use some #ifdef to define the default function only for Windows:

#ifdef SANITIZER_WINDOWS
  SANITIZER_INTERFACE_ATTRIBUTE const char *__ubsan_options__def() { return ""; }
  SANITIZER_INTERFACE_ATTRIBUTE const char *__ubsan_options();
  WIN_WEAK_ALIAS(__ubsan_options, __ubsan_options__def);
#else
  SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE const char *__ubsan_options(){ return ""; }
#endif

In this case we have to duplicate the code for the default implementation.

With the macro WEAK_DEF() that I define, you can see that I simply the code, since we don't need any special code for different platforms:

WEAK_DEF(const char *, __ubsan_options, ()) { return ""; }

This works fine for Windows and other platforms.

• Also, we need to export default functions but only on Windows, with your approach, this means I should write this code in the header file:

#ifdef SANITIZER_WINDOWS
  const char *__ubsan_options();
  SANITIZER_INTERFACE_ATTRIBUTE const char *__ubsan_options__def();
#else
  SANITIZER_INTERFACE_ATTRIBUTE const char *__ubsan_options();
#endif

Which I achieve with the macro WEAK_INTERFACE_DECL(), and simplifies that code to:

WEAK_INTERFACE_DECL(const char *, __ubsan_options, ());

[aizatsky]

I suggest to make WIN* macros no-op on all platforms that do not need it.

#if WINDOWS
  #define WIN_WEAK_FN(...) ...
#else
  #define WIN_WEAK_FN(...)
#end

And have the function definition code look like:

SANITIZER_WEAK_ATTRIBUTE const char *__ubsan_default_options() { return ""; }
WIN_WEAK_FN(__ubsan_default_options, ...)

Notice the absence of ifdefs in the client code and C++ syntax for simple cases.

How about your solution and other platforms? E.g. mac? If you want to push such a heavy interface change you at least should think through how it will solve other problems as well. Does it help other platforms? It wouldn't be great if we had to introduce another level of complexity for another platform.

[aizatsky]

Another wild thought: is there any template magic that we might invoke to automate creation of function aliases?

[mpividori]

But that doesn't work, we shouldn't define __ubsan_default_options on Windows. It must be a "weak alias" to a default implementation "ubsan_default_optionsdef".
And "ubsan_default_optionsdef" shouldn't exist in non-windows systems, where you can define weak functions.

[kubamracek]

I also prefer to keep the function declarations as they are. Otherwise IDEs can't find the function. Another idea: Is there a way to rename the function with an attribute? Or e.g. with __asm ("another_name"); (this works on Darwin)?

[zturner]

In D28596#649055, @aizatsky wrote:

Another wild thought: is there any template magic that we might invoke to automate creation of function aliases?

I don't think a template would work because of the attributes and the fact that the name has to be different. You couldn't propagate the name through a template, it has to be done via macro. I'm trying to think of alternatives, but nothing really comes to mind. Will give it some more thought.

[mpividori]

Another option (as I mentioned in https://reviews.llvm.org/D28525#641954 ), that will also simplify the code, is to ALWAYS use weak aliases instead of weak functions.
So, always define a default implementation with "__def" suffix, and then define a weak alias: fun = fun__def
I am not sure if weak aliases work on Apple. If that is the case, this would STRONGLY SIMPLIFY the code since we take EXACTLY the same approach for all the platforms.
So, this is the idea:

• we define a general macro for weak aliases:

#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

• We define a macro to represent the name of the default implemenation:

#define WEAK_DEFAULT_NAME(Name) Name##__def

• For each weak function "fun" we provide a default implementation and weak alias:

void WEAK_DEFAULT_NAME(fun)(int a, int b) {
   // Default implementation .....
}
WEAK_ALIAS(fun, WEAK_DEFAULT_NAME(fun))

What do you think? I whink this is the best we can do, and, at the same time, keep the code similar for all platforms.

[mpividori]

(I am using weak pragmas for linux: https://gcc.gnu.org/onlinedocs/gcc/Weak-Pragmas.html)

So, in Windows we only have "weak aliases". In Linux, we have both, weak functions and weak aliases.
I would like to know which options we have in Darwin. If we can use weak aliases for Darwin, it would be simple, since we can use weak aliases for all the platforms.
If not, we need different approaches.

[kubamracek]

In D28596#649126, @mpividori wrote:

(I am using weak pragmas for linux: https://gcc.gnu.org/onlinedocs/gcc/Weak-Pragmas.html)

So, in Windows we only have "weak aliases". In Linux, we have both, weak functions and weak aliases.
I would like to know which options we have in Darwin. If we can use weak aliases for Darwin, it would be simple, since we can use weak aliases for all the platforms.
If not, we need different approaches.

Weak aliases don't work on Darwin.

[mpividori]

@kubabrecka @aizatsky Ok, thanks for your comments.

So, we can't take the same approach for all platforms because: Windows supports "weak aliases" , Linux support weak functions and weak aliases, Darwin support weak functions.
So, we need to continue with this approach:

• Weak functions with default implementation for Linux and Darwin:

__attribute__ ((weak)) void fun () {
  // Default implementation
}

• Use an auxiliar function for windows and define a weak alias:

void fun_def() {
  // Default implementation
}
WIN_WEAK_ALIAS(fun, fun__def)

Do you agree on this point? Would you do it differently?

---

So, if we include that code, we have some duplicated code:

#if ! SANITIZER_WINDOWS
 __attribute__ ((weak)) void fun () {
   // Default implementation
 }
#else
 void fun_def() {
   // Default implementation
 }
 WIN_WEAK_ALIAS(fun, fun__def)
#endif

To avoid duplicated code, I will define a macro WEAK_DEF(), so we can reduce that code to:

WEAK_DEF(void, fun, ()) {
   // Default implementation
 }

Would you agree on this point? Would you do it differently?

---

In the headers, I also need to export the default function on Windows ("fun__def").
So, we can use "#ifdef" and declare the functions as:

SANITIZER_INTERFACE_ATTRIBUTE void fun();
#ifdef SANITIZER_WINDOWS
SANITIZER_INTERFACE_ATTRIBUTE void fun__def();
#endif

To avoid that, I will define a macro WEAK_INTERFACE_DECL(), so we can reduce that code to:

WEAK_INTERFACE_DECL(void, fun, ());

Would you agree on this point? Would you do it differently?

---

As we declare many weak functions in different sanitizers, not only in sanitizer_common, I thought it would be appropriate to use the macros that I mention instead of using "#ifdefs".

I would appreciate your comment on this. I really want to finish with these changes to continue focused on libFuzzer.

Thanks.

[kubamracek]

To avoid duplicated code, I will define a macro WEAK_DEF(), so we can reduce that code to:

WEAK_DEF(void, fun, ()) {
  // Default implementation
}

Would you agree on this point? Would you do it differently?

I don't like that this is not a "standard" function declaration. It's not obvious that this declares a function. My editor/IDE will not recognize this as a function, and "Go to definition" and such will not work. Is there really no way of keeping the syntax of...:

WHATEVER_MACROS_NECESSARY
void fun() MAYBE_OTHER_MACROS {
  ...code...
}

?

[mpividori]

In D28596#649214, @kubabrecka wrote:

To avoid duplicated code, I will define a macro WEAK_DEF(), so we can reduce that code to:

WEAK_DEF(void, fun, ()) {
  // Default implementation
}

Would you agree on this point? Would you do it differently?

I don't like that this is not a "standard" function declaration. It's not obvious that this declares a function. My editor/IDE will not recognize this as a function, and "Go to definition" and such will not work. Is there really no way of keeping the syntax of...:

WHATEVER_MACROS_NECESSARY
void fun() MAYBE_OTHER_MACROS {
  ...code...
}

?

Hi @kubabrecka ,
Thanks for your reply.
I think the macro name WEAK_DEF() makes it explicit that we are defining a function. Also, I am writing the components in the same order: return value, function name, parameters and function body. I think it is easy to understand by any user, so the only problem is the editor.
I can't keep the syntax as you suggest, because I need to define a function with a different name adding the suffix "__def" and also define a weak alias, and declare the original function. So, definitely this can not be accomplished in that way.
Also, is the same approach used for the definition of the macros: INTERCEPTOR() , DECLARE_REAL() (in: interception/interception.h), for RECOVERABLE() and UNRECOVERABLE() (in: ubsan/ubsan_handlers.h), LLVM_SYMBOLIZER_INTERCEPTOR*() (in: sanitizer_common/symbolizer/sanitizer_wrappers), etc. So, this is also used in other parts of compiler-rt repository.
As weak functions are a small portion of the definitions, I think this won't generate many problems to users working with an editor that doesn't expand the macros.
Also, I think the advantages are considerably more relevant than that disadvantage. We can define weak functions that will work in all platforms with a unique macro, without boilerplate code. If we want to continue supporting Linux, Darwin and Windows, I think this is useful.
Would you agree?
Thanks

[kubamracek]

We can define weak functions that will work in all platforms with a unique macro, without boilerplate code. If we want to continue supporting Linux, Darwin and Windows, I think this is useful.

Oh, I absolute agree with that.

The macros you mentioned (INTERCEPTOR, DECLARE_REAL, RECOVERABLE, and TEST from gtest) and a bit of mess that doesn't have a unified syntax of the user side. Gtest's TEST() macro sounds like a different story to me, because it doesn't have parameters nor return type -- we only have TEST(classname, testname) { code }, which is completely reasonable. The syntax of INTERCEPTOR() is very close to what you're proposing here, and frankly I don't like the syntax of INTERCEPTOR either.

I need to define a function with a different name adding the suffix "__def" and also define a weak alias, and declare the original function. So, definitely this can not be accomplished in that way.

Have you looked into e.g. changing the exported symbol names with /export:...=...? Couldn't this help? Could we use it to basically switch the two functions?

If there's really no other way, then I suggest either:

1. Changing WEAK_DEF to drop the extra set of parentheses to have parity with INTERCEPTOR macro. I.e.: WEAK_DEF(void, OnPrint, const char *str) { code }.
2. Having a macro only around the function name, i.e.: WEAK_ATTRIBUTE void WEAK_NAME(OnPrint)(const char *str) { code }.

[mpividori]

Hi @kubabrecka ,
Thanks for your suggestions.

I need to define a function with a different name adding the suffix "__def" and also define a weak alias, and declare the original function. So, definitely this can not be accomplished in that way.

Have you looked into e.g. changing the exported symbol names with /export:...=...? Couldn't this help? Could we use it to basically switch the two functions?

Unfortunately, /export:...=... doesn't help.

If there's really no other way, then I suggest either:

1. Changing WEAK_DEF to drop the extra set of parentheses to have parity with INTERCEPTOR macro. I.e.: WEAK_DEF(void, OnPrint, const char *str) { code }.

Ok. Yes, I agree. I will do that.

2. Having a macro only around the function name, i.e.: WEAK_ATTRIBUTE void WEAK_NAME(OnPrint)(const char *str) { code }.

I can't do that because not only I need to change the name, I also need to add a linker pragma WIN_WEAK_ALIAS(OnPrint, OnPrint__def) and declare OnPrint.
I mean, for non-windows OnPrint definition will be:

void OnPrint(const char *str) {
  // Default implementation
}

And for Windows, it will be:

void OnPrint__def(const char *str) {
  // Default implementation
}
void OnPrint(const char *str);
WIN_WEAK_ALIAS(OnPrint, OnPrint__def)

Thanks.

[zturner]

In D28596#649978, @mpividori wrote:

2. Having a macro only around the function name, i.e.: WEAK_ATTRIBUTE void WEAK_NAME(OnPrint)(const char *str) { code }.

I can't do that because not only I need to change the name, I also need to add a linker pragma WIN_WEAK_ALIAS(OnPrint, OnPrint__def) and declare OnPrint.
I mean, for non-windows OnPrint definition will be:

void OnPrint(const char *str) {
  // Default implementation
}

And for Windows, it will be:

void OnPrint__def(const char *str) {
  // Default implementation
}
void OnPrint(const char *str);
WIN_WEAK_ALIAS(OnPrint, OnPrint__def)

Thanks.

What if instead of getting it all into one line, we settle for this:

void WEAK_FN_DEF(OnPrint)(const char *str) {
}
WEAK_FN_ALIAS(OnPrint)

The macro WEAK_FN_ALIAS could look like this:

#define WEAK_FN_ALIAS(Fn) \
  decltype(Fn##__def) Fn; \
  WIN_WEAK_ALIAS(Fn, Fn##__def)

On non-Windows platforms, it would just do nothing. Apparently the crazy decltype magic works and should give you a definition named OnPrint__def and a forward declaration named OnPrint.

[kubamracek]

Yes, what @zturner proposes works. @kubabrecka Would that make your editor work fine?
I would prefer to use only one macro as I defined it before, because if we use 2 macros, we always need to remember to include both.
But just let me know what you prefer and I will do that.

The only way i can think to improve that is to make it so WEAK_ALIAS macro comes first, and make WEAK_DEF comes second and static_assert if WEAK_ALIAS has not been called (by using sizeof on the forward decl perhaps). This way you will get a compiler error if you don't do both.

I don't even know if this would work (and i also don't think it should hold up this change regardless since it's only a minor improvement).

That said, I'm fine with any approach that works, so I'll leave it up kuba@ and aizatsky@ to decide which they like better

Both solutions work by me.

What about FreeBSD? It hasn't been mentioned in this thread.

[mpividori]

@kubabrecka I don't change the implementation for non-windows systems. I only modify the code to use a macro, but after the preprocessor the resulting code is nearly the same. I guess weak functions works for FreeBSD.

[mpividori]

@kubabrecka according to the definition of SANITIZER_WEAK_ATTRIBUTE in sanitizer_common/sanitizer_internal_defs.h, for FreeBSD we are considering __attribute__((weak)). So, in this diff I don't change the behavior on FreeBSD.
Thanks.

[rnk]

In D28596#649052, @aizatsky wrote:

I suggest to make WIN* macros no-op on all platforms that do not need it.

#if WINDOWS
  #define WIN_WEAK_FN(...) ...
#else
  #define WIN_WEAK_FN(...)
#end

And have the function definition code look like:

SANITIZER_WEAK_ATTRIBUTE const char *__ubsan_default_options() { return ""; }
WIN_WEAK_FN(__ubsan_default_options, ...)

Notice the absence of ifdefs in the client code and C++ syntax for simple cases.

How about your solution and other platforms? E.g. mac? If you want to push such a heavy interface change you at least should think through how it will solve other problems as well. Does it help other platforms? It wouldn't be great if we had to introduce another level of complexity for another platform.

I don't want it to be possible for non-Windows developers to forget to use WIN_WEAK_FN. I would prefer it if the macro wrapped the function declaration so that it can't be forgotten. The Windows build is too often broken when adding weak functions.

[kubamracek]

I don't want it to be possible for non-Windows developers to forget to use WIN_WEAK_FN. I would prefer it if the macro wrapped the function declaration so that it can't be forgotten. The Windows build is too often broken when adding weak functions.

Makes sense. Marcos, can we get an updated patch? I'll test it on Darwin.

[mpividori]

Hi,
I updated the diff to include requested changes. I define 2 macros: WEAK_DEF() and WEAK_INTERFACE_DECL().

Tests pass on Linux.

Now, if we use WEAK_DEF() to define a weak function, it will work on Windows too, as long as:

• We only define it once. This means, only one definition when linking. (This is different to weak symbols in linux, where we can define it many times in different TU, and the linker will choose the first that it finds). If this generates problems in the future (this doesn't happen now, but could happen when combining many sanitizers on Windows, providing the default weak implementation twice), this could be solved modifying the macro, WEAK_DEFAULT_NAME() to generate different default names for different sanitizers, for example, for asan, define fun__def__asan and for lsan, define fun__def__lsan.
• We link statically.

When linking dynamically, weak functions are exported with the __dll suffix.
So, on the client side, we can define a weak alias: WIN_WEAK_ALIAS(fun , fun__dll) and use the default implementation or override it, redefining "fun". This is what I do in nexts diffs to fix Sanitizer Coverage for Windows. With the difference that I include the "windows weak aliases" in a static library which is included by clang driver (clang-rt_asan_dynamic_runtime_thunk-arch.lib), so they don't need to do anything different to make it work on Windows.

Thanks.

[kubamracek]

Can you rebase? The patch doesn't apply cleanly on SVN trunk.

[mpividori]

Really? I rebased some hours ago. Ok I will rebase again.

[mpividori]

@kubamracek I see the problem. You need to apply this patch before: https://reviews.llvm.org/D28525

[kubamracek]

In D28596#651838, @mpividori wrote:

@kubamracek I see the problem. You need to apply this patch before: https://reviews.llvm.org/D28525

That diff looks accepted by Reid. Can you land it first?

[mpividori]

Ok, I agree

[mpividori]

@kubamracek done.

[kubamracek]

The patch passes all tests on Darwin.

[mpividori]

Great! @zturner @aizatsky @rnk ready to land?

[aizatsky]

I think we can clean this up a bit.

lib/asan/asan_allocator.cc

960 ↗

(On Diff #85181)

WEAK_* macros specify extern "C", right? Let's remove this and similar usages then.

lib/sanitizer_common/sanitizer_common.cc

508 ↗

(On Diff #85181)

I'm not sure what's up with !SANITIZER_GO here, do you know?

lib/sanitizer_common/sanitizer_coverage_libcdep.cc

1021 ↗

(On Diff #85181)

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__.

lib/sanitizer_common/sanitizer_internal_defs.h

24 ↗

(On Diff #85181)

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.

lib/sanitizer_common/sanitizer_printf.cc

218 ↗

(On Diff #85181)

do you still neeed this #if?

218 ↗

(On Diff #85181)

Now that you are introducing these macros, should we continue to have SANITIZER_SUPPORTS_WEAK_HOOKS visible to the code?

[mpividori]

lib/asan/asan_allocator.cc

960 ↗

(On Diff #85181)

@aizatsky Ok, I can do that.

lib/sanitizer_common/sanitizer_common.cc

508 ↗

(On Diff #85181)

@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.

lib/sanitizer_common/sanitizer_coverage_libcdep.cc

1021 ↗

(On Diff #85181)

@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.

lib/sanitizer_common/sanitizer_internal_defs.h

24 ↗

(On Diff #85181)

@aizatsky I agree, I will add more documentation.

lib/sanitizer_common/sanitizer_printf.cc

218 ↗

(On Diff #85181)

@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.

[mpividori]

lib/sanitizer_common/sanitizer_common.cc

508 ↗

(On Diff #85181)

@aizatsky SANITIZER_GO flag is only used to build ThreadSanitizer runtime for Go. This condition was added in r285400: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20161024/400704.html I think we can remove it, but to avoid problems, I don't.

[mpividori]

lib/sanitizer_common/sanitizer_coverage_libcdep.cc

1021 ↗

(On Diff #85181)

@aizatsky If I do so, the compiler generates a lot of warnings: warning: must specify at least one argument for ... parameter of variadic macro

[mpividori]

Hi,
I add a new diff to include suggested changes:

• I move Windows specific code to sanitizer_win_defs.h.
• I include a general explanation on using WEAK_DEF() and WEAK_DECL() in sanitizer_internal_defs.h.
• I remove redundant extern "c".
• I clarify the purpose of SANITIZER_SUPPORT_WEAK_HOOKS and add some documentation.

So, to simplify the code, we always provide a default implementation for weak functions.

But, for some special cases, like weak hooks __sanitizer_[malloc|free]_hook() that will be called very often, we want to avoid the overhead of executing the default implementation when it is not necessary. (this overhead really matters? only the overhead of function call, because the default implmentation is empty... maybe...)
So, for weak hooks, I don't modify the code for platforms that can use real weak symbols that will evaluate to a null pointer when not defined. I only define the default implementation for platforms that doesn't support weak hooks, like:

#if !SANITIZER_SUPPORT_WEAK_HOOKS
WEAK_DEF(void, __sanitizer_malloc_hook, void *ptr, uptr size) {
   (void)ptr;
   (void)size;
}
#endif

Maybe this doesn't represent a relevant difference in practice and we can remove the #if #endif and always provide a default implementation for weak hooks too.

So, the final idea is this:

• WEAK_DECL() for declaring a weak function.
• WEAK_DEF() for defining a default weak implementation.
• If we don't want to provide a a default implementation when not necessary and avoid the overhead:

#if !SANITIZER_SUPPORT_WEAK_HOOKS
`WEAK_DEF( ... weak_hook ...)` { // default}
#endif

[mpividori]

@aizatsky Can me move forward?

After this diff, I would be grateful if you could consider these diffs in this order:

• https://reviews.llvm.org/D28434
• https://reviews.llvm.org/D28435
• https://reviews.llvm.org/D28597
• https://reviews.llvm.org/D28598
• https://reviews.llvm.org/D28600
• https://reviews.llvm.org/D28601

So we finish fixing Sanitizer Coverage for Windows and I can continue working on libFuzzer.
Thanks,

[rnk]

lib/sanitizer_common/sanitizer_win_defs.h

91 ↗

(On Diff #85251)

Remind me why we need to declare the *__dll variants in headers? Can this be a .cc file implementation detail?

If we don't need to declare both foo and foo__dll, then we can get rid of WEAK_DECL and go back to something more like:

extern "C" void SANITIZER_WEAK_DECL sancov_blah(int params);

[mpividori]

lib/sanitizer_common/sanitizer_win_defs.h

91 ↗

(On Diff #85251)

@rnk I think that macro is appropriate. Every time we declare a weak function in Windows, we are declaring 2 functions. So, when we include that header, we are also including both declarations, which is what is expected.
So, for example in: https://reviews.llvm.org/D28598 when I include the header: sanitizer_common/sanitizer_interface_internal.h , it include both declarations, and I don't need to add extra declarations.

[mpividori]

@rnk Ok, I realized you are right, we can remove WEAK_DECL and continue using declarations like: void SANITIZER_WEAK_ATTRIBUTE ... , because we only need to refer to the exported function "fun__dll" on a linker pragma WEAK_ALIAS(...), so we don't need a declaration.
So, now, I only define a macro "WEAK_DEF()".
Thanks.

[rnk]

lgtm Thanks!

[zturner]

+alekseyshl

Is there anything that remains to be done on this revision? IIUC all concerns have been addressed. kcc@, aizatsky@, alekseyshl@, kubamracek@: Do you guys have any further issues or is this ok to go in?

[kubamracek]

LGTM.

[mpividori]

In this diff I modify the name of exported weak symbols, so I need to temporarily disable a win test which check the interfaces. I will fix that test in a different diff, after all the refactoring to windows sanitizer.

[alekseyshl]

LGTM with a few minor comments.

lib/asan/asan_allocator.cc

961 ↗

(On Diff #85804)

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?

lib/sanitizer_common/sanitizer_internal_defs.h

17 ↗

(On Diff #85804)

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?

lib/sanitizer_common/sanitizer_win_defs.h

62 ↗

(On Diff #85804)

We cannot define weak functions on Windows,

93 ↗

(On Diff #85804)

A default implementation must always be provided

99 ↗

(On Diff #85804)

I think your summary explains it in a bit more coherent way, can you expand this comment?

[mpividori]

lib/asan/asan_allocator.cc

961 ↗

(On Diff #85804)

@alekseyshl Ok, I agree. I will change the macro.

lib/sanitizer_common/sanitizer_internal_defs.h

17 ↗

(On Diff #85804)

@alekseyshl Ok I can change that, but in https://reviews.llvm.org/D28525 they suggested that I simply include the header and only use #ifdef SANITIZER_WINDOWS inside the header, to simplify the code. Whatever you prefer is ok for me.

[alekseyshl]

lib/sanitizer_common/sanitizer_internal_defs.h

17 ↗

(On Diff #85804)

To me, D28525 can be justified by the fact that it's a .cc file containing both win and non-win code, hence both types of headers are there (although I'd rather split it into platform specific files and have a check inside sanitizer_win_defs.h that SANITIZER_WINDOWS is defined, but anyway).

Here you're defining things, you already have those #if blocks, move it into the first one requiring definitions from this file.

Just and idea, you don't have to do anything about it: if we take a step back, whatever definitions use from sanitizer_win_defs.h, are they used anywhere else? My (limited) understandin so far, now the rest of the code is going to use WEAK_DEF macro, everything else is its implementation detail, right? If so, why not move all those definitions to sanitizer_internal_defs.h under #if SANITIZER_WINDOWS? Wouldn't it express better what we're trying to achieve?

[mpividori]

@alekseyshl Thanks for your feedback.

Yes, I agree about the #ifdefs. After this diff I only include sanitizer_win_defs.h in files including windows code, except sanitizer_internal_def.h, so I think I can simply use an "#ifdef" there.

I created 2 macros: WIN_WEAK_IMPORT_DEF and WIN_WEAK_EXPORT_DEF and I included their definition in sanitizer_win_defs.h because this simplifies the code in sanitizer_internal_defs.h. Otherwise, I would be including a lot of Windows specific comments there which make the code confusing.

Also, I need WIN_WEAK_IMPORT_DEF for other windows specific files, in the next diffs.

[mpividori]

@alekseyshl , I include requested changes.

[alekseyshl]

LGTM

lib/asan/asan_win_dll_thunk.cc

337 ↗

(On Diff #86154)

If you don't need those two functions, just delete them.

[bcmills]

According to git bisect, this broke lib/tsan/go/build.bat, and it seems to be broken ever since.

[rnk]

In D28596#1096267, @bcmills wrote:

According to git bisect, this broke lib/tsan/go/build.bat, and it seems to be broken ever since.

I don't believe that runs continuously anywhere, so it doesn't surprise me that it's been broken for over a year. So far as I can tell, the tsan + Windows + go build is unmaintained. =/ It'd be nice to get it straightened out.

[dvyukov]

Turns out this broke an important part of our internal integration because sanitizer::OnPrint become OnPrint, so existing hook stopped working. We are clearly missing some tests.
But exporting and calling a global extern "C" OnPrint does not look OK. All functions need to be either extern "C" and sanitizer_ prefixed, or in __sanitizer:: namespace.

Now if you build:

#include <stdio.h>
void OnPrint() { printf("OnPrint\n"); }
int main() {}

It crashes with:

$ TSAN_OPTIONS=verbosity=1 ./a.out

103485==Installed the sigaction for signal 11

ThreadSanitizer:DEADLYSIGNAL
ThreadSanitizer:DEADLYSIGNAL
ThreadSanitizer: nested bug in the same thread, aborting.

[rnk]

I see. I audited the other instances of SANITIZER_INTERFACE_WEAK_DEF, and they all seem to be properly namespaced into __asan_, __sanitizer_, etc.

Do you (or someone else from dynamic-tools) mind driving the fix for this, since you control the internal integration for this and would know where to add the missing test? Again, sorry for the breakage.

[morehouse]

In D28596#1680946, @rnk wrote:

I see. I audited the other instances of SANITIZER_INTERFACE_WEAK_DEF, and they all seem to be properly namespaced into __asan_, __sanitizer_, etc.

Do you (or someone else from dynamic-tools) mind driving the fix for this, since you control the internal integration for this and would know where to add the missing test? Again, sorry for the breakage.

Proposed fix in https://reviews.llvm.org/D67987.

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

if (allocated_size == 0) {
     GET_STACK_TRACE_FATAL_HERE;
     ReportSanitizerGetAllocatedSizeNotOwned(ptr, &stack);
   }
   return allocated_size;
 }
 
 #if !SANITIZER_SUPPORTS_WEAK_HOOKS
 // Provide default (no-op) implementation of malloc hooks.
-extern "C" {
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_malloc_hook(void *ptr, uptr size) {
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_malloc_hook,
+                             void *ptr, uptr size) {
   (void)ptr;
   (void)size;
 }
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_free_hook(void *ptr) {
+
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_free_hook, void *ptr) {
   (void)ptr;
 }
-} // extern "C"
 #endif

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

#endif
   if (!f->replace_str && common_flags()->intercept_strchr) {
     Report("WARNING: strchr* interceptors are enabled even though "
            "replace_str=0. Use intercept_strchr=0 to disable them.");
   }
 }
 
 }  // namespace __asan
 
-#if !SANITIZER_SUPPORTS_WEAK_HOOKS
-extern "C" {
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-const char* __asan_default_options() { return ""; }
-}  // extern "C"
-#endif
+SANITIZER_INTERFACE_WEAK_DEF(const char*, __asan_default_options, void) {
+  return "";
+}

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

void __sanitizer_ptr_sub(void *a, void *b) {
   CheckForInvalidPointerPair(a, b);
 }
 SANITIZER_INTERFACE_ATTRIBUTE
 void __sanitizer_ptr_cmp(void *a, void *b) {
   CheckForInvalidPointerPair(a, b);
 }
 } // extern "C"
 
-#if !SANITIZER_SUPPORTS_WEAK_HOOKS
 // Provide default implementation of __asan_on_error that does nothing
 // and may be overriden by user.
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE NOINLINE
-void __asan_on_error() {}
-#endif
+SANITIZER_INTERFACE_WEAK_DEF(void, __asan_on_error, void) {}

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

 static const char kInterceptorName[] = "interceptor_name";
 static const char kInterceptorViaFunction[] = "interceptor_via_fun";
 static const char kInterceptorViaLibrary[] = "interceptor_via_lib";
 static const char kODRViolation[] = "odr_violation";
 static const char *kSuppressionTypes[] = {
     kInterceptorName, kInterceptorViaFunction, kInterceptorViaLibrary,
     kODRViolation};
 
-extern "C" {
-#if SANITIZER_SUPPORTS_WEAK_HOOKS
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-const char *__asan_default_suppressions();
-#else
-// No week hooks, provide empty implementation.
-const char *__asan_default_suppressions() { return ""; }
-#endif  // SANITIZER_SUPPORTS_WEAK_HOOKS
-}  // extern "C"
+SANITIZER_INTERFACE_WEAK_DEF(const char *, __asan_default_suppressions, void) {
+  return "";
+}
 
 void InitializeSuppressions() {
   CHECK_EQ(nullptr, suppression_ctx);
   suppression_ctx = new (suppression_placeholder)  // NOLINT
       SuppressionContext(kSuppressionTypes, ARRAY_SIZE(kSuppressionTypes));
   suppression_ctx->ParseFromFile(flags()->suppressions);
   if (&__asan_default_suppressions)
     suppression_ctx->Parse(__asan_default_suppressions());

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

int __asan_should_detect_stack_use_after_return() {
   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;
 }
-
-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() {}
-
-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)

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

 INTERFACE_FUNCTION(__sanitizer_cov_dump)
 INTERFACE_FUNCTION(__sanitizer_dump_coverage)
 INTERFACE_FUNCTION(__sanitizer_dump_trace_pc_guard_coverage)
 INTERFACE_FUNCTION(__sanitizer_cov_indir_call16)
 INTERFACE_FUNCTION(__sanitizer_cov_init)
 INTERFACE_FUNCTION(__sanitizer_cov_module_init)
 INTERFACE_FUNCTION(__sanitizer_cov_trace_basic_block)
 INTERFACE_FUNCTION(__sanitizer_cov_trace_func_enter)
-INTERFACE_FUNCTION(__sanitizer_cov_trace_pc_guard)
-INTERFACE_FUNCTION(__sanitizer_cov_trace_pc_guard_init)
 INTERFACE_FUNCTION(__sanitizer_cov_with_check)
 INTERFACE_FUNCTION(__sanitizer_get_allocated_size)
 INTERFACE_FUNCTION(__sanitizer_get_coverage_guards)
 INTERFACE_FUNCTION(__sanitizer_get_current_allocated_bytes)
 INTERFACE_FUNCTION(__sanitizer_get_estimated_allocated_size)
 INTERFACE_FUNCTION(__sanitizer_get_free_bytes)
 INTERFACE_FUNCTION(__sanitizer_get_heap_size)
 INTERFACE_FUNCTION(__sanitizer_get_ownership)

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

 // Sanitizer Coverage runtime flags.
 //
 //===----------------------------------------------------------------------===//
 
 #include "sancov_flags.h"
 #include "sanitizer_flag_parser.h"
 #include "sanitizer_platform.h"
 
-#if !SANITIZER_LINUX
-// other platforms do not have weak symbols out of the box.
-extern "C" const char* __sancov_default_options() { return ""; }
-#endif
+SANITIZER_INTERFACE_WEAK_DEF(const char*, __sancov_default_options, void) {
+  return "";
+}
 
 using namespace __sanitizer;
 
 namespace __sancov {
 
 SancovFlags sancov_flags_dont_use_directly;  // use via flags();
 
 void SancovFlags::SetDefaults() {

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

 
 SANITIZER_INTERFACE_ATTRIBUTE
 int __sanitizer_install_malloc_and_free_hooks(void (*malloc_hook)(const void *,
                                                                   uptr),
                                               void (*free_hook)(const void *)) {
   return InstallMallocFreeHooks(malloc_hook, free_hook);
 }
 
-#if !SANITIZER_GO && !SANITIZER_SUPPORTS_WEAK_HOOKS
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void __sanitizer_print_memory_profile(int top_percent) {
+#if !SANITIZER_GO
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_print_memory_profile,
+                             int top_percent) {
   (void)top_percent;
 }
 #endif
 } // extern "C"

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

 uptr __sanitizer_get_number_of_counters() {
   return coverage_data.GetNumberOf8bitCounters();
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 uptr __sanitizer_update_counter_bitset_and_clear_counters(u8 *bitset) {
   return coverage_data.Update8bitCounterBitsetAndClearCounters(bitset);
 }
+
 // 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
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_cmp, void) {}
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_cmp1, void) {}
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_cmp2, void) {}
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_cmp4, void) {}
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_cmp8, void) {}
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_switch, void) {}
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_div4, void) {}
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_div8, void) {}
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_gep, void) {}
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_pc_indir, void) {}
 } // extern "C"

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

 }  // namespace __sancov
 
 extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE void __sanitizer_dump_coverage(  // NOLINT
     const uptr* pcs, uptr len) {
   return __sancov::SanitizerDumpCoverage(pcs, len);
 }
 
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE void
-__sanitizer_cov_trace_pc_guard(u32* guard) {
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_pc_guard, u32* guard) {
   if (!*guard) return;
   __sancov::pc_guard_controller.TracePcGuard(guard, GET_CALLER_PC() - 1);
 }
 
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE void
-__sanitizer_cov_trace_pc_guard_init(u32* start, u32* end) {
+SANITIZER_INTERFACE_WEAK_DEF(void, __sanitizer_cov_trace_pc_guard_init,
+                             u32* start, u32* end) {
   if (start == end || *start) return;
   __sancov::pc_guard_controller.InitTracePcGuard(start, end);
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE void __sanitizer_dump_trace_pc_guard_coverage() {
   __sancov::pc_guard_controller.Dump();
 }
 }  // extern "C"

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

 
 #ifndef SANITIZER_DEBUG
 # define SANITIZER_DEBUG 0
 #endif
 
 // Only use SANITIZER_*ATTRIBUTE* before the function return type!
 #if SANITIZER_WINDOWS
 # define SANITIZER_INTERFACE_ATTRIBUTE __declspec(dllexport)
-// FIXME find out what we need on Windows, if anything.
 # define SANITIZER_WEAK_ATTRIBUTE
 #elif SANITIZER_GO
 # define SANITIZER_INTERFACE_ATTRIBUTE
 # define SANITIZER_WEAK_ATTRIBUTE
 #else
 # define SANITIZER_INTERFACE_ATTRIBUTE __attribute__((visibility("default")))
 # define SANITIZER_WEAK_ATTRIBUTE  __attribute__((weak))
 #endif
 
-#if (SANITIZER_LINUX || SANITIZER_MAC || SANITIZER_WINDOWS) && !SANITIZER_GO
+//--------------------------- WEAK FUNCTIONS ---------------------------------//
+// When working with weak functions, to simplify the code and make it more
+// portable, when possible define a default implementation using this macro:
+//
+// SANITIZER_INTERFACE_WEAK_DEF(<return_type>, <name>, <parameter list>)
+//
+// For example:
+//   SANITIZER_INTERFACE_WEAK_DEF(bool, compare, int a, int b) { return a > b; }
+//
+#if SANITIZER_WINDOWS
+#include "sanitizer_win_defs.h"
+# define SANITIZER_INTERFACE_WEAK_DEF(ReturnType, Name, ...)                   \
+  WIN_WEAK_EXPORT_DEF(ReturnType, Name, __VA_ARGS__)
+#else
+# define SANITIZER_INTERFACE_WEAK_DEF(ReturnType, Name, ...)                   \
+  extern "C" SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE            \
+  ReturnType Name(__VA_ARGS__)
+#endif
+
+// SANITIZER_SUPPORTS_WEAK_HOOKS means that we support real weak functions that
+// will evaluate to a null pointer when not defined.
+#if (SANITIZER_LINUX || SANITIZER_MAC) && !SANITIZER_GO
 # define SANITIZER_SUPPORTS_WEAK_HOOKS 1
 #else
 # define SANITIZER_SUPPORTS_WEAK_HOOKS 0
 #endif
+// For some weak hooks that will be called very often and we want to avoid the
+// overhead of executing the default implementation when it is not necessary,
+// we can use the flag SANITIZER_SUPPORTS_WEAK_HOOKS to only define the default
+// implementation for platforms that doesn't support weak symbols. For example:
+//
+//   #if !SANITIZER_SUPPORT_WEAK_HOOKS
+//     SANITIZER_INTERFACE_WEAK_DEF(bool, compare_hook, int a, int b) {
+//       return a > b;
+//     }
+//   #endif
+//
+// And then use it as: if (compare_hook) compare_hook(a, b);
+//----------------------------------------------------------------------------//
+
 
 // We can use .preinit_array section on Linux to call sanitizer initialization
 // functions very early in the process startup (unless PIC macro is defined).
 // FIXME: do we have anything like this on Mac?
 #if SANITIZER_LINUX && !SANITIZER_ANDROID && !defined(PIC)
 # define SANITIZER_CAN_USE_PREINIT_ARRAY 1
 #else
 # define SANITIZER_CAN_USE_PREINIT_ARRAY 0

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

 }
 
 static void (*PrintfAndReportCallback)(const char *);
 void SetPrintfAndReportCallback(void (*callback)(const char *)) {
   PrintfAndReportCallback = callback;
 }
 
 // Can be overriden in frontend.
-#if SANITIZER_SUPPORTS_WEAK_HOOKS
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
-void OnPrint(const char *str) {
-  (void)str;
-}
-#elif SANITIZER_GO && defined(TSAN_EXTERNAL_HOOKS)
-void OnPrint(const char *str);
+#if SANITIZER_GO && defined(TSAN_EXTERNAL_HOOKS)
+// Implementation must be defined in frontend.
+extern "C" void OnPrint(const char *str);
 #else
-void OnPrint(const char *str) {
+SANITIZER_INTERFACE_WEAK_DEF(void, OnPrint, const char *str) {
   (void)str;
 }
 #endif
 
 static void CallPrintfAndReportCallback(const char *str) {
   OnPrint(str);
   if (PrintfAndReportCallback)
     PrintfAndReportCallback(str);

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

 int WaitForProcess(pid_t pid) { return -1; }
 
 // FIXME implement on this platform.
 void GetMemoryProfile(fill_profile_f cb, uptr *stats, uptr stats_size) { }
 
 
 }  // namespace __sanitizer
 
-#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) {}
-
-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

 
 // 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.
+// Take into account that this is a pragma directive for the linker, so it will
+// be ignored by the compiler and the function will be marked as UNDEF in the
+// symbol table of the resulting object file. 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".
+// is compiled into the object file "test.obj" including the pragma directive.
 // 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.
+// "fun" appears as UNDEF, so it doesn't resolve the symbol "fun", and will
+// result in a link error (the linker doesn't find the pragma directive).
 // 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)))
 
+#define WIN_EXPORT(ExportedName, Name)                                         \
+  __pragma(comment(linker, "/export:" WIN_SYM_PREFIX STRINGIFY(ExportedName)   \
+                                  "=" WIN_SYM_PREFIX STRINGIFY(Name)))
+
+// We cannot define weak functions on Windows, but we can use WIN_WEAK_ALIAS()
+// which defines an alias to a default implementation, and only works when
+// linking statically.
+// So, to define a weak function "fun", we define a default implementation with
+// a different name "fun__def" and we create a "weak alias" fun = fun__def.
+// Then, users can override it just defining "fun".
+// We impose "extern "C"" because otherwise WIN_WEAK_ALIAS() will fail because
+// of name mangling.
+
+// Dummy name for default implementation of weak function.
+# define WEAK_DEFAULT_NAME(Name) Name##__def
+// Name for exported implementation of weak function.
+# define WEAK_EXPORT_NAME(Name) Name##__dll
+
+// Use this macro when you need to define and export a weak function from a
+// library. For example:
+//   WIN_WEAK_EXPORT_DEF(bool, compare, int a, int b) { return a > b; }
+# define WIN_WEAK_EXPORT_DEF(ReturnType, Name, ...)                            \
+  WIN_WEAK_ALIAS(Name, WEAK_DEFAULT_NAME(Name))                                \
+  WIN_EXPORT(WEAK_EXPORT_NAME(Name), Name)                                     \
+  extern "C" ReturnType Name(__VA_ARGS__);                                     \
+  extern "C" ReturnType WEAK_DEFAULT_NAME(Name)(__VA_ARGS__)
+
+// Use this macro when you need to import a weak function from a library. It
+// defines a weak alias to the imported function from the dll. For example:
+//   WIN_WEAK_IMPORT_DEF(compare)
+# define WIN_WEAK_IMPORT_DEF(Name)                                             \
+  WIN_WEAK_ALIAS(Name, WEAK_EXPORT_NAME(Name))
+
+// So, for Windows we provide something similar to weak symbols in Linux, with
+// some differences:
+// + A default implementation must always be provided.
+//
+// + When linking statically it works quite similarly. For example:
+//
+//   // libExample.cc
+//   WIN_WEAK_EXPORT_DEF(bool, compare, int a, int b) { return a > b; }
+//
+//   // client.cc
+//   // We can use the default implementation from the library:
+//   compare(1, 2);
+//   // Or we can override it:
+//   extern "C" bool compare (int a, int b) { return a >= b; }
+//
+//  And it will work fine. If we don't override the function, we need to ensure
+//  that the linker includes the object file with the default implementation.
+//  We can do so with the linker option "-wholearchive:".
+//
+// + When linking dynamically with a library (dll), weak functions are exported
+//  with "__dll" suffix. Clients can use the macro WIN_WEAK_IMPORT_DEF(fun)
+//  which defines a "weak alias" fun = fun__dll.
+//
+//   // libExample.cc
+//   WIN_WEAK_EXPORT_DEF(bool, compare, int a, int b) { return a > b; }
+//
+//   // client.cc
+//   WIN_WEAK_IMPORT_DEF(compare)
+//   // We can use the default implementation from the library:
+//   compare(1, 2);
+//   // Or we can override it:
+//   extern "C" bool compare (int a, int b) { return a >= b; }
+//
+//  But if we override the function, the dlls don't have access to it (which
+//  is different in linux). If that is desired, the strong definition must be
+//  exported and interception can be used from the rest of the dlls.
+//
+//   // libExample.cc
+//   WIN_WEAK_EXPORT_DEF(bool, compare, int a, int b) { return a > b; }
+//   // When initialized, check if the main executable defined "compare".
+//   int libExample_init() {
+//     uptr fnptr = __interception::InternalGetProcAddress(
+//         (void *)GetModuleHandleA(0), "compare");
+//     if (fnptr && !__interception::OverrideFunction((uptr)compare, fnptr, 0))
+//       abort();
+//     return 0;
+//   }
+//
+//   // client.cc
+//   WIN_WEAK_IMPORT_DEF(compare)
+//   // We override and export compare:
+//   extern "C" __declspec(dllexport) bool compare (int a, int b) {
+//     return a >= b;
+//   }
+//
 #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;
void InitializeFlags() {
   InitializeCommonFlags();
   if (Verbosity()) ReportUnrecognizedFlags();
 
   if (common_flags()->help) parser.PrintFlagDescriptions();
 }
 
 }  // namespace __ubsan
 
-extern "C" {
-
-#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 ""; }
-
-WIN_WEAK_ALIAS(__ubsan_default_options, __ubsan_default_default_options)
-#endif
-
-}  // extern "C"
+SANITIZER_INTERFACE_WEAK_DEF(const char *, __ubsan_default_options, void) {
+  return "";
+}
 
 #endif  // CAN_SANITIZE_UB

compiler-rt/trunk/test/asan/TestCases/Windows/dll_host.cc

 // RUN: cat %t.dll_imports1 %t.dll_imports2 %t.dll_imports3 | sort | uniq > %t.dll_imports-sorted
 // RUN: cat %t.exported_wrappers1 %t.exported_wrappers2 %t.exported_wrappers3 | sort | uniq > %t.exported_wrappers-sorted
 //
 // Now make sure the DLL thunk imports everything:
 // RUN: echo
 // RUN: echo "=== NOTE === If you see a mismatch below, please update asan_win_dll_thunk.cc"
 // RUN: diff %t.dll_imports-sorted %t.exported_wrappers-sorted
 // REQUIRES: asan-static-runtime
+// XFAIL: win
 
 #include <stdio.h>
 #include <windows.h>
 
 int main(int argc, char **argv) {
   if (argc != 2) {
     printf("Usage: %s [client].dll\n", argv[0]);
     return 101;