Page MenuHomePhabricator

[libc++] Move the linker script generation step to CMake
ClosedPublic

Authored by ldionne on Oct 2 2019, 9:59 AM.

Details

Summary

This allows the linker script generation to query CMake properties
(specifically the dependencies of libc++.so) instead of having to
carry these dependencies around manually in global variables. Notice
the removal of the LIBCXX_INTERFACE_LIBRARIES global variable.

Diff Detail

Event Timeline

ldionne created this revision.Oct 2 2019, 9:59 AM
Herald added a project: Restricted Project. · View Herald Transcript

My goal with this patch is to eliminate some global variables from the CMake sources, which make my life harder while refactoring other parts of the build (I'm working on other, upcoming patches).

I checked that the linker script generated with this patch matched the one generated before this patch on a Docker container running Ubuntu.

thakis added inline comments.
libcxx/cmake/Modules/DefineLinkerScript.cmake
48

We recently made it so that the script is silent during normal operation. It looks like this adds the build output back. Could you omit this please? Successful build steps should be silent.

ldionne marked an inline comment as done.Oct 2 2019, 11:44 AM
This comment was removed by ldionne.
libcxx/cmake/Modules/DefineLinkerScript.cmake
48

This build output is only included if the verbose mode of the CMake generator is used.

ldionne marked an inline comment as done.Oct 2 2019, 11:57 AM

Sorry, I sent the previous comment while I was still drafting.

@phosek @thakis I do have one question. Is there a reason why we want the linker script to look like

INPUT(libc++.so.1 -lunwind -lc++abi)

instead of

INPUT(libc++.so.1 -lpthread -lc <etc...> -lunwind -lc++abi)

It seems like it should be OK to include -lpthread and friends in the linker script, since we need to link against those too, no?

libcxx/cmake/Modules/DefineLinkerScript.cmake
48

This build output is only included if the verbose mode of the CMake generator is used.

Scratch that, I was in the process of verifying the claim when I sent out the comment!

So it turns out the comment is only printed when the Makefile generator is used, but I think you guys use Ninja. LMK if you still want me to remove it.

Also, if you really want your build not to output anything when it's successful, there's more you need to do than just suppress these intermediate status updates, since several other tools will have intermediate output too.

I do not know why the linker script looks like it does.

I don't particularly care about the make build's output. I weakly feel that build steps should be silent (unless they fail) in all generators (http://www.linfo.org/rule_of_silence.html), but cmake's make generator output produces pretty verbose output by default already, so it's just another broken window there.

It turns out that @EricWF was the one to originally write the script, so maybe he knows why the output is

INPUT(libc++.so.1 -lunwind -lc++abi)

instead of

INPUT(libc++.so.1 -lpthread -lc <etc...> -lunwind -lc++abi)
phosek added a comment.Oct 7 2019, 5:35 PM

It turns out that @EricWF was the one to originally write the script, so maybe he knows why the output is

INPUT(libc++.so.1 -lunwind -lc++abi)

instead of

INPUT(libc++.so.1 -lpthread -lc <etc...> -lunwind -lc++abi)

In case of shared libraries, the libpthread and libc dependency is already encoded in the shared library itself (e.g. in case of ELF it'd be as DT_NEEDED entries), specifying them in the linker script is not necessary and in many cases undesirable (e.g. if your application doesn't use any threading API, you probably don't want to get implicit DT_NEEDED dependency on libpthread just because you're linking libc++). Where it'd make sense is for static libraries, but we don't currently generate linker script for those.

libcxx/cmake/Modules/DefineLinkerScript.cmake
47

Would the file redirect (>) work on Windows as well?

ldionne marked an inline comment as done.Oct 8 2019, 7:58 AM

In case of shared libraries, the libpthread and libc dependency is already encoded in the shared library itself (e.g. in case of ELF it'd be as DT_NEEDED entries), specifying them in the linker script is not necessary and in many cases undesirable (e.g. if your application doesn't use any threading API, you probably don't want to get implicit DT_NEEDED dependency on libpthread just because you're linking libc++). Where it'd make sense is for static libraries, but we don't currently generate linker script for those.

Ok. After r374053 and r374056, the linker script will contain the right dependencies.

I think this is good to go unless we're using a linker script on Windows and the redirection doesn't work, in which case I'll find a workaround.

libcxx/cmake/Modules/DefineLinkerScript.cmake
47

I don't think so, but would you create a linker script on Windows? I thought this was only on Linux?

I don't think linker scripts are a thing on Window. lld-link doesn't support them. Maybe something in MinGW does, I don't know.

I don't think linker scripts are a thing on Window. lld-link doesn't support them. Maybe something in MinGW does, I don't know.

GNU ld does support linker script on COFF (somewhat at least, I think), but lld doesn't.

ldionne accepted this revision.Oct 8 2019, 1:59 PM

I'll go ahead with this since there doesn't seem to be a clear agreement that we need this to work on Windows.

This revision is now accepted and ready to land.Oct 8 2019, 1:59 PM
This revision was automatically updated to reflect the committed changes.
vitalybuka added inline comments.
libcxx/cmake/Modules/DefineLinkerScript.cmake
41

https://llvm.org/docs/CMake.html min requirement is 3.4.3
but JOIN is 3.4.12

vitalybuka marked an inline comment as done.Oct 8 2019, 3:21 PM
vitalybuka added inline comments.
libcxx/cmake/Modules/DefineLinkerScript.cmake
41

I see a fix r374120

phosek added a comment.Oct 9 2019, 3:30 PM

I think this is good to go unless we're using a linker script on Windows and the redirection doesn't work, in which case I'll find a workaround.

I don't think linker scripts are a thing on Window. lld-link doesn't support them. Maybe something in MinGW does, I don't know.

GNU ld does support linker script on COFF (somewhat at least, I think), but lld doesn't.

It's not about using linker scripts on Windows but cross-compiling for other systems that use linker scripts on Windows, e.g. we use linker scripts in Fuchsia and would like to build Clang toolchain including all runtimes on Windows which requires running this code when cross-compiling runtimes for Fuchsia.

phosek added a comment.Oct 9 2019, 3:31 PM

This seems to have broke our build, I checked the generated linker script and it has the following format:

INPUT(libc++.so.2 -lunwind_shared -lcxxabi_shared)

libunwind and libc++abi dependencies are incorrect, it should be:

INPUT(libc++.so.2 -lunwind -lc++abi)

This seems to have broke our build, I checked the generated linker script and it has the following format:

INPUT(libc++.so.2 -lunwind_shared -lcxxabi_shared)

libunwind and libc++abi dependencies are incorrect, it should be:

INPUT(libc++.so.2 -lunwind -lc++abi)

D68791 is a fix.

This seems to have broke our build, I checked the generated linker script and it has the following format:

INPUT(libc++.so.2 -lunwind_shared -lcxxabi_shared)

libunwind and libc++abi dependencies are incorrect, it should be:

INPUT(libc++.so.2 -lunwind -lc++abi)

How do you configure your build? The paths were certainly correct when I tested this on Linux. I think this may have something to do with the fact that you use the (kind of hacky) HAVE_LIBCXXABI switch. If you let me know how you configure your build, I'll try to repro and fix it.

This seems to have broke our build, I checked the generated linker script and it has the following format:

INPUT(libc++.so.2 -lunwind_shared -lcxxabi_shared)

libunwind and libc++abi dependencies are incorrect, it should be:

INPUT(libc++.so.2 -lunwind -lc++abi)

How do you configure your build? The paths were certainly correct when I tested this on Linux. I think this may have something to do with the fact that you use the (kind of hacky) HAVE_LIBCXXABI switch. If you let me know how you configure your build, I'll try to repro and fix it.

Oops, I didn't reload before replying. I'll follow up on https://reviews.llvm.org/D68791.