r365825 - [clang-shlib] Fix clang-shlib for PRIVATE dependencies

Tue Jul 16 10:33:07 PDT 2019

Makes sense, thanks. Seems like INTERFACE is the way to go then.

From: <cbieneman at apple.com> on behalf of Chris Bieneman <beanz at apple.com>
Date: Tuesday, July 16, 2019 at 10:32 AM
To: Shoaib Meenai <smeenai at fb.com>
Cc: Alex Bradbury <asb at lowrisc.org>, cfe-commits <cfe-commits at lists.llvm.org>
Subject: Re: r365825 - [clang-shlib] Fix clang-shlib for PRIVATE dependencies

I get that CMake does something different for PRIVATE, but the way we use CMake in LLVM we really can't make a private distinction reasonable. Because we don't sub-divide our headers per library, we don't support per-dependency include paths in LLVM or Clang's libraries.

This behavior also assumes you are using `target_include_directories` to assign PRIVATE | PUBLIC | INTERFACE to include directories, which we don't generally do in LLVM (a quick grep showed one place in LLVM code excluding our google benchmark drop). We do generally use `include_directories` which always treats things as PRIVATE, so they don't cascade to dependencies.

-Chris

On Jul 12, 2019, at 7:16 PM, Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>> wrote:

I struggled for a while thinking why PRIVATE might be useful in a target_link_libraries call for a shared library, and then I found https://cmake.org/pipermail/cmake/2016-May/063400.html<https://urldefense.proofpoint.com/v2/url?u=https-3A__cmake.org_pipermail_cmake_2016-2DMay_063400.html&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=NhVmexDDl4XvCBWPO8Gl6v5pSPl5UAQ-g0gZ9RQeJl0&s=GzTNsMdGWEe8rJMKHfM017MWdw-WBJyV493ARE6B8YE&e=>. The relevant bit is:

If you were paying careful attention, you would have noticed that when A
links in B as PRIVATE, the include directories of B never propagate to
something linking to A, but if A is a static library, then the *linking* of
B behaves as though the relationship was PUBLIC. This
PRIVATE-becomes-PUBLIC behaviour for static libraries only applies to the
*linking*, not to the other dependencies (compiler options/flags and
include search paths).

So PRIVATE/INTERFACE/PUBLIC doesn’t make any difference as far as the actual linking goes, but it does affect propagation of other options, and I think it’s valid to want to have a PRIVATE dependency for a static library.

From: <cbieneman at apple.com<mailto:cbieneman at apple.com>> on behalf of Chris Bieneman <beanz at apple.com<mailto:beanz at apple.com>>
Date: Friday, July 12, 2019 at 9:08 AM
To: Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>>
Cc: Alex Bradbury <asb at lowrisc.org<mailto:asb at lowrisc.org>>, cfe-commits <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>>
Subject: Re: r365825 - [clang-shlib] Fix clang-shlib for PRIVATE dependencies

Ah! I see what is going on here. This is kinda a silliness of CMake. `PRIVATE` linkage for a static archive is silly, and shouldn't be possible. All link dependencies for static archives are really `INTERFACE` dependencies, and it looks like CMake is doing something kinda silly instead of producing a reasonable error or warning like it probably should do.

For context, `PRIVATE` linkage in the context of that change would mean DirectoryWalker links something, but things that link DirectoryWalker don't. That just isn't possible with static archives, so they become interface dependencies (and CMake seems to insert a generator expression to make that work).

In `llvm_add_library` we always use `PRIVATE` linkage for shared libraries, and `INTERFACE` for static, which does the right thing.

Unless there is a better reason than a new patch coming in, I think the right fix is to revert this back and expect the new patch to correct its linkage behavior.

-Chris

On Jul 12, 2019, at 8:53 AM, Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>> wrote:

See https://reviews.llvm.org/D58418#1577670<https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D58418-231577670&d=DwMFAg&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=BaInVCyhXHMyre_oyIEnYhuQC5zsW6p65QBgZLR7ujc&s=7bhRDMtKEyVqFDlMWzo361mQT2N8lcOt-YDh-XGjZok&e=>. More generally it would appear for any static library with a PRIVATE dependency though.

I guess an alternative would be to use the LINK_LIBRARIES property (which should be free of generator expressions, I believe) to propagate dependencies instead of INTERFACE_LINK_LIBRARIES, and just assume that no one is gonna set an INTERFACE dependency on a static library. (Supporting PRIVATE dependencies on a static library definitely seems more valuable than supporting INTERFACE dependencies.)

From: <cbieneman at apple.com<mailto:cbieneman at apple.com>> on behalf of Chris Bieneman <beanz at apple.com<mailto:beanz at apple.com>>
Date: Friday, July 12, 2019 at 8:49 AM
To: Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>>
Cc: Alex Bradbury <asb at lowrisc.org<mailto:asb at lowrisc.org>>, cfe-commits <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>>
Subject: Re: r365825 - [clang-shlib] Fix clang-shlib for PRIVATE dependencies

One of the benefits of the object library approach for generating the clang dylib is that it was compatible with BUILD_SHARED_LIBS. We had lots of issues with libLLVM where people using BUILD_SHARED_LIBS would make changes that broke it, so I was trying to make the clang dylib in a way that it could always be enabled.

Do we know where the nested generator expression was coming from?

-Chris

On Jul 12, 2019, at 8:32 AM, Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>> wrote:

Oops, sorry about the breakage.

Chris, aren't BUILD_SHARED_LIBS and the combined Clang dylib incompatible? Should we disable building the latter if the former is set?

From: Alex Bradbury <asb at lowrisc.org<mailto:asb at lowrisc.org>>
Date: Friday, July 12, 2019 at 2:02 AM
To: Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>>
Cc: cfe-commits <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>>
Subject: Re: r365825 - [clang-shlib] Fix clang-shlib for PRIVATE dependencies

On Thu, 11 Jul 2019 at 22:20, Shoaib Meenai via cfe-commits
<cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>> wrote:

Author: smeenai
Date: Thu Jul 11 14:20:38 2019
New Revision: 365825

URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D365825-26view-3Drev&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=ETCU2JhfBcjWz-nbe6LUVSRQR0T1f3wCzxLIhKlMmQo&s=R9NSZG1XQDVSiE0wJUgb1kMUrG6bJkG3v5GDcTdkpAk&e=
Log:
[clang-shlib] Fix clang-shlib for PRIVATE dependencies

Any static library with a PRIVATE dependency ends up with a
$<LINK_ONLY:...> generator expression in its INTERFACE_LINK_LIBRARIES,
which won't be evaluated by the $<TARGET_PROPERTY:...>, so we end up
with an unevaluated generator expression in the generated build file and
Ninja chokes on the dollar sign. Just use the static library directly
for its dependencies instead of trying to propagate dependencies
manually.

Differential Revision: https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D64579&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=ETCU2JhfBcjWz-nbe6LUVSRQR0T1f3wCzxLIhKlMmQo&s=mutN2zF1KixMEVFjNzG_Q7iVJzG5ECbrU4tX3AKWLVQ&e=

This seems to break a -DBUILD_SHARED_LIBS build for me. It fails at
the point of linking lib/libclang_shared.so.9svn with errors like:
ld.lld: error: undefined symbol: llvm::llvm_unreachable_internal(char
const*, char const*, unsigned int)
referenced by Attributes.cpp:34 (/home/asb/llvm-project/clang/lib/Basic/Attributes.cpp:34)
               tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Attributes.cpp.o:(clang::attr::getSubjectMatchRuleSpelling(clang::attr::SubjectMatchRule))

ld.lld: error: undefined symbol: llvm::llvm_unreachable_internal(char
const*, char const*, unsigned int)
referenced by TargetCXXABI.h:168 (/home/asb/llvm-project/clang/include/clang/Basic/TargetCXXABI.h:168)
               tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Attributes.cpp.o:(clang::TargetCXXABI::isMicrosoft() const)

ld.lld: error: undefined symbol: llvm::llvm_unreachable_internal(char
const*, char const*, unsigned int)
referenced by TargetCXXABI.h:149 (/home/asb/llvm-project/clang/include/clang/Basic/TargetCXXABI.h:149)
               tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Attributes.cpp.o:(clang::TargetCXXABI::isItaniumFamily() const)

ld.lld: error: undefined symbol: llvm::EnableABIBreakingChecks
referenced by Attributes.cpp
               tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Attributes.cpp.o:(llvm::VerifyEnableABIBreakingChecks)

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190716/80c5e494/attachment-0001.html>