[PATCH] D133165: [lld][COFF] Add support for overriding weak symbols in LLVM bitcode input

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

Thanks for your effort on this! This looks mostly good to me. It's a shame that weak symbols from bitcode has to be handled differently than regular weak symbols from object files.

FWIW, I've got a set of testcases for weak symbol interactions in mingw, and a couple of them used to fail with LTO before; with this patch in place, all except one pass.

The one that still fails does this:

  __attribute__((weak)) void func(void);
  void doCall(void) {
      if (func)
          func();
  }

If this is linked without any definition of `func`, it still fails with an undefined reference. (In the regular object file form, there's a weak undefined named `func`, which points at an absolute symbol (with a autogenerated unique name) with the value zero. If there's no other definition of `func`, the symbol resolves as the absolute address zero.)



================
Comment at: lld/COFF/Symbols.h:148
+  // rather, it's used for managing weak symbol overrides.
+  unsigned isWeak : 1;
 protected:
----------------
Would it be good to mention that this only is tracked for bitcode/LTO symbols? For object files, weak symbols are handled in a different way.


================
Comment at: lld/COFF/Symbols.h:160
 public:
-  Defined(Kind k, StringRef n) : Symbol(k, n) {}
+Defined(Kind k, StringRef n) : Symbol(k, n) {}
 
----------------
Nit: This looks like an unintentded indentation change?


================
Comment at: lld/test/COFF/weak-override.ll:7
+; RUN: lld-link /dll /out:%t-strong-first.dll %t.obj %t-strong.obj %t-weak.obj
+; RUN: llvm-objdump -d %t-weak-first.dll | FileCheck %s
+; RUN: llvm-objdump -d %t-strong-first.dll | FileCheck %s
----------------
I think it'd be useful to test that linking without the strong definition also succeeds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133165/new/

https://reviews.llvm.org/D133165