Skip to content

[llvm] annotate LLVMCloneModule for export #145570

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrurogerz
Copy link
Contributor

@andrurogerz andrurogerz commented Jun 24, 2025

Purpose

This patch is one in a series of code-mods that annotate LLVM’s public interface for export. This patch annotates the implementation of LLVMCloneModule with LLVM_ABI to match its declaration in llvm-c/Core.h. The annotation currently has no meaningful impact on the LLVM build; however, it is a prerequisite to support an LLVM Windows DLL (shared library) build.

Background

This effort 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

Local builds and tests to validate cross-platform compatibility. This included llvm, clang, and lldb on the following configurations:

  • Windows with MSVC
  • Windows with Clang
  • Linux with GCC
  • Linux with Clang
  • Darwin with Clang

@andrurogerz andrurogerz marked this pull request as ready for review June 24, 2025 21:15
@llvmbot
Copy link
Member

llvmbot commented Jun 24, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Andrew Rogers (andrurogerz)

Changes

Purpose

This patch is one in a series of code-mods that annotate LLVM’s public interface for export. This patch effectively annotates the implementation of LLVMCloneModule with LLVM_ABI by including llvm-c/Core.h in the implementation file. The included the function declaration is already annotated with LLVM_ABI. The annotation currently has no meaningful impact on the LLVM build; however, it is a prerequisite to support an LLVM Windows DLL (shared library) build.

Background

This effort 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

Local builds and tests to validate cross-platform compatibility. This included llvm, clang, and lldb on the following configurations:

  • Windows with MSVC
  • Windows with Clang
  • Linux with GCC
  • Linux with Clang
  • Darwin with Clang

Full diff: https://github.com/llvm/llvm-project/pull/145570.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/CloneModule.cpp (+1)
diff --git a/llvm/lib/Transforms/Utils/CloneModule.cpp b/llvm/lib/Transforms/Utils/CloneModule.cpp
index 55fb0acd39eae..ae6234c1349d6 100644
--- a/llvm/lib/Transforms/Utils/CloneModule.cpp
+++ b/llvm/lib/Transforms/Utils/CloneModule.cpp
@@ -11,6 +11,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm-c/Core.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Transforms/Utils/Cloning.h"

@andrurogerz
Copy link
Contributor Author

@compnerd, @vgvassilev another simple one, thanks!

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Lgtm!

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I'm not sure that this is the right direction. "LLVM C" sits atop of LLVM, providing the C interface to the C++ library. This inverts that by making LLVM C a dependency of LLVM.

@andrurogerz
Copy link
Contributor Author

I'm not sure that this is the right direction. "LLVM C" sits atop of LLVM, providing the C interface to the C++ library. This inverts that by making LLVM C a dependency of LLVM.

Ah, ok. I see what you mean. There are a few places outside of llvm-c that include llvm-c/Error.h and llvm-c/Types.h, but nothing pulls in this llvm-c/Core.h. I will resolve this by annotating the LLVMCloneModule at the implementation instead.

@andrurogerz andrurogerz force-pushed the llvm-lib-transform-CloneModule branch from b1062a1 to cdbf07c Compare June 25, 2025 20:19
@andrurogerz andrurogerz force-pushed the llvm-lib-transform-CloneModule branch from cdbf07c to 914916a Compare June 25, 2025 20:19
@andrurogerz andrurogerz changed the title [llvm] include llvm-c/Core.h from CloneModule.cpp for the LLVMCloneModule declaration [llvm] annotate LLVMCloneModule for export Jun 25, 2025
@andrurogerz andrurogerz changed the title [llvm] annotate LLVMCloneModule for export [llvm] annotate LLVMCloneModule for export Jun 25, 2025
@andrurogerz andrurogerz requested a review from compnerd June 25, 2025 23:32
@@ -216,8 +218,7 @@ std::unique_ptr<Module> llvm::CloneModule(

extern "C" {

LLVMModuleRef LLVMCloneModule(LLVMModuleRef M) {
LLVM_ABI LLVMModuleRef LLVMCloneModule(LLVMModuleRef M) {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this only export now if you build llvm as a DLL? The use of the visibility header is fine. Bleh, I think that I missed that this was in the implementation of the llvm-c technically. If this function is not used elsewhere, I'd suggest just moving this function into a new TU in LLVM C.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants