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

[Marcos Pividori via Phabricator via llvm-commits]

mpividori added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_internal_defs.h:42
+# define WEAK(ReturnType, Name, Params)                                        \
+  WIN_WEAK_ALIAS(Name, WEAK_DEF_NAME(Name))                                    \
+  extern "C" ReturnType Name Params;                                           \
----------------
rnk wrote:
> 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.
@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?


Repository:
  rL LLVM

https://reviews.llvm.org/D28596