[PATCH] D130121: [3/3] [COFF] Emit embedded -exclude-symbols: directives for hidden visibility for MinGW

Wed Jul 20 12:48:31 PDT 2022

mstorsjo added a comment.

In D130121#3666011 <https://reviews.llvm.org/D130121#3666011>, @mati865 wrote:

> I'm a little bit concerned this could break some projects in the wild but for projects that use it properly this sounds like huge win (as your LLVM results show).

Yes, there's some risk with this.

Any project that uses hidden visibility will break linking with older LLD, as LLD errors out on unhandled embedded directives. As the patches to LLD would be landed before, that's probably fine as long as a user doesn't update Clang without updating LLD. And the binutils patch seems well received so far too.

Secondly, as you mention - the risk is with projects using visibility inconsistently in mingw configurations so far. Ironically, it seems like LLVM is such a case... https://github.com/llvm/llvm-project/blob/bc9b964f8f38bdaa4574847fc59527ea597dbc0a/llvm/lib/Target/CMakeLists.txt#L24-L30 enables hidden visibility by default for all code under `llvm/lib/Target`, with the expectation that `LLVM_EXTERNAL_VISIBILITY` overrides it - but the `LLVM_EXTERNAL_VISIBILITY` define doesn't expand to anything on mingw: https://github.com/llvm/llvm-project/blob/bc9b964f8f38bdaa4574847fc59527ea597dbc0a/llvm/include/llvm/Support/Compiler.h#L116-L127

Other than that, I haven't run into any issues in my testing with these patches, but I don't build all that many projects with tricky DLL setups either.

================
Comment at: llvm/lib/IR/Mangler.cpp:246
+  if (GV->hasHiddenVisibility() && !GV->isDeclaration() &&
+      (TT.isWindowsGNUEnvironment() || TT.isWindowsCygwinEnvironment())) {
+
----------------
mati865 wrote:
> This could be simplified to `TT.isOSCygMing()`.
Ah, indeed, thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130121