This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix linker script generation
ClosedPublic

Authored by phosek on Oct 10 2019, 8:14 AM.

Details

Summary

Handle the case when libc++abi and libunwind are being built together
with libc++ in the runtimes build. This logic was used in the previous
implementation but dropped in r374116.

Diff Detail

Repository
rCXX libc++

Event Timeline

phosek created this revision.Oct 10 2019, 8:14 AM
ldionne added inline comments.Oct 10 2019, 8:40 AM
libcxx/cmake/Modules/DefineLinkerScript.cmake
34

How can it be that ${lib} is equal to cxxabi_static or cxxabi_shared, but it's not a target? The same applies to the unwind_xxx targets. I think this has to do with how you configure your build for Fuchsia with HAVE_LIBCXXABI. Can you show me exactly what arguments are used in the CMake invocation?

TBH, I'd like to better understand the purpose of HAVE_LIBCXXABI and HAVE_LIBUNWIND, since those certainly look like slight hacks that would be nice to support better.

34

However, feel free to commit this for the time being in order to fix your build. But I'd really like to have the discussion about HAVE_LIBCXXABI, since I've been meaning to have it for a while.

phosek marked an inline comment as done.
phosek added inline comments.
libcxx/cmake/Modules/DefineLinkerScript.cmake
34

The issue is that when building these in the runtimes build, they get added in whatever order user specifies on the command-line so by the time we're processing libc++, libc++abi and libunwind may have not yet been processed so if(TARGET "${lib}) is going to fail. D68833 is an alternative solution that's trying to address this issue more generally.

phosek marked an inline comment as done.Oct 10 2019, 9:52 PM
phosek added inline comments.
libcxx/cmake/Modules/DefineLinkerScript.cmake
34

Based on the discussion in D68833, it seems like a proper solution is going to require more discussion, so I'm going to land this to fix our toolchain build and work on cleaner solution afterwards.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 10 2019, 9:56 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 9:56 PM