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

Sun Sep 5 14:21:02 PDT 2021

compnerd added a comment.

In D109192#2984321 <https://reviews.llvm.org/D109192#2984321>, @MaskRay wrote:

> If I am to summarize the state. The currently ELF build models are:
>
> - default: `libLLVMSupport.a`
> - `-DBUILD_SHARED_LIBS=on`: `libLLVMSupport.so`
> - `-DLLVM_LINK_LLVM_DYLIB=on`: `libLLVM-14git.so`
>
> Windows dynamic linking doesn't work.
>
> From
>
>> - minimal set of interfaces required for llvm-ar (archiver/librarian)
>> - minimal set of interfaces required for lld (linker)
>> - minimal set of interfaces required for clang (compiler)
>
> it seems that the intention is to have `libLLVM{foo,bar,qux}.so` as the end state? And that is a controversial point among at least @compnerd, @jrtc27, and me.
>
> I think the main purpose of this patch is to introduce these source file annotations (`LLVM_SUPPORT_ABI` `#include "llvm/Support/LLVMSupportExports.h"`) which will be the backbone of whatever dynamic linking mode we end up doing.
> And we should ensure all parties are satisfied with the solution.
> If you remove the behavior changing CMake changes, I will give a strong support.

More to introduce the model for the annotations.  The annotations are module dependent, that is each module must introduce its own decoration and the function must have a clear owner defined, which is why LLVMSupport is a good basis for the conceptual change - the intent is that the functions in LLVMSupport are owned by LLVMSupport and thus will be annotated with the `LLVM_SUPPORT_ABI`.

> Specifically, any risky CMake or visibility annotation should be dropped from this starting point.

I think that this we disagree on.  I think that the visibility handling needs to be part of this - otherwise the behaviour becomes too complicated to reason about.  The default beahviour on Windows is that the symbol is `dso_local` and the default behaviour on ELF and MachO targets is *not* `dso_local`.  This means that we have different defaults and any future change will need to contend with this.  By changing the visibility for LLVMSupport when building LLVMSupport as a shared library to hidden visibility, we should get consistent behaviour across the board and make it easier to reason through for changes.

> E.g.
>
> - `#define LLVM_SUPPORT_ABI` is acceptable since it is clearly a no-op.
> - `#define LLVM_SUPPORT_ABI __attribute__((visibility("protected")))` can be risky and is thus unacceptable
> - Changing `-DBUILD_SHARED_LIBS=on` behavior is unacceptable (this presumably doesn't work on Windows, so changing Windows is fine)
> - Changing `-DLLVM_LINK_LLVM_DYLIB=on` behavior is unacceptable (this doesn't work on Windows, so changing Windows is fine)
>
> ---
>
> Currently there is a hacking change in `lib/Support/CMakeLists.txt`: `add_llvm_component_library(LLVMSupport SHARED`
> This breaks the regular `libLLVMSupport.a` build, so it should be fixed.

Yes, and I even commented as such that it is unacceptable, but is done to allow for quick turnaround on testing.  Once the pre-commit checks have verified that it is happy with the build, this will be changed to restore the build of LLVMSupport to the way that it is handled today.

> The `libLLVM{foo,bar,qux}.so` design is related to the Windows DLL limitation about 64k exported symbols.
> To reduce platform differences (ELF/Mach-O vs PE/COFF), @compnerd wants the `libLLVM{foo,bar,qux}.so` design to be the way forward and the monolithic `libLLVM-14git.so` scheme to be dropped.
> I agree with @lebedev.ri that such change needs to discussed on llvm-dev.
> The `libLLVM-14git.so` scheme is adopted by multiple major Linux distributions with many LLVM dependent packages.
> Such a split should not be taken lightly.

Yes I do expect that we are going to hit the limit of the exported symbols and thus its very likely to require that split.  However, I cannot say with certainty that will happen until we get to that point.  Once LLVMSupport changes are baked enough to demonstrate the pattern, the changes to allow building LLVM as a library can be tested to see what the current limitation is.  The more fragmented the build, the worse it is for optimizations and size reduction possibilities.  It also makes distribution more complicated.

> FWIW For my https://github.com/MaskRay/ccls/blob/master/CMakeLists.txt#L95 , I'll need to adjust
>
>   if(LLVM_LINK_LLVM_DYLIB)
>     target_link_libraries(ccls PRIVATE LLVM)
>
> Other packages may need similar adaptation as well.
> Personally I consider that ELF/Mach-O changing `libLLVM-14git.so` to cater to Windows DLL limitation unfortunate, but I can be convinced if the split argument (platform difference, maintenance cost consideration) is sufficiently strong.

I don't think that it is "catering" nor "unfortunate".  There are tradeoffs in software engineering, and this is one of them.  There are similar issues where LLVM is doing things that are non-optimal and other platforms suffer from them.  An example is the use of the `anchor` key functions which are meaningless on some platforms.

The current state of the shared library build is another example of the "unfortunate" results of supporting ELF targets (I say this as a counterpoint to your argument that it is unfortunate that ELF must do something different, I tend to care about ELF targets just as much as PE/COFF targets).

Platforms are different, and software engineering requires that you evaluate different tradeoffs.  There is a rich design space for structuring binaries, and choices aren't inherently unfortunate.  They are evaluating with different weights being ascribed to different features, and that results in different choices.

Keeping LLVM friendly to a variety of platforms with contributors developing and testing on a different set of platforms means that it is best to have it behave as consistently as possible.  This reduces the likelihood of someone unintentionally regressing a system other than the one that they are developing on.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109192