Skip to content
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

Fix LDC compilation with LDC 1.37 universal on macOS. Make ExtractDMDSystemLinker.cmake compatible with -target and -arch #4431

Merged
merged 9 commits into from
Mar 1, 2024

Conversation

JohanEngelen
Copy link
Member

I need this fix when the system D compiler has -target in its default linker line

@@ -64,6 +64,9 @@ separate_arguments(linker_line)
list(GET linker_line 0 D_LINKER_COMMAND)
list(REMOVE_AT linker_line 0)

# Fixup "-target triple" argument, which would be turned into "-target;triple" by `separate_arguments`. Replace ";" with "=".
string(REGEX REPLACE ";-target;" ";--target=" linker_line "${linker_line}")
Copy link
Member

Choose a reason for hiding this comment

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

linker_line and D_LINKER_ARGS later are CMake lists, so semicolons separating individual args should be fine. IIRC, Apple clang is picky and requires -target <bla>, not accepting --target=bla - might have been an older version though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that the D_LINKER_ARGS is passed to CMake, which does something smart: if the argument does not start with -, it assumes it is a library and will add -l in front of it. Thus -target some-triple is converted to -target -lsome-triple.

Copy link
Member

Choose a reason for hiding this comment

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

OMG - yeah I hit that too sometime. That's because we use target_link_libraries() for these (with LDC_LINK_MANUALLY=ON). There's https://cmake.org/cmake/help/latest/command/target_link_options.html since CMake v3.13, which would presumably fix this and similar ugliness.

Copy link
Member Author

Choose a reason for hiding this comment

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

thnx for the target_link_options suggestion.
I've been conservative in applying it (not changing it for other linker args that we pass), because some of the linker "flags" in our script are actually linker dependencies (i.e. libraries) such as LDC_LIB==LDCShared.

@JohanEngelen
Copy link
Member Author

I just hit this when trying to build LDC locally with LDC 1.36.0 universal, because our ldc2.conf contains:

        "-Xcc=-target",
        "-Xcc=arm64-apple-macos",

@JohanEngelen
Copy link
Member Author

Updated the PR to use target_link_options. This increases the CMake requirement from 3.4 to 3.13. Note that LLVM requires v3.20.

@JohanEngelen
Copy link
Member Author

reverted to the old solution if CI likes that better; perhaps somehow the flags are not passed correctly for some of the CI cases with more complex flag setups

@JohanEngelen
Copy link
Member Author

JohanEngelen commented Feb 29, 2024

This patch without using target_link_options does not work with the -arch flag that LDC 1.37 universal macOS package uses. (although CI does like that version)

@JohanEngelen
Copy link
Member Author

JohanEngelen commented Mar 1, 2024

The difference between target_link_options and target_link_libraries is that the first adds flags at start of commandline, the latter at the end.
There are some other small deltas:

target_link_options
[108/1132] : && /usr/bin/c++ -DDMDV2 -O3 -DNDEBUG -m64 -Xlinker --export-dynamic -L/opt/hostedtoolcache/dc/dmd-2.107.1-rc.1/x64/dmd2/linux/bin64/../lib64 -Bstatic -lphobos2 -Bdynamic -lpthread -lm -lrt -ldl obj/ldc2.o -o bin/ldc2 lib/libldc.a <LLVM libs> -lz -lrt -ldl -lpthread -lm -Wl,--export-dynamic -ldl && :

target_link_libraries
[108/1132] : && /usr/bin/c++ -DDMDV2 -O3 -DNDEBUG obj/ldc2.o -o bin/ldc2 lib/libldc.a <LLVM libs> -lz -lrt -ldl -lpthread -lm -Wl,--export-dynamic -m64 -Xlinker --export-dynamic -L/opt/hostedtoolcache/dc/dmd-2.107.1-rc.1/x64/dmd2/linux/bin64/../lib64 -Xlinker -Bstatic -lphobos2 -Xlinker -Bdynamic -lpthread -lm -lrt -ldl -ldl -lpthread -lm && :

There is also this note in CMake documentation about target_link_options, that I do not yet know whether it is important for us:
Note This command cannot be used to add options for static library targets, since they do not use a linker.

@JohanEngelen JohanEngelen added this to the 1.37.0 milestone Mar 1, 2024
@JohanEngelen
Copy link
Member Author

I think I have it working well now. Will clean up after CI confirms.

@JohanEngelen JohanEngelen changed the title Make ExtractDMDSystemLinker.cmake compatible with -target Fix LDC compilation with LDC 1.37 universal on macOS. Make ExtractDMDSystemLinker.cmake compatible with -target and -arch Mar 1, 2024
@JohanEngelen JohanEngelen enabled auto-merge (squash) March 1, 2024 12:25
@kinke
Copy link
Member

kinke commented Mar 1, 2024

Thx, just in time for v1.37. :)

@JohanEngelen JohanEngelen merged commit 0b62d9d into master Mar 1, 2024
39 checks passed
@kinke kinke deleted the JohanEngelen-targetlink branch March 1, 2024 14:08
JohanEngelen added a commit to weka/ldc that referenced this pull request Jul 24, 2024
…SystemLinker.cmake compatible with `-target` and `-arch` (ldc-developers#4431)

The attempt to use CMake's `target_link_options` for `D_LINKER_ARGS` failed, because the list can be a mix of library dependencies and options.

LDC upstream: 0b62d9d
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.

2 participants