[llvm-c] Guard include of llvm-config in Visibility.h

[cyndyishida]

Visibility.h defines common macro for being explicit about symbol visibility, instead of relying on defaults that may not be the same on a given platform/toolchain set, and is installed with llvm-c API headers.

However, it checks against configuration macros defined by a separate header not installed with llvm-c, to support distribution without this dependency, add an include guard.

[compnerd]

Please extract the __has_include check separately. I don't think that this should be guarded by #if defined(__has_include).

What about the alternative: we install llvm-config.h? I think that makes sense as we don't know what the configuration of the library is (is it shared or static?).

[cyndyishida]

Please extract the __has_include check separately. I don't think that this should be guarded by #if defined(__has_include).

Sorry, I'm not sure I understand. Are you suggesting using has_include directly without checking if the operator is supported? If so, why?

What about the alternative: we install llvm-config.h? I think that makes sense as we don't know what the configuration of the library is (is it shared or static?).

I'd like to avoid pulling in more dependencies if possible. We don't actually need Visibility.h either, since it's not currently necessary or advantageous for us to have these annotated in the preexisting APIs with LLVM_C_ABI for our platform. But since it's already bundled with llvm-c, it's not too much of a hassle.

This is specifically regarding for how the llvm-c headers get installed and then used by specific clients, not to build LLVM/compilers themselves.

[compnerd]

Please extract the __has_include check separately. I don't think that this should be guarded by #if defined(__has_include).

Sorry, I'm not sure I understand. Are you suggesting using has_include directly without checking if the operator is supported? If so, why?

I'm suggesting that we do something like:

#if !defined(__has_include)
#define __has_include(include) 0
#endif

But, we should also guard it with __has_include(...) || 1.

What about the alternative: we install llvm-config.h? I think that makes sense as we don't know what the configuration of the library is (is it shared or static?).

I'd like to avoid pulling in more dependencies if possible. We don't actually need Visibility.h either, since it's not currently necessary or advantageous for us to have these annotated in the preexisting APIs with LLVM_C_ABI for our platform. But since it's already bundled with llvm-c, it's not too much of a hassle.

This is specifically regarding for how the llvm-c headers get installed and then used by specific clients, not to build LLVM/compilers themselves.

I don't think it is pulling in more dependencies. The header is a single standalone header that gives very specific information that the user must provide or the build is invalid (aka, we can invoke UB). You need to indicate if you are building against a static version of the library or not because that impacts code generation on some platforms. If we don't simply add the single CMake generated header, we should provide default values for the provided macros in the configuration.

[cachemeifyoucan]

I don't think it is correct to have llvm-c header to include llvm header, even though you can argue that config can be an exception.

Maybe the better fix is just always define LLVM_ABI in c header? I don't know if anyone building C API will argue against exporting them by default.

[compnerd]

I don't think it is correct to have llvm-c header to include llvm header, even though you can argue that config can be an exception.

I had suggested that we avoid that dependency, but it was difficult to split it apart.

Maybe the better fix is just always define LLVM_ABI in c header? I don't know if anyone building C API will argue against exporting them by default.

I don't think that you can simply define LLVM_ABI per se. There is no guarantee that the number of macros will not expand in the future to multiple macros. Each macro depends on the configuration of each library. e.g. lets say that LLVM-C depends on LLVMSupport, LLVM, and LLVMBackend.

That would require 3 macros: LLVMSupport_ABI, LLVM_ABI, LLVMBackend_ABI, which would depend on 3 different macros: LLVMSupport_STATIC, LLVM_STATIC, and LLVMBackend_STATIC. So, the definition that you would need be something to the effect of:

#if defined(LLVMSupport_STATIC)
# define LLVMSupport_ABI /**/
#else
# if defined(_WIN32)
#   if defined(LLVMSupport_EXPORTS)
#     define LLVMSupport_ABI __declspec(dllexport)
#   else
#     define LLVMSupport_ABI __declspec(dllimport)
#   endif
# else
#   if defined(__linux__)
#     if defined(LLVMSupport_EXPORTS)
#       define LLVMSupport_ABI __attribute__((__visibility__("protected")))
#     else
#       define LLVMSupport_ABI __attribute__((__visibility__("default")))
#     endif
#   else
#     define LLVMSupport_ABI __attribute__((__visibility__("default")))
#   endif
#endif

#if defined(LLVM_STATIC)
# define LLVM_ABI /**/
#else
# if defined(_WIN32)
#   if defined(LLVM_EXPORTS)
#     define LLVM_ABI __declspec(dllexport)
#   else
#     define LLVM_ABI __declspec(dllimport)
#   endif
# else
#   if defined(__linux__)
#     if defined(LLVM_EXPORTS)
#       define LLVM_ABI __attribute__((__visibility__("protected")))
#     else
#       define LLVM_ABI __attribute__((__visibility__("default")))
#     endif
#   else
#     define LLVM_ABI __attribute__((__visibility__("default")))
#   endif
#endif

#if defined(LLVMBackend_STATIC)
# define LLVMBackend_ABI /**/
#else
# if defined(_WIN32)
#   if defined(LLVMBackend_EXPORTS)
#     define LLVMBackend_ABI __declspec(dllexport)
#   else
#     define LLVMBackend_ABI __declspec(dllimport)
#   endif
# else
#   if defined(__linux__)
#     if defined(LLVMBackend_EXPORTS)
#       define LLVMBackend_ABI __attribute__((__visibility__("protected")))
#     else
#       define LLVMBackend_ABI __attribute__((__visibility__("default")))
#     endif
#   else
#     define LLVMBackend_ABI __attribute__((__visibility__("default")))
#   endif
#endif

This requires that LLVMSupport_STATIC, LLVM_STATIC, and LLVMBackend_STATIC are mapped to exactly how the dependency was configured and built. Additionally, you need to ensure that any other macros are also identically configured. This is why the CMake generated header being exposed is the right thing to do.

[cachemeifyoucan]

This requires that LLVMSupport_STATIC, LLVM_STATIC, and LLVMBackend_STATIC are mapped to exactly how the dependency was configured and built. Additionally, you need to ensure that any other macros are also identically configured. This is why the CMake generated header being exposed is the right thing to do

I am not sure why exporting C interface requires exporting c++ interface on Windows...

I guess the other alternative is that we can preprocess the headers when installing them so the installed C headers don't need to have dependency on config?

[compnerd]

This requires that LLVMSupport_STATIC, LLVM_STATIC, and LLVMBackend_STATIC are mapped to exactly how the dependency was configured and built. Additionally, you need to ensure that any other macros are also identically configured. This is why the CMake generated header being exposed is the right thing to do

I am not sure why exporting C interface requires exporting c++ interface on Windows...

Oh, that is easy to answer: because it does! There is no true C API to LLVM. The C API is bit just a shim over the C++ interfaces. The configuration here is required to actually import the C++ interface. We could completely sink the implementation into a .cpp file, and make the header completely opaque, but I think that would be a larger change (but one that I would be in favour of).

I guess the other alternative is that we can preprocess the headers when installing them so the installed C headers don't need to have dependency on config?

Hmm, do you have a portable mechanism for doing this? Remember that we should only rely on tools that are available on Windows natively rather than grow a new dependency.

[cachemeifyoucan]

Hmm, do you have a portable mechanism for doing this? Remember that we should only rely on tools that are available on Windows natively rather than grow a new dependency.

I was thinking just using clang. Or maybe we can have a llvm-c/llvm-config.h file generated by CMake?

[compnerd]

The problem is that the configuration is not LLVMC, but LLVM itself. You need the configuration for the target libraries that it is creating bindings for.

[cachemeifyoucan]

The problem is that the configuration is not LLVMC, but LLVM itself. You need the configuration for the target libraries that it is creating bindings for.

I think you are trying to solve a bigger problem than the one trying to fix here. For this PR, just want to achieve the same visibility granularity as main while try to break llvm-c out of the dependency of the big config file for c++ header.

I don't know how people usually use llvm-c headers. I don't assume people will use c++ and c header together. I might consider split out all the functions that implements C interface into a separate lib libLLVMC and make that link other c++ libraries.

[compnerd]

The problem is that the configuration is not LLVMC, but LLVM itself. You need the configuration for the target libraries that it is creating bindings for.

I think you are trying to solve a bigger problem than the one trying to fix here. For this PR, just want to achieve the same visibility granularity as main while try to break llvm-c out of the dependency of the big config file for c++ header.

I think that you are misunderstanding the code. The C++ header dependency is a requirement because the C interfaces call into the C++ interfaces.

I don't know how people usually use llvm-c headers. I don't assume people will use c++ and c header together. I might consider split out all the functions that implements C interface into a separate lib libLLVMC and make that link other c++ libraries.

The llvm-c headers are used to actually use the C++ API from C. The C bindings are the only officially supported stable ABI. Separating the inline definitions this way would allow you to also remove the llvm-config.h header because the C++ APIs would no longer be inlined, but would come at a performance cost because you could no longer inline without LTO.

[cachemeifyoucan]

I think that you are misunderstanding the code. The C++ header dependency is a requirement because the C interfaces call into the C++ interfaces.

I am not misunderstanding the code, just don't quite sure what are all the build configurations LLVM_ABI trying to support. Visibility only matters across binary boundaries. If C APIs is in the same .dll as the C++ APIs, it can be whatever visibility it wants because c++ APIs can be inlined. The dependencies on C++ LLVM_ABI annotation only matters when C APIs lives in a different .dll from the c++ APIs like I proposed. Is my understanding correct?

[cyndyishida]

For what it's worth, I am hoping for a solution where we (as in maintainers for a platform's toolchain) can continue to ship a limited set of llvm-c APIs based on a prebuilt non-configurable libLLVM. Adding header dependencies also adds module dependencies that I was also hoping to avoid complexity around.
I was hoping that this patch would offer a reasonable middle ground where it doesn't impact preexisting workflows but still resolves our issue.

If we don't simply add the single CMake generated header, we should provide default values for the provided macros in the configuration.

Having LLVM_ABI resolve to __attribute__((__visibility__("default"))) or nothing for our fallback also seems reasonable to me. But, of course, I am concerned about reimplementing complex configuration logic to be able to redefine the same macros llvm-config.hdoes, for our use case.

I'm suggesting that we do something like:

#if !defined(__has_include)
#define __has_include(include) 0
#endif

But, we should also guard it with __has_include(...) || 1.

Ah I see, thanks for explaining.

[compnerd]

For what it's worth, I am hoping for a solution where we (as in maintainers for a platform's toolchain) can continue to ship a limited set of llvm-c APIs based on a prebuilt non-configurable libLLVM. Adding header dependencies also adds module dependencies that I was also hoping to avoid complexity around.

I agree with this goal. I think that the easiest way to achieve this goal is to simply ship the one extra header. This particular header is not modular, it is a textual header (though could be a standalone module I suppose),

I was hoping that this patch would offer a reasonable middle ground where it doesn't impact preexisting workflows but still resolves our issue.

The interspersed implementation really complicates this. It would be ideal to not require the header (I had the same concerns when this change went in).

[cyndyishida]

I think that the easiest way to achieve this goal is to simply ship the one extra header. This particular header is not modular, it is a textual header (though could be a standalone module I suppose),

It already is modular, https://github.com/llvm/llvm-project/blob/main/llvm/include/module.modulemap.build
Do you have a suggestion on how to isolate only the module declaration for LLVM_Config_Config in this tree, so we wouldn't need to at installation time downstream? Part of what's appealing to me about a has_include or basic LLVM_ABI fallback is that it reduces the amount of internal changes required.

[compnerd]

Do you have a suggestion on how to isolate only the module declaration for LLVM_Config_Config in this tree, so we wouldn't need to at installation time downstream? Part of what's appealing to me about a has_include or basic LLVM_ABI fallback is that it reduces the amount of internal changes required.

Are you asking how to make it easy to consume standalone downstream or how to avoid the header downstream entirely?

The module has no dependencies, the content of the file is simply the configuration that was passed to cmake (i.e., it is just a series of #define and comments). So, this should have little to no challenge for downstream consumers.

If the question is to how to avoid the dependency on llvm/Config/llvm-config.h entirely - I think that would require that we change llvm-c/Visibility.h to a preprocessed header that is written out by CMake. It ends up being the same situation as the current thing in that we would need to install a generated header.