Skip to content

[Clang][Cygwin] Disable shared libs on Cygwin by default #138119

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 2, 2025
Merged

[Clang][Cygwin] Disable shared libs on Cygwin by default #138119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions clang/tools/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ endif()
add_clang_subdirectory(c-index-test)

add_clang_subdirectory(clang-refactor)
# For MinGW we only enable shared library if LLVM_LINK_LLVM_DYLIB=ON.
# For MinGW/Cygwin we only enable shared library if LLVM_LINK_LLVM_DYLIB=ON.
# Without that option resulting library is too close to 2^16 DLL exports limit.
if(UNIX OR (MSVC AND LLVM_BUILD_LLVM_DYLIB_VIS) OR (MINGW AND LLVM_LINK_LLVM_DYLIB))
if((UNIX AND NOT CYGWIN) OR (MSVC AND LLVM_BUILD_LLVM_DYLIB_VIS) OR
((MINGW OR CYGWIN) AND LLVM_LINK_LLVM_DYLIB))
add_clang_subdirectory(clang-shlib)
endif()

Expand Down
3 changes: 2 additions & 1 deletion clang/tools/libclang/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ if (LLVM_EXPORTED_SYMBOL_FILE)
DEPENDS ${LIBCLANG_VERSION_SCRIPT_FILE})
endif()

if(LLVM_ENABLE_PIC OR (WIN32 AND NOT LIBCLANG_BUILD_STATIC))
if((NOT (WIN32 OR CYGWIN) AND LLVM_ENABLE_PIC) OR
((WIN32 OR CYGWIN) AND NOT LIBCLANG_BUILD_STATIC))
Copy link
Member

Choose a reason for hiding this comment

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

This bit here is quite unintuitive to read, but I don't have any specific preference for any better way of doing it either...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that's unfortunate and has always been a problem.
LLVM_ENABLE_PIC defaults to ON on Windows, so that condition never worked unless you explicitly disabled LLVM_ENABLE_PIC as found out by @jeremyd2019.

Copy link
Contributor

Choose a reason for hiding this comment

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

this stops libclang.dll from building on (non-mingw) windows when we specify LLVM_ENABLE_PIC, can we revert that part of this change?

Copy link
Member

Choose a reason for hiding this comment

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

Right, so in order not to change the behaviour for other windows platforms, this should probably be if ((NOT CYGWIN AND LLVM_ENABLE_PIC) OR ((WIN32 OR CYGWIN) AND NOT LIBCLANG_BUILD_STATIC)), i.e. removing the WIN32 OR part from the first part of the conditional?

Copy link
Contributor

@jeremyd2019 jeremyd2019 May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused now, the condition before had WIN32 AND NOT LIBCLANG_BUILD_STATIC but that branch was never hit because LLVM_ENABLE_PIC was defaulted ON on Windows. What exactly is LIBCLANG_BUILD_STATIC supposed to do, if not switch from building a shared libclang to building a static one?

Oh, the docs on the option does say "in addition to a shared one"

Copy link
Contributor

Choose a reason for hiding this comment

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

As I recall, the libclang.dll did build successfully so I think that could be reverted. Perhaps I/we just misunderstood what that option was supposed to do

Copy link
Contributor

@jeremyd2019 jeremyd2019 May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

as in,

if(LLVM_ENABLE_PIC OR ((WIN32 OR CYGWIN) AND NOT LIBCLANG_BUILD_STATIC))

Although I'm still not clear on what the WIN32 AND NOT LIBCLANG_BUILD_STATIC case is supposed to be doing, it's pretty clear that Cygwin is like WIN32 in regards to its shared/static libraries

Copy link
Contributor

Choose a reason for hiding this comment

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

sent out #138343

set(ENABLE_SHARED SHARED)
endif()

Expand Down