[PATCH] D109192: Support: introduce public API annotation support

Fri Jun 23 05:31:34 PDT 2023

compnerd added a comment.

In D109192#4443741 <https://reviews.llvm.org/D109192#4443741>, @nikic wrote:

> In D109192#4407274 <https://reviews.llvm.org/D109192#4407274>, @tstellar wrote:
>
>> After talking with @compnerd on Discord, I'm planning on helping to push this
>> patch through.  At this point my goal is just to annotate the ABI functions and
>> not make any changes to how -DBUILD_SHARED_LIBS=ON or -DLLVM_LINK_LLVM_DYLIB=ON
>> behave.  We can make changes to those configurations in a future patch (if we
>> want to).
>>
>> My plan is to:
>>
>> 1. Annotate all the ABI functions in the LLVM support library with LLVM_SUPPORT_ABI macro (this patch), and use this an example for how to do the rest of the annotations.
>> 2. Annotate the rest of the LLVM ABI functions using a different macro (e.g. LLVM_ABI)
>
> Why do we need/want to annotate the support library with a different macro from everything else? Isn't LLVMSupport part of libLLVM just like the rest?

LLVMSupport is used very commonly as a support library outside of LLVM.

> We already have the LLVM_EXTERNAL_VISIBILITY and LLVM_LIBRARY_VISIBILITY macros. LLVM_EXTERNAL_VISIBILITY is used in the places that already use the `-fvisibility=hidden` default, namely the backend. LLVM_LIBRARY_VISIBILTY is used in a few places that want to explicitly exclude symbols in code that still uses default visibilty.
>
> I would have expected that this would annotate symbols using the existing LLVM_EXTERNAL_VISIBILITY macro (and LLVM_LIBRARY_VISIBILITY could be retired once everything uses `-fvisibility=hidden`).

Visibility and DLL Storage are technically different and provide subtly different things.  `LLVM_EXTERNAL_VISIBILITY` does not take the library into account nor does it take into account the static vs dynamic linking nature and thus would not be sufficient for the purposes of DLL Storage.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109192