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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/include/llvm/Support/LLVMSupportExports.h:12
+
+#if defined(__ELF__)
+# if defined(LLVM_SUPPORT_STATIC)
----------------
rnk wrote:
> arichardson wrote:
> > compnerd wrote:
> > > MaskRay wrote:
> > > > 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.
> > > Wait, we want to give the user the flexibility to *not* define that?  I intend to make `-fvisibility=hidden` the only option once the annotations are complete.  I want to simply globally set:
> > > 
> > > ```
> > > set(CMAKE_C_VISIBILITY_PRESET hidden)
> > > set(CMAKE_CXX_VISIBILITY_PRESET hidden)
> > > set(CMAKE_C_VISIBILITY_INLINES_HIDDEN YES)
> > > set(CMAKE_CXX_VISIBILITY_INLINES_HIDDEN YES)
> > > ```
> > > 
> > > Everything in LLVM should be internalized unless you are doing a shared library build, where the annotations will expose the necessary interfaces.
> > > 
> > > Is there a use case that I am not considering?
> > If we need to add this for every library, it seems like the header should be generated?
> > 
> > Maybe we could even re-use https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html?
> CMake is the official build system for LLVM, so maybe this should not weigh on the project priorities, but there are two unofficial build systems in-tree that would have to replicate this functionality (Bazel & gn).
> 
> I think we should strive to keep the build system as simple as possible, even if it has ways to help us reduce boilerplate code. Most LLVM developers are not CMake experts, but we can all read and reason about C++ and C pre-processor directives.
> 
> I believe there are ways to factor out the selection of the appropriate attribute using token pasting.
We would in order to support `BUILD_SHARED_LIBS` universally.  However, my goal is not to enable `BUILD_SHARED_LIBS` but rather allow `LLVM_BUILD_LLVM_DYLIB` which builds a single shared library.  LLVMSupport is a good target to demonstrate what is needed without having to implement the full coverage for all of LLVM without actually reaching some consensus.  That said, I would certainly be interested in having LLVMSupport as a shared library as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109192