[PATCH] D109192: [WIP/DNM] Support: introduce public API annotation support

Sat Sep 4 13:21:27 PDT 2021

MaskRay added a comment.

Thanks for working on this! I agree that `LLVM_*_ABI` is a better name because it is related to what interfaces other libraries can use.

In D109192#2983041 <https://reviews.llvm.org/D109192#2983041>, @compnerd wrote:

> In D109192#2982999 <https://reviews.llvm.org/D109192#2982999>, @rnk wrote:
>
>> In D109192#2982723 <https://reviews.llvm.org/D109192#2982723>, @compnerd wrote:
>>
>>> Some additional questions:
>>>
>>> - should we be encouraging of exporting entire classes or should we be more judicious in the visibility (e.g. attribute members)?  I'm not 100% certain that this works for non-dll-interface targets.
>>
>> I think you can give entire classes default visibility, and that will apply to all members. I think we should annotate classes because it will reduce the total number of annotations, and that will reduce the churn. For large, key classes that have too many private methods (Sema, ASTContext?) we can go back later and export only the public APIs, or something like that.
>
> This is sort of the direction that I had in mind.  The problem is, there are cases where I cannot get the desired behaviour correct.  Doing those where it doesn't just work seems like an easier path and one that could be refined later.  Its a bit unfortunate, but seems like a decent trade off.
>
>>> I'm more convinced now that we should introduce an option to introduce this behaviour.  The new option can opt into building the shared library components.  Perhaps something like `LLVM_EXPERIMENTAL_SHARED_LIBRARY_COMPONENTS` is a good option name?
>>
>> I agree this new thing shouldn't become the default build config. How close is this to BUILD_SHARED_LIBS vs LLVM_ENABLE_DYLIB? Can we repurpose one of those?
>
> I think that `BUILD_SHARED_LIBS` is desirable, and `LLVM_ENABLE_DYLIB` should go away.  That said, I think that the ideal thing here is to have a different configuration that is less granular than `BUILD_SHARED_LIBS`.  That would correspond to `LLVMTools`, `LLVMCompiler` and depending on the delta, a potential third library.  These three would give us a nice hybrid approach to dynamic and static linking.  It would be dynamic for buckets of use (bintuils, clang, and possibly lld if it makes `LLVMTools` too large) but statically linking large components of LLVM to enable us to take advantage of LTO and DCE amongst that set.  It would try to minimise the cost of the GOT/PLT and IAT so that we don't introduce an extremely heavy penalty for these libraries.

Why should `LLVM_ENABLE_DYLIB` go away? To reduce the number of configurations when the few-dylibs scheme is adopted more?
Other than that, I don't see any downside.

>> ----
>>
>> Going in a completely different direction, why even start by splitting out Support as its own shared library? We already have the single-DSO build config, why not improve that by adding annotations and using `-fvisibility=hidden`?
>
> I don't really care if we keep LLVMSupport as a separate library or not in the end.  The point here is to work through a representative set to setup such a configuration.  This gives us a good pattern to apply and tries to iron out the issues that this brings up in a smaller context.  The `-fvisbility=hidden` (and `-fvisibility-inlines-hidden`) is a requirement for this so we do not continually regress.  One of the hacks in this patch is the unconditional application of that to LLVMSupport (which I will remove as this patch gets closer to a final form).  I don't think that the single DSO configuration is ideal either, we would still have an unnecessarily large overhead for tooling.  I think that the ideal split would be a library sufficient for the binutils, a second library that has everything else that is needed for clang.

================
Comment at: llvm/include/llvm/Support/LLVMSupportExports.h:12
+
+#if defined(__ELF__)
+# if defined(LLVM_SUPPORT_STATIC)
----------------
For flexibility, maybe give user a choice to redefine `LLVM_*_ABI`. Some users may want to customize this to hidden to enable better internalization (LTO,DCE) and linker GC.

================
Comment at: llvm/lib/Support/CMakeLists.txt:289
+  CXX_VISIBILITY_PRESET hidden
+  VISIBILITY_INLINES_HIDDEN YES)

----------------
-fvisibility=hidden implies -fvisibility-inlines-hidden

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109192