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

Fri Sep 3 12:43:57 PDT 2021

rnk added a comment.

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.

> 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?

----

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`?

================
Comment at: llvm/lib/Support/AArch64TargetParser.cpp:27

-unsigned AArch64::getDefaultFPU(StringRef CPU, AArch64::ArchKind AK) {
+unsigned LLVMSupport_API AArch64::getDefaultFPU(StringRef CPU,
+                                                AArch64::ArchKind AK) {
----------------
Is this necessary? In general, you shouldn't have to annotate the definitions so long as the previous declaration was annotated. I don't see any compilers that warn about this:
https://gcc.godbolt.org/z/E7T1c9z81

================
Comment at: llvm/lib/Support/CMakeLists.txt:104

-add_llvm_component_library(LLVMSupport
+add_llvm_component_library(LLVMSupport SHARED
   AArch64TargetParser.cpp
----------------
As I understand it, this makes the support library always shared. I don't think that gives people using the traditional static build a pathway to transition. What I had in mind was making this conditional on LLVM_BUILD_DYLIB or whatever we're calling it, the one that builds libLLVM.so, the one all the Linux distros use.

I think our long term goal should be to align the default LLVM cmake build configuration with the one that distributors are using. The fact that most devs don't use the most commonly distributed LLVM build configuration leads to surprises like the ones from earlier this year:
https://maskray.me/blog/2021-05-16-elf-interposition-and-bsymbolic
https://lore.kernel.org/lkml/CAHk-=whs8QZf3YnifdLv57+FhBi5_WeNTG1B-suOES=RcUSmQg@mail.gmail.com/

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109192