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

Tue Aug 9 13:39:35 PDT 2022

mstorsjo added a comment.

In D130121#3710488 <https://reviews.llvm.org/D130121#3710488>, @rnk wrote:

> I think this is reasonable, but it's a big change worth discussing more widely. The basic idea is to try to make hidden visibility work on GNU COFF targets.

Thanks for picking up the thread here - this is exactly the discussion I'd like to have. Yes, it's a bigger change than it seems originally.

FWIW, the scope of my effort is that I've added support to binutils and lld for handling the directive, and with this patch, LLVM would produce such directives for hidden visibility. Currently, setting hidden visibility with Clang/LLVM produces no warnings but doesn't have any effect. With GCC, using `__attribute__((visibility("hidden")))` currently gives warnings about not having any effect, but passing e.g. `-fvisibility=hidden` doesn't - so projects may be passing that accidentally somewhere (like we did, before D130200 <https://reviews.llvm.org/D130200>). I'm not planning on making patches myself to GCC to map the visibility to this directive there though.

> One major reservation that I have about this solution is that we've discovered that the .drective section is extremely inefficient.

Yup. What led me to this solution was that it turned out to be extremely straightforward to implement (which shouldn't be an argument for defining new features, but alas...) as it's modelled exactly 1:1 after the regular dllexport mechanism. In lld, I used the same early handling of the directive just like what has been done for exports for performance reasons.

> Consider that, with heavy use of templates, most C++ symbols are inline. Roughly speaking, O(all) strings in the symbol table will be duplicated into the .drectve section. C++ symbols, can in many cases, dominate symbol file size.

On one hand, one could argue that with dllexport, you can also have such large .drectve sections with all the exported symbols. However in practice, I guess the norm would be that a fraction of the symbols are exported and the majority aren't. And in the default/hidden visibility scenario, we have the inverse - in the typical case, most symbols would be hidden (ideally at least). So I agree that the issue is on a different scale here, and this could be worse than current day dllexport.

I can try to do a test build of LLVM with and without this feature (with a separate patch that takes the hidden visibility into use) and see how much it inflates the build directory. What would be the best measuring point for that? Maybe the size of the largest object file and/or largest static library?

> This was a big problem for us in PGO builds, which use llvm.used, which uses the /include: directive.

> I think I filed a feature request to implement a more efficient scheme similar to the address-significance tables. This is a **much** more efficient way to tag symbols with some boolean property,

Where did you file this request? It'd be interesting to track how it fares.

> and you can see it was discussed a few times recently with regard to the ELF address significance tables because it interacts with strip and objcopy in bad ways. However, I've never really seen those tools as part of the mainstream for COFF targets, so we kept the address significance protocol as it is.

Strip and objcopy are used a lot for GNU COFF though, mainly to get rid of symbol tables and embedded dwarf debug info, neither which are a thing in the MSVC ecosystem.

> I have never coordinated with binutils folks, and I think compatibility probably outweighs efficiency here, but if they are open to improvements, it's worth looking into this before we lock in this inefficient format.

FWIW on the binutils side, the support for the new directive (equivalent to D130120 <https://reviews.llvm.org/D130120>) was merged recently, but it was just past the 2.39 branch, so I think we've got another 5 months or so to settle things finally. I'm sorry I went ahead and merged that patch there before we've had this discussion here (I wasn't sure if we'd get any good discussion though). On the other hand, I guess nothing says we must remove support for the directive if we settle on a more efficient mechanism either, but it'd feel a bit unnecessary to have a brand new directive that won't be used for what it was meant for.

In practice, there doesn't seem to be much activity from others to coordinate with there either (there wasn't any discussion like this one) - I guess it's all up to writing an acceptable patch for whatever format can be argued to be the best way forward.

Initially, I had hoped to get this landed by the 15.x branch, but it's clearly for the best to delay it to get things sorted out properly. I tried to do a mingw dylib build including mlir+flang, and that requires this feature to get libMLIR.dll under the export limit.

For the mingw dylib builds, we're awfully close to the export limit for libLLVM.dll right now for the 3 architectures I support in llvm-mingw - while msys2 have had to successively disable more and more targets the last few releases to fit under that limit. So I think msys2 might want to cherrypick this already to their 15.x release in order not to need to cut down the list of targets entirely - if we've settled on a way forward.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130121