Page MenuHomePhabricator

[CMake] Default to static linking for subprojects.
ClosedPublic

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

Details

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

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?

  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?

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...

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 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.

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 edited the summary of this revision. (Show Details)

@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, 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 marked 2 inline comments as done.
  • Remove space between endif and parens
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Feb 26 2020, 6:04 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?

http://llvm.org/PR45001

Next time, please include this in the commit message.