This is an archive of the discontinued LLVM Phabricator instance.

Differential D72372

[CMake] Default to static linking for subprojects.
ClosedPublic

Authored by Meinersbur on Jan 7 2020, 3:51 PM.

Details

Reviewers
serge-sans-paille
MaskRay
fhahn
hfinkel
bollu
Commits
rGd7afdb596e86: [CMake] Default to static linking for subprojects.
rG6369b9bf3118: [CMake] Default to static linking for subprojects.
Summary

Pass plugins introduced D61446 do not support dynamic linking on Windows, hence the option LLVM_${name_upper}_LINK_INTO_TOOLS can only work being set to "ON". Currently, it defaults to "OFF" such that such plugins are inoperable by default on Windows. Change the default for subprojects to follow LLVM_ENABLE_PROJECTS.

Diff Detail

Event Timeline

Meinersbur created this revision.Jan 7 2020, 3:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2020, 3:51 PM
Herald added a subscriber: mgorny. · View Herald Transcript
MaskRay accepted this revision.Jan 8 2020, 4:03 PM

I don't really understand the mechanism on Windows. Hope @serge-sans-paille can confirm.

llvm/cmake/modules/AddLLVM.cmake
873–880

The convention seems that there is no space before (

This revision is now accepted and ready to land.Jan 8 2020, 4:03 PM
serge-sans-paille added a comment.Jan 8 2020, 10:01 PM

I like the idea but

  1. I'm unsure it's fine to have a different default for different platform (but that's arguable)
  2. This would link the *example* Bye plugin statically, which is not something we want (and that's critical).

One workaround would be to warn the user if the setting is set to OFF on Windows, or disable the test on Windows?

Meinersbur added a comment.Jan 9 2020, 9:59 AM
In D72372#1811384, @serge-sans-paille wrote:
  1. I'm unsure it's fine to have a different default for different platform (but that's arguable)

IMHO it is worse if the default settings don't even work. We also have such different default already, e.g. for LLVM_ENABLE_MODULE_DEBUGGING (enabled by default only on MacOS) and LLVM_ENABLE_PLUGINS.

  1. This would link the *example* Bye plugin statically, which is not something we want (and that's critical).

I agree. Ideally, we find a common solution for all example/tutorial code.

One workaround would be to warn the user if the setting is set to OFF on Windows

The OFF setting yields a broken build and as a consequence could even not used. It happens that the dll still builds/links but I wouldn't rely on that.

, or disable the test on Windows?

I dislike disabling tests/examples even though they are supposed to work.

Currently, loadable module give a warning "LLVMHello ignored -- Loadable modules not supported on this platform." We could do the same here with the message explaining how to enable it.
(Actually enabling LLVMHello on Windows requires setting more flags such as LLVM_ENABLE_PLUGINS and LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)

On the other side, I would also like to see -DLLVM_ENABLE_PROJECT=polly to just work. Any ideas?

serge-sans-paille added a comment.Jan 20 2020, 6:33 AM

Currently, loadable module give a warning "LLVMHello ignored -- Loadable modules not supported on this platform." We could do the same here with the message explaining how to enable it.

On the other side, I would also like to see -DLLVM_ENABLE_PROJECT=polly to just work. Any ideas?

What if, in addition of the current diff, you'd just disable the Bye test on windows when compiled statically? With the proper comment, it looks like a good compromise as of now. Ican't think of any simple way to test static builds without introducing more cmake variables...

Meinersbur added a subscriber: hfinkel.Jan 21 2020, 3:04 PM

I talked with @hfinkel about this and think that we should distinct between example passes (static linking has to be enabled explicitly) and project (enabled using LLVM_ENABLE_PROJECT). I will update this diff by adding an option to add_llvm_pass_plugin.

serge-sans-paille added a comment.Jan 22 2020, 12:08 AM
In D72372#1832544, @Meinersbur wrote:

I talked with @hfinkel about this and think that we should distinct between example passes (static linking has to be enabled explicitly) and project (enabled using LLVM_ENABLE_PROJECT). I will update this diff by adding an option to add_llvm_pass_plugin.

I'm fine with that appraoch!

Meinersbur updated this revision to Diff 243429.Feb 8 2020, 9:49 PM
Meinersbur retitled this revision from [CMake] Force static linking for registered plugins on Windows. to [CMake] Default to static linking for subprojects..
Meinersbur edited the summary of this revision. (Show Details)
Meinersbur added a reviewer: hfinkel.

Different default for subprojects.

Herald added a reviewer: bollu. · View Herald TranscriptFeb 8 2020, 9:49 PM
serge-sans-paille added inline comments.Feb 10 2020, 11:34 AM
llvm/cmake/modules/AddLLVM.cmake
864

nit: *the plugin will link it statically by default if...*

873–880

nit: *link statically by default if it was...*

Meinersbur updated this revision to Diff 243707.Feb 10 2020, 5:55 PM
Meinersbur edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B46158: Diff 243707.Feb 10 2020, 5:59 PM
Meinersbur added a comment.Feb 23 2020, 12:08 PM

@serge-sans-paille OK to commit?

ctetreau added a subscriber: ctetreau.Feb 24 2020, 7:56 AM

@Meinersbur, thank you for taking the time to look at linkage on windows. Could you please give a concrete example of an issue that this patch resolves?

I am currently troubleshooting a breakage caused by D61446, and I am wondering if it might be related.

Meinersbur added a comment.Feb 24 2020, 8:52 AM
In D72372#1889407, @ctetreau wrote:

@Meinersbur, thank you for taking the time to look at linkage on windows. Could you please give a concrete example of an issue that this patch resolves?

http://llvm.org/PR45001

Meinersbur updated this revision to Diff 246233.Feb 24 2020, 9:10 AM
Meinersbur marked 2 inline comments as done.
  • Remove space between endif and parens
Harbormaster completed remote builds in B47141: Diff 246233.Feb 24 2020, 9:17 AM
serge-sans-paille accepted this revision.Feb 24 2020, 9:34 AM

@Meinersbur LGTM, thanks

Closed by commit rG6369b9bf3118: [CMake] Default to static linking for subprojects. (authored by Meinersbur). · Explain WhyFeb 24 2020, 10:03 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Feb 26 2020, 6:04 AM
In D72372#1889521, @Meinersbur wrote:
In D72372#1889407, @ctetreau wrote:

@Meinersbur, thank you for taking the time to look at linkage on windows. Could you please give a concrete example of an issue that this patch resolves?

http://llvm.org/PR45001

Next time, please include this in the commit message.

Meinersbur mentioned this in rGdce2ef9e6421: [Polly][docs] Polly release notes..Mar 1 2020, 8:29 AM

Revision Contents

PathSize
llvm/
cmake/
modules/
15 lines
polly/
lib/
1 line

Diff 246241

llvm/cmake/modules/AddLLVM.cmake

Show First 20 Lines • Show All 853 Lines • ▼ Show 20 Lines
# add_llvm_pass_plugin(name [NO_MODULE] ...) # add_llvm_pass_plugin(name [NO_MODULE] ...)
# Add ${name} as an llvm plugin. # Add ${name} as an llvm plugin.
# If option LLVM_${name_upper}_LINK_INTO_TOOLS is set to ON, the plugin is registered statically. # If option LLVM_${name_upper}_LINK_INTO_TOOLS is set to ON, the plugin is registered statically.
# Otherwise a pluggable shared library is registered. # Otherwise a pluggable shared library is registered.
# #
# If NO_MODULE is specified, when option LLVM_${name_upper}_LINK_INTO_TOOLS is set to OFF, # If NO_MODULE is specified, when option LLVM_${name_upper}_LINK_INTO_TOOLS is set to OFF,
# only an object library is built, and no module is built. This is specific to the Polly use case. # only an object library is built, and no module is built. This is specific to the Polly use case.
#
# The SUBPROJECT argument contains the LLVM project the plugin belongs
# to. If set, the plugin will link statically by default it if the
serge-sans-pailleUnsubmitted
Not Done
ReplyInline Actions

nit: *the plugin will link it statically by default if...*

serge-sans-paille: nit: *the plugin will link it statically by default if...*
# project was enabled.
function(add_llvm_pass_plugin name) function(add_llvm_pass_plugin name)
cmake_parse_arguments(ARG cmake_parse_arguments(ARG
"NO_MODULE" "" "" "NO_MODULE" "SUBPROJECT" ""
${ARGN}) ${ARGN})
string(TOUPPER ${name} name_upper) string(TOUPPER ${name} name_upper)
option(LLVM_${name_upper}_LINK_INTO_TOOLS "Statically link ${name} into tools (if available)" OFF) # Enable the plugin by default if it was explicitly enabled by the user.
# Note: If was set to "all", LLVM's CMakeLists.txt replaces it with a
# list of all projects, counting as explicitly enabled.
set(link_into_tools_default OFF)
if (ARG_SUBPROJECT AND LLVM_TOOL_${name_upper}_BUILD)
set(link_into_tools_default ON)
endif()
option(LLVM_${name_upper}_LINK_INTO_TOOLS "Statically link ${name} into tools (if available)" ${link_into_tools_default})
MaskRayUnsubmitted
Done
ReplyInline Actions

The convention seems that there is no space before (

MaskRay: The convention seems that there is no space before `(`
serge-sans-pailleUnsubmitted
Done
ReplyInline Actions

nit: *link statically by default if it was...*

serge-sans-paille: nit: *link statically by default if it was...*
if(LLVM_${name_upper}_LINK_INTO_TOOLS) if(LLVM_${name_upper}_LINK_INTO_TOOLS)
list(REMOVE_ITEM ARG_UNPARSED_ARGUMENTS BUILDTREE_ONLY) list(REMOVE_ITEM ARG_UNPARSED_ARGUMENTS BUILDTREE_ONLY)
# process_llvm_pass_plugins takes care of the actual linking, just create an # process_llvm_pass_plugins takes care of the actual linking, just create an
# object library as of now # object library as of now
add_llvm_library(${name} OBJECT ${ARG_UNPARSED_ARGUMENTS}) add_llvm_library(${name} OBJECT ${ARG_UNPARSED_ARGUMENTS})
target_compile_definitions(${name} PRIVATE LLVM_${name_upper}_LINK_INTO_TOOLS) target_compile_definitions(${name} PRIVATE LLVM_${name_upper}_LINK_INTO_TOOLS)
set_property(TARGET ${name} APPEND PROPERTY COMPILE_DEFINITIONS LLVM_LINK_INTO_TOOLS) set_property(TARGET ${name} APPEND PROPERTY COMPILE_DEFINITIONS LLVM_LINK_INTO_TOOLS)
▲ Show 20 Lines • Show All 1,068 Lines • Show Last 20 Lines

polly/lib/CMakeLists.txt

Show All 19 Lines
if (MSVC_IDE OR XCODE) if (MSVC_IDE OR XCODE)
file(GLOB_RECURSE POLLY_HEADER_FILES "${POLLY_SOURCE_DIR}/include/polly/*.h") file(GLOB_RECURSE POLLY_HEADER_FILES "${POLLY_SOURCE_DIR}/include/polly/*.h")
endif () endif ()
# Use an object-library to add the same files to multiple libs without requiring # Use an object-library to add the same files to multiple libs without requiring
# the sources them to be recompiled for each of them. # the sources them to be recompiled for each of them.
add_llvm_pass_plugin(Polly add_llvm_pass_plugin(Polly
NO_MODULE NO_MODULE
SUBPROJECT Polly
Analysis/DependenceInfo.cpp Analysis/DependenceInfo.cpp
Analysis/PolyhedralInfo.cpp Analysis/PolyhedralInfo.cpp
Analysis/ScopDetection.cpp Analysis/ScopDetection.cpp
Analysis/ScopDetectionDiagnostic.cpp Analysis/ScopDetectionDiagnostic.cpp
Analysis/ScopInfo.cpp Analysis/ScopInfo.cpp
Analysis/ScopBuilder.cpp Analysis/ScopBuilder.cpp
Analysis/ScopGraphPrinter.cpp Analysis/ScopGraphPrinter.cpp
Analysis/ScopPass.cpp Analysis/ScopPass.cpp
▲ Show 20 Lines • Show All 143 Lines • Show Last 20 Lines