Skip to content

RFC: [llvm] refactor common export macros to llvm/Config #149175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

andrurogerz
Copy link
Contributor

Purpose

Eliminate code duplication in LLVM_ABI and related export macros by defining a foundational set of LLVM_INTERFACE_ macros in a new config header llvm/Config/export-config.h.

Overview

  • Introduce a new export-config.h.cmake config file which contains new definitions of LLVM_INTERFACE_ macros. These macros are always defined to the appropriate export/visibility annotation.
  • Move definitions controlling individual library export annotations from llvm-config.h.cmake to the new export-config.h.cmake file.
  • Update LLVM_ABI macro definitions in llvm/Support/Compiler.h to alias to the new LLVM_INTERFACE_ macros, conditional on LLVM_ENABLE_LLVM_EXPORT_ANNOTATIONS being defined.
  • Update 'LLVM_C_ABImacro definition inllvm-c/Visibility.hto alias toLLVM_INTERFACE_ABI, conditional on LLVM_ENABLE_LLVM_C_EXPORT_ANNOTATIONS` being defined.

Background

The effort to build LLVM as a WIndows DLL is tracked in #109483. Additional context is provided in this discourse, and documentation for LLVM_ABI and related annotations is found in the LLVM repo here.

Validation

  • Built llvm-project on Windows with clang-cl and MSVC cl.
  • Built llvm-project on Linux with clang and gcc.

@andrurogerz andrurogerz changed the title [llvm] refactor common export macros to llvm/Config RFC: [llvm] refactor common export macros to llvm/Config Jul 16, 2025
@andrurogerz
Copy link
Contributor Author

@compnerd this is my proposed solution to reducing preprocessor logic duplication between export macro definitions in different libraries throughout LLVM. This pattern lets us define export macros for abi-breaking.h.config like in #145575 (which was reverted) but without taking a dependency on llvm/Support/Compiler.h or duplicating a bunch of preprocessor logic.

With this patch in place, we'd be able to just use a pattern like this for any library that needs its own macros:

#if defined(LLVM_ENABLE_EXAMPLE_EXPORT_ANNOTATIONS)
#define EXAMPLE_ABI LLVM_INTERFACE_ABI
#else
#define EXAMPLE_ABI
#endif

We can even redefine the CLANG_ABI annotations this way (though I left them as-is for now).

My primary concern with this approach is that it hides the real definitions in a cmake config file making them a little harder for developers to find and understand. Do you think this is this a reasonable approach? Any other ideas/suggestions?

// two preprocessor definitions to gate LLVM_ABI macro definitions.
#if !defined(LLVM_BUILD_STATIC)
#if defined(_WIN32) && !defined(__MINGW32__)
#if defined(LLVM_EXPORTS)
Copy link
Member

@compnerd compnerd Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work. This defines the interface as to how LLVM was built. If I build LLVM dynamically, llvm-c statically, and LLVM Demangle dynamically, I need 2 independent sets of definitions for LLVM and LLVM Demangle. Doing something else really doesn't work. The de-duplication is not worth additional complexity and introducing new patterns IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The de-duplication is not worth additional complexity and introducing new patterns IMO.

I am completely on-board with the complexity argument for not doing it this way. I will put up the abi-breaking PR again with the duplicated logic.

@andrurogerz andrurogerz deleted the llvm-abi-move-defs branch July 23, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants