Initial changes for llvm shared library build using explicit visibility annotations #96630

fsfod · 2024-06-25T13:05:09Z

These are my initial build and code changes to supporting building llvm as shared library/DLL on windows(without force exporting all symbols) and making symbol visibility hidden by default on Linux which adding explicit symbol visibility macros to the whole llvm codebase.

Updated cmake code to allow building llvm-shlib on windows by appending /WHOLEARCHIVE:lib to the linker options.
Remove the hardcoded CMake error from using LLVM_BUILD_LLVM_DYLIB on windows.
Updated CMake to define new macros to control conditional export macros in llvm/Support/Compiler.h
Use /Zc:dllexportInlines- when compiling with clang-cl on windows with a opt out CMake option to disable using it.
Replace some use of LLVM_EXTERNAL_VISIBILITY with new export macros.

Some of the cmake and code changes are based on @tstellar's earlier PR #67502.

I have Windows building using clang-cl, while for MSVC its at-least able to build libllvm, but some tests can't build because llvm iterator template metaprogramming that doesn't work well with dllexport. Linux should build without issue. My full branch is here https://github.com/fsfod/llvm-project/tree/llvm-export-api-20.0 and including all the auto generated export macros from clang tooling based system.

llvm/lib/Support/BLAKE3/CMakeLists.txt

llvm/include/llvm/CodeGen/SelectionDAGNodes.h

llvm/include/llvm/Support/Compiler.h

vgvassilev · 2024-07-02T06:34:07Z

@rnk, can you take a look? This pull request is in the context of https://llvm.org/OpenProjects.html#clang-plugins-windows and google summer of code.

llvm/CMakeLists.txt

rnk

Broadly speaking, yeah, I think this is the right direction for building libLLVM.dll. I'm looking forward to it coming together. :)

I didn't hit approve since only early feedback was requested, and I think there's stuff to iterate on.

llvm/include/llvm/Support/Compiler.h

llvm/tools/llvm-shlib/CMakeLists.txt

rnk · 2024-07-02T18:52:46Z

llvm/tools/llvm-shlib/CMakeLists.txt

+        # reduce dynamic relocations.
+        # Note: for -fno-pic default, the address of a function may be different from
+        # inside and outside libLLVM.so.
+        target_link_options(LLVM PRIVATE LINKER:-Bsymbolic-functions)


vgvassilev · 2024-07-06T15:32:16Z

@fsfod, @compnerd, @tstellar, @rnk, what is the preferred way to move forward here? If we want atomic history we should probably land the whole pull request. It is not clear to me if we can chunk it at a reasonable level...

github-actions · 2024-07-07T15:15:39Z

✅ With the latest revision this PR passed the C/C++ code formatter.

fsfod · 2024-07-08T16:21:49Z

@fsfod, @compnerd, @tstellar, @rnk, what is the preferred way to move forward here? If we want atomic history we should probably land the whole pull request. It is not clear to me if we can chunk it at a reasonable level...

The thing is a lot of these changes can't go in without the rest of changes in my full branch that annotates the whole llvm codebase headers with export macros and the many fixes required from MSVC instantiating all class members. People probably still want the old windows shared library build system to also keep working before all my changes are merged so the export macros on windows would initially have to be a nop.

What i was thinking was just get the export macros definitions and cmake machinery to drive them merged first.

compnerd · 2024-07-08T16:23:44Z

Yeah, I think that annotating the libraries with the ABI boundaries should be good as a first step. It would allow us to split up the patch along maintainer boundaries as well hopefully and keep the size of the review more manageable.

fsfod · 2024-07-15T14:47:22Z

I've added cmake config option(LLVM_BUILD_LLVM_DYLIB_VIS) to test explicit visibility macros on windows and Linux.
When its off the macros are nop on windows and for Linux there is no difference with it off , except the default symbol visibility is not set to hidden for llvm components.
If option is enabled llvm shared library should build on both windows(only clang-cl this early on) and Linux with it on, but tools won't link successfully yet because only limited number of symbols are exported.

llvm/CMakeLists.txt

compnerd · 2024-07-15T22:26:54Z

llvm/include/llvm/Support/Compiler.h

@@ -114,7 +114,8 @@
 /// this attribute will be made public and visible outside of any shared library
 /// they are linked in to.

-#if LLVM_HAS_CPP_ATTRIBUTE(gnu::visibility)
+#if LLVM_HAS_CPP_ATTRIBUTE(gnu::visibility) && defined(__GNUC__) &&            \
+    !defined(__clang__)


Clang should support this under the GNU standard. Can you add a note on why this condition is needed?

I'm not sure what the need for it was, it was part of @tstellar initial changes to the file that i based my code on. I can just remove it if there wasn't some clear purpose for it.

Clang supports it, but IIRC the rules about where in the function declaration the c++11 style attributes can be placed is different between gcc in clang. See the comments here:

https://github.com/tstellar/template-visibility/blob/testing/templates.h

That file is my pain. I did add a option to the clang tool to switch between the two places where to generate the macro for for functions. I also did try generating export macros on template classes, but it just creates ton a more code that needs to be fixed for MSVC and can be problematic for class static fields when classes with instantiations outside the llvm shared library.

vgvassilev · 2024-07-18T12:40:33Z

@compnerd, @tstellar, @rnk, can you take a look. It seems this PR is ready to review.

vgvassilev · 2024-07-31T16:01:40Z

@compnerd, @tstellar, @rnk, can you take a look. It seems this PR is ready to review.

ping.

compnerd · 2024-07-31T18:24:36Z

I think that @petrhosek might have opinions on the CMake changes.

compnerd

Most of the non-CMake changes seem reasonable to me.

llvm/include/llvm/Support/Compiler.h

vgvassilev · 2024-08-07T11:16:31Z

FWIW, the google summer of code midterms are in 9 days and I'd very much like to converge on this PR and hopefully have it merged. That'd be good for the GSoC reporting.

tstellar

I don't understand some of these things as well as I used to, but I don't see any major issues with this patch.

compnerd · 2024-08-08T02:13:42Z

llvm/include/llvm/Support/Compiler.h

+#define LLVM_TEMPLATE_ABI
+#define LLVM_EXPORT_TEMPLATE
+#endif
+#define LLVM_C_ABI LLVM_ABI


We really should not conflate the LLVM ABI and the LLVM C ABI.

The idea was it could maybe made separate later on for some kind of optimized llvmc only build. There also exported functions in Target libs like LLVMInitialize* that are used both in C and c++ API.

That's actually more towards what my point was :) We shouldn't be defining the C ABI in terms of the C++ ABI. The C ABI is guaranteed stable while the C++ is not. It may be someone wants to not expose the C++ ABI (builds statically) but still build the C interface as a DSO.

compnerd · 2024-08-08T02:14:36Z

llvm/include/llvm/Support/Compiler.h

+#if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS) ||       \
+    defined(LLVM_ENABLE_PLUGINS)


We should hoist this into CMake and map this to LLVM_BUILD_STATIC.

I just wanted avoid doing it in CMake yet because its more complex to do and easy to get wrong.

Okay; I guess getting it in is a good first step. The CMake bits, I think you should wait to get sign off from @petrhosek as well.

vgvassilev · 2024-08-14T15:39:31Z

@fsfod, can you rebase this PR?

@compnerd, @tstellar, @petrhosek can we make another review iteration of this PR?

compnerd

The non-CMake portions seem fine. I'd say get @petrhosek's signoff for the CMake bits.

compnerd · 2024-08-14T15:57:04Z

llvm/include/llvm/Support/Compiler.h

 #if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS)
 #define LLVM_EXTERNAL_VISIBILITY LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #else
 #define LLVM_EXTERNAL_VISIBILITY
 #endif
+
+#if (!(defined(_WIN32) || defined(__CYGWIN__)) ||                              \
+     (defined(__MINGW32__) && defined(__clang__)))


Why the defined(__MINGW32__) && defined(__clang__) ?

I don't know it was existing code that got rearranged slightly, it was controlling LLVM_EXTERNAL_VISIBILITY before

compnerd · 2024-08-14T15:58:21Z

llvm/include/llvm/Support/Compiler.h

+#if (!(defined(_WIN32) || defined(__CYGWIN__)) ||                              \
+     (defined(__MINGW32__) && defined(__clang__)))
+#define LLVM_LIBRARY_VISIBILITY LLVM_ATTRIBUTE_VISIBILITY_HIDDEN
+#define LLVM_ALWAYS_EXPORT LLVM_ATTRIBUTE_VISIBILITY_DEFAULT


I'm not sure how I feel about this abstraction (LLVM_ATTRIBUTE_VISIBILITY_*), but I suppose that it doesn't matter too much.

What is the alternative for non windows platforms other than this? I used LLVM_ALWAYS_EXPORT for some special JIT debugger registration functions that were annotated with LLVM_ATTRIBUTE_VISIBILITY_DEFAULT before.

Explicitly spelling out the attribute OSS what I was suggesting. On Linux, protected visibility has interesting behavior where symbols are not interpositionable and are externally visible. Using that is potentially useful for load times on Linux for exported symbols.

I didn't know about protected, wouldn't it also indirectly stop symbols being merging across shared libraries and executables by the loader? maybe that would be what you always want for the JIT debugger symbols.

compnerd · 2024-09-06T17:51:29Z

I think that this is ready to go at this point. This unblocks the subsequent work to start marking the boundaries for the library ABI.

fsfod · 2024-09-06T18:13:14Z

I'm not sure if LLVM_LIBRARY_VISIBILITY error on gvn is the the issue @tstellar mentioned about some attribute types are only supported in different places by some compilers and older versions of clang.

tstellar · 2024-09-06T18:15:48Z

@fsfod Is there a failing buildbot with a log I can look at?

fsfod · 2024-09-06T18:17:30Z

https://lab.llvm.org/buildbot/#/builders/12/builds/5294
https://lab.llvm.org/buildbot/#/builders/78/builds/5463

fsfod · 2024-09-06T18:20:55Z

The error seems to imply LLVM_LIBRARY_VISIBILITY is undefined but compiler.h is included by the file

tstellar · 2024-09-06T18:29:21Z

I thought it might be this: https://github.com/tstellar/template-visibility/blob/testing/templates.h#L37

That buildbot is using clang 14.

tstellar · 2024-09-06T18:32:49Z

I thought it might be this: https://github.com/tstellar/template-visibility/blob/testing/templates.h#L37

That buildbot is using clang 14.

If this is indeed the problem, i would recommend just disabling the annotations for clang < 15. (i.e. have the macros expand to nothing).

fsfod · 2024-09-06T18:50:23Z

Would changing the preprocessor condition from

#if LLVM_HAS_CPP_ATTRIBUTE(gnu::visibility) && defined(__GNUC__) &&            \
    !defined(__clang__)

to

#if LLVM_HAS_CPP_ATTRIBUTE(gnu::visibility) && defined(__GNUC__) &&            \
    (!defined(__clang__) || _clang_major__ < 15)

not be enough to fix it?

tstellar · 2024-09-06T19:07:38Z

@fsfod possibly, I'm not sure what implications that would have in othe parts of the code.

…gnu style attributes on namespaces Fixes errors in GVN.h after changes in llvm#96630 to how LLVM_LIBRARY_VISIBILITY is defined

@compnerd

This is to try and fix buildbot failure on [clang-ppc64-aix](https://lab.llvm.org/buildbot/#/builders/64/builds/881) from my changes in #96630. I don't really know much about AIX so it would be good to have someone more knowledgeable to say if visibility macros on extern templates is needed on AIX similar to ELF. @compnerd @tstellar

@compnerd

) This is to try and fix buildbot failure on [clang-ppc64-aix](https://lab.llvm.org/buildbot/#/builders/64/builds/881) from my changes in llvm#96630. I don't really know much about AIX so it would be good to have someone more knowledgeable to say if visibility macros on extern templates is needed on AIX similar to ELF. @compnerd @tstellar

@compnerd

…107873) Update llvm's TableGen to emit new explicit symbol visibility macros I added in #96630 to the function declarations it creates The generated functions need to be exported from llvm's shared library for Clang and some OpenMP tests. @compnerd

@compnerd

…lvm#107873) Update llvm's TableGen to emit new explicit symbol visibility macros I added in llvm#96630 to the function declarations it creates The generated functions need to be exported from llvm's shared library for Clang and some OpenMP tests. @compnerd

This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL. These macros are similar to the ones i added in #96630, but are for clang. Added explicit symbol visibility macros definitions that are defined in a new header and will be used to for shared library builds of clang to export symbols. Updated clang cmake to define a macro to enable the symbol visibility macros and explicitly disable them for clang tools that always use static linking. --------- Co-authored-by: Aaron Ballman <[email protected]>

This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL. These macros are similar to the ones i added in llvm#96630, but are for clang. Added explicit symbol visibility macros definitions that are defined in a new header and will be used to for shared library builds of clang to export symbols. Updated clang cmake to define a macro to enable the symbol visibility macros and explicitly disable them for clang tools that always use static linking. --------- Co-authored-by: Aaron Ballman <[email protected]>

This is part of the effort to support for enabling plugins on windows by adding better support for building llvm as a DLL. The export macros used here were added in #96630 Since shared library symbols aren't deduplicated across multiple libraries on windows like Linux we have to manually explicitly import and export `Any::TypeId` template instantiations for the uses of `llvm::Any` in the LLVM codebase to support LLVM Windows shared library builds. This change ensures that external code, including LLVM's own tests, can use PassManager callbacks when LLVM is built as a DLL. I also removed the only use of llvm::Any for LoopNest that only existed in debug code and there also doesn't seem to be any code creating `Any<LoopNest>`

) This is part of the effort to support for enabling plugins on windows by adding better support for building llvm as a DLL. The export macros used here were added in llvm#96630 Since shared library symbols aren't deduplicated across multiple libraries on windows like Linux we have to manually explicitly import and export `Any::TypeId` template instantiations for the uses of `llvm::Any` in the LLVM codebase to support LLVM Windows shared library builds. This change ensures that external code, including LLVM's own tests, can use PassManager callbacks when LLVM is built as a DLL. I also removed the only use of llvm::Any for LoopNest that only existed in debug code and there also doesn't seem to be any code creating `Any<LoopNest>`

tstellar reviewed Jun 25, 2024

View reviewed changes

llvm/lib/Support/BLAKE3/CMakeLists.txt Outdated Show resolved Hide resolved

fsfod commented Jun 26, 2024

View reviewed changes

llvm/include/llvm/CodeGen/SelectionDAGNodes.h Outdated Show resolved Hide resolved

fsfod commented Jun 26, 2024

View reviewed changes

llvm/include/llvm/Support/Compiler.h Outdated Show resolved Hide resolved

rnk reviewed Jul 2, 2024

View reviewed changes

llvm/CMakeLists.txt Outdated Show resolved Hide resolved

rnk reviewed Jul 2, 2024

View reviewed changes

fsfod changed the title ~~Sketch of build and code changes for llvm shared library build using explicit visibility annotations~~ Initial changes for llvm shared library build using explicit visibility annotations Jul 15, 2024

compnerd reviewed Jul 15, 2024

View reviewed changes

fsfod marked this pull request as ready for review July 17, 2024 11:05

fsfod requested a review from rnk July 22, 2024 13:23

compnerd requested a review from petrhosek July 31, 2024 18:24

compnerd reviewed Jul 31, 2024

View reviewed changes

llvm/include/llvm/Support/Compiler.h Outdated Show resolved Hide resolved

tstellar reviewed Aug 7, 2024

View reviewed changes

compnerd reviewed Aug 8, 2024

View reviewed changes

compnerd reviewed Aug 14, 2024

View reviewed changes

fsfod force-pushed the exported-api-pr branch from 1a98960 to 94cbc38 Compare August 14, 2024 16:19

llvmbot added cmake Build system in general and CMake in particular llvm:support llvm:ir llvm:analysis labels Aug 14, 2024

fsfod added 2 commits September 6, 2024 13:44

Remove comment about $<LINK_LIBRARY:WHOLE_ARCHIVE,${library}>

4241049

Remove comment thats repeating what i said earlier in the cmake file

bdcd2a3

compnerd merged commit 59f8796 into llvm:main Sep 6, 2024
8 checks passed

fsfod mentioned this pull request Sep 7, 2024

[LLVM]Fix symbol visibility macros not being define for AIX #107705

Merged

This was referenced Sep 9, 2024

[TableGen] Add explicit symbol visibility macros to code generated #107873

Merged

[llvm] Support llvm::Any across shared libraries on windows #108051

Merged

[Clang] Add explicit visibility symbol macros #108276

Merged

fsfod mentioned this pull request Sep 20, 2024

Meta issue to track changes for adding plugin and LLVM_BUILD_LLVM_DYLIB support for Windows #109483

Open

40 tasks

fsfod deleted the exported-api-pr branch October 14, 2024 22:21

		#if defined(LLVM_BUILD_LLVM_DYLIB) \|\| defined(LLVM_BUILD_SHARED_LIBS) \|\| \
		defined(LLVM_ENABLE_PLUGINS)

Initial changes for llvm shared library build using explicit visibility annotations #96630

Initial changes for llvm shared library build using explicit visibility annotations #96630

Conversation

fsfod commented Jun 25, 2024 • edited Loading

vgvassilev commented Jul 2, 2024

rnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgvassilev commented Jul 6, 2024 • edited Loading

github-actions bot commented Jul 7, 2024 • edited Loading

fsfod commented Jul 8, 2024 • edited Loading

compnerd commented Jul 8, 2024

fsfod commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstellar Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgvassilev commented Jul 18, 2024

vgvassilev commented Jul 31, 2024

compnerd commented Jul 31, 2024

compnerd left a comment

Choose a reason for hiding this comment

vgvassilev commented Aug 7, 2024

tstellar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgvassilev commented Aug 14, 2024

compnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compnerd commented Sep 6, 2024

fsfod commented Sep 6, 2024

tstellar commented Sep 6, 2024

fsfod commented Sep 6, 2024

fsfod commented Sep 6, 2024

tstellar commented Sep 6, 2024

tstellar commented Sep 6, 2024

fsfod commented Sep 6, 2024

tstellar commented Sep 6, 2024

fsfod commented Jun 25, 2024 •

edited

Loading

vgvassilev commented Jul 6, 2024 •

edited

Loading

github-actions bot commented Jul 7, 2024 •

edited

Loading

fsfod commented Jul 8, 2024 •

edited

Loading

tstellar Jul 16, 2024 •

edited

Loading