[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a subscriber: jfb.
beanz added a comment.

Adding @jfb since this is his domain now too.


Repository:
  rC Clang

https://reviews.llvm.org/D45639



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49486: [cfe][CMake] Export the clang resource directory

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I’m not opposed to this patch, but is there a real need for this? Have you 
considered just running clang with the `-print-resource-dir` flag?


https://reviews.llvm.org/D49486



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49486: [cfe][CMake] Export the clang resource directory

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

Please check and see if that works for you. In general I prefer to avoid adding 
new features that aren't used in-tree unless it is unavoidable or would be 
difficult to work around out-of-tree. This one seems pretty straightforward to 
me.


https://reviews.llvm.org/D49486



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50618: Refactor Darwin driver to refer to runtimes by component

[Chris Bieneman via Phabricator via cfe-commits]

beanz created this revision.
beanz added reviewers: phosek, bruno, arphaman.

In r335809, Petr Hosek lays out support for what he calls the multiarch
runtimes layout. This new way of laying out the directories for runtime
libraries is workable for all platforms. Petr did some of the common
infrastructure and made it work for Linux and Fuscia. This patch is a
cleanup to the Darwin and MachO drivers to serve as a step toward
supporting it in Darwin.

This patch does primarily two things:
(1) Changes the APIs for how the Darwin driver refers to compiler-rt
libraries to use the component names, similar to how Linux and Fuscia do

(2) Removes some legacy functionality for supporting macOS versions
before 10.6. This functionality is effectively dead code because in
r339277, the support was removed from compiler-rt for generating the 10.4
runtime support library, and Xcode 10 (currently in beta) removes
libgcc_s.10.4 and libgcc_s.10.5 from the macOS SDK.

With this patch landed a subsequent patch can modify
MachO::AddLinkRuntimeLib to support the multiarch runtimes layout.

Worth noting: None of the removed functionality was actually covered in
the test suite. So no test case updates are required.


Repository:
  rC Clang

https://reviews.llvm.org/D50618

Files:
  lib/Driver/ToolChains/Darwin.cpp
  lib/Driver/ToolChains/Darwin.h

Index: lib/Driver/ToolChains/Darwin.h
===
--- lib/Driver/ToolChains/Darwin.h
+++ lib/Driver/ToolChains/Darwin.h
@@ -189,9 +189,9 @@
 
   /// Add a runtime library to the list of items to link.
   void AddLinkRuntimeLib(const llvm::opt::ArgList &Args,
- llvm::opt::ArgStringList &CmdArgs,
- StringRef DarwinLibName,
- RuntimeLinkOptions Opts = RuntimeLinkOptions()) const;
+ llvm::opt::ArgStringList &CmdArgs, StringRef Component,
+ RuntimeLinkOptions Opts = RuntimeLinkOptions(),
+ bool IsShared = false) const;
 
   /// Add any profiling runtime libraries that are needed. This is essentially a
   /// MachO specific version of addProfileRT in Tools.cpp.
@@ -252,6 +252,8 @@
 return llvm::ExceptionHandling::None;
   }
 
+  virtual StringRef getOSLibraryNameSuffix() const { return ""; }
+
   /// }
 };
 
@@ -418,7 +420,7 @@
  Action::OffloadKind DeviceOffloadKind) const override;
 
   StringRef getPlatformFamily() const;
-  StringRef getOSLibraryNameSuffix() const;
+  StringRef getOSLibraryNameSuffix() const override;
 
 public:
   static StringRef getSDKName(StringRef isysroot);
Index: lib/Driver/ToolChains/Darwin.cpp
===
--- lib/Driver/ToolChains/Darwin.cpp
+++ lib/Driver/ToolChains/Darwin.cpp
@@ -513,8 +513,7 @@
   // These libraries should be linked first, to make sure the
   // __safestack_init constructor executes before everything else
   if (getToolChain().getSanitizerArgs().needsSafeStackRt()) {
-getMachOToolChain().AddLinkRuntimeLib(Args, CmdArgs,
-  "libclang_rt.safestack_osx.a",
+getMachOToolChain().AddLinkRuntimeLib(Args, CmdArgs, "safestack",
   toolchains::Darwin::RLO_AlwaysLink);
   }
 
@@ -917,8 +916,17 @@
 }
 
 void MachO::AddLinkRuntimeLib(const ArgList &Args, ArgStringList &CmdArgs,
-  StringRef DarwinLibName,
-  RuntimeLinkOptions Opts) const {
+  StringRef Component, RuntimeLinkOptions Opts,
+  bool IsShared) const {
+  SmallString<64> DarwinLibName = StringRef("libclang_rt.");
+  // an Darwin the builtins compomnent is not in the library name
+  if (Component != "builtins") {
+DarwinLibName += Component;
+if (!(Opts & RLO_IsEmbedded))
+  DarwinLibName += "_";
+  }
+  DarwinLibName += getOSLibraryNameSuffix();
+  DarwinLibName += IsShared ? "_dynamic.dylib" : ".a";
   SmallString<128> Dir(getDriver().ResourceDir);
   llvm::sys::path::append(
   Dir, "lib", (Opts & RLO_IsEmbedded) ? "macho_embedded" : "darwin");
@@ -1022,10 +1030,8 @@
   ArgStringList &CmdArgs) const {
   if (!needsProfileRT(Args)) return;
 
-  AddLinkRuntimeLib(
-  Args, CmdArgs,
-  (Twine("libclang_rt.profile_") + getOSLibraryNameSuffix() + ".a").str(),
-  RuntimeLinkOptions(RLO_AlwaysLink | RLO_FirstLink));
+  AddLinkRuntimeLib(Args, CmdArgs, "profile",
+RuntimeLinkOptions(RLO_AlwaysLink | RLO_FirstLink));
 
   // If we have a symbol export directive and we're linking in the profile
   // runtime, automatically export symbols necessary to implement some of the
@@ -1042,12 +1048,7 @@
   StringRef Sanitizer,
   bool Shared) const {
   auto RLO = RuntimeLinkOptio

[PATCH] D50755: [Driver] -print-target-triple and -print-effective-triple options

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rC Clang

https://reviews.llvm.org/D50755



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50547: [Driver] Use normalized triples for multiarch runtime path

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rC Clang

https://reviews.llvm.org/D50547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50547: [Driver] Use normalized triples for multiarch runtime path

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

Just want to bring an IRC conversation that I had with @phosek into the proper 
code review.

This patch is great as-is, but if we want to extend this to Darwin there are 
some problems because of how Darwin handles triples. Specifically Darwin has 
multiple potentially valid triples that can mean the same things. For example 
x86_64-apple-darwin17.7.0 and x86_64-apple-macos10.13.6 are 100% 
interchangeable (even down to referring to the same OS version). Things get 
even stranger when you start talking about the -simulator triples...

This means that to support this in Darwin we need to support iterating over a 
list of triples. We also probably want to cache that list so that we don't run 
`getVFS().exists(...)` over and over again. @phosek said on IRC he will update 
the patch to reflect our conversation.

Thanks @phosek for all the great work on this!


Repository:
  rC Clang

https://reviews.llvm.org/D50547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50547: [Driver] Use normalized triples for multiarch runtime path

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.

LGTM as-is




Comment at: clang/lib/Driver/ToolChain.cpp:372
 
-  const Driver &D = getDriver();
-  SmallString<128> P(D.ResourceDir);
-  llvm::sys::path::append(P, D.getTargetTriple(), "lib");
-  if (getVFS().exists(P)) {
+  for (const auto &LibPath : getLibraryPaths()) {
+SmallString<128> P(LibPath);

phosek wrote:
> One possible alternative I've considered would be to simply return 
> `-lclang_rt..` instead of returning the full path when 
> `getLibraryPaths()` is not empty. That should work because we add all paths 
> in this list as `-L` in `AddFilePathLibArgs` and it's more consistent with 
> e.g. `-lgcc_s`.
I'd prefer not doing that since Darwin doesn't call AddFilePathLibArgs, and 
adjusting to that is a larger change on the Darwin side.


Repository:
  rC Clang

https://reviews.llvm.org/D50547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31363: [libc++] Remove cmake glob for source files

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

Building libcxx without LLVM's CMake modules is very important, not just to 
Apple. This is how several open source distributions work, and it would be a 
huge disservice to break this. Same is true for compiler-rt, libcxxabi, 
libunwind, etc.

Historically we've been drawing the line that building and running tests for 
runtime projects can require LLVM, but building the runtime libraries 
themselves must work without LLVM. I believe that is still the correct line to 
draw.


https://reviews.llvm.org/D31363



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51440: [ToolChains] Link to compiler-rt with -L + -l when possible

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a reviewer: phosek.
beanz added a subscriber: phosek.
beanz added a comment.

Looping in @phosek.


Repository:
  rC Clang

https://reviews.llvm.org/D51440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51440: [ToolChains] Link to compiler-rt with -L + -l when possible

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a subscriber: dexonsmith.
beanz added a comment.

Unfortunately I can't make this kind of policy decision about whether or not 
this would be acceptable for Darwin. That would probably need to be @dexonsmith.


Repository:
  rC Clang

https://reviews.llvm.org/D51440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51645: [CMake] Don't use -rtlib=compiler-rt with -nodefaultlibs.

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D51645



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51986: Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I don’t think this is the right solution. The build system knows what 
components are in the dylib and should remove them from the list of libraries 
linked individually. You should be able to make Polly behave like an LLVM 
component, then tools don’t need to care if the dylib is used or not.


Repository:
  rC Clang

https://reviews.llvm.org/D51986



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51986: Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

After line 18 in this file you could do something like:

  if(WITH_POLLY)
  list(APPEND LLVM_LINK_COMPONENTS Polly)
  endif()

Then you can get rid of the `target_link_libraries` call.


Repository:
  rC Clang

https://reviews.llvm.org/D51986



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51986: Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I agree that changing the naming scheme probably makes sense, but I also think 
that you can probably make a fairly well contained change to the LLVM CMake 
module that maps components to libnames to special case Polly and make this all 
work.

I’d really like to see us avoid needing special casing for 
`LLVM_LINK_LLVM_DYLIB` as much as possible because it really shouldn’t matter 
to the tool where the LLVM library functionality comes from as long as it is 
there.


Repository:
  rC Clang

https://reviews.llvm.org/D51986



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32604: CMakeLists: Deprecate using llvm-config to find llvm install path

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM!


https://reviews.llvm.org/D32604



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32595: CMakeLists: Don't set LLVM_MAIN_SRC_DIR when building stand-alone clang

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

Is this really something we should be supporting? Building and testing clang 
with potentially out-of-sync lit or gtest seems undesirable to me.

It is worth noting that we do have mods to gtest, so supporting any standard 
distribution of it seems like a not great idea. The general approach in the 
past for standalone builds has been to disable testing unless an LLVM source 
tree is available, and I think that might be the right approach.

Also, this all kinda becomes moot if LLVM moves to a mono-repo (really not 
trying to troll here), because people actively developing LLVM projects will 
basically be forced to have LLVM source trees anyways.

Thoughts?


https://reviews.llvm.org/D32595



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32817: [CMake] Build runtimes for Fuchsia targets

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

This all looks great. I'm really excited to have such a complete toolchain 
build example in-tree. Thank you!


Repository:
  rL LLVM

https://reviews.llvm.org/D32817



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32595: CMakeLists: Don't set LLVM_MAIN_SRC_DIR when building stand-alone clang

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

Ah. I see what you're saying. I was thinking of the mismatch in a different 
way, but your solution makes sense. LGTM.


https://reviews.llvm.org/D32595



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41660: [cmake] Add new linux toolchain file

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

You should split the CMake cache file you created into two files, (1) a CMake 
Cache to manage the build configuration and (2) a tool chain file for targeting 
Linux. As @semeenai pointed out we absolutly want the behavior of the toolchain 
file being loaded multiple times. That is the correct way this build should 
work.

For bootstrap builds where you want the stage1 to run on your build host, you 
should be able to set `BOOTSTRAP_CMAKE_TOOLCHAIN_FILE` in the first stage 
build, to signal to the first stage build that the second stage will be 
cross-compiled, and we can customize the multi-stage dependencies correctly 
based on that. That avoids the need for the `ADDITIONAL_CLANG_BOOTSTRAP_DEPS` 
variable, which feels a bit hacky to me.


Repository:
  rC Clang

https://reviews.llvm.org/D41660



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41706: [CMake] Install resource files into a share/ directory

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a reviewer: kubamracek.
beanz added a subscriber: kubamracek.
beanz added a comment.

For reference this relates to https://reviews.llvm.org/D41673. Looking in 
@kubamracek.


Repository:
  rC Clang

https://reviews.llvm.org/D41706



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41807: [cmake] Delete redundant install command for clang-refactor.

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D41807



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47355: [CMake] Allow specifying extra dependencies of bootstrap stage

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

I prefer `DEPS` to `COMPONENTS` because I've tried to explicitly use the term 
components to mean targets that have paired build and install targets. 
Otherwise looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D47355



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60926: [CMake] Replace the sanitizer support in runtimes build with multilib

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

I really like this. Using `+` to add variants seems much widely useful.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60926/new/

https://reviews.llvm.org/D60926



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57521: [CMake] External compiler-rt-configure requires LLVMTestingSupport when including tests

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a reviewer: phosek.
beanz added a subscriber: phosek.
beanz added a comment.
Herald added a project: clang.

@phosek, does LLVM’s runtime directory do this correctly?

At some point we should try and deprecate and remove this option in favor of 
the LLVM directory because the LLVM approach is much more complete and robust.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57521/new/

https://reviews.llvm.org/D57521



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35133: [cmake] Use new GetSVN.cmake SOURCE_DIRS parameter

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

Since the dependent patch landed, this LGTM too.


https://reviews.llvm.org/D35133



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62693: Support codesigning bundles and forcing

[Chris Bieneman via Phabricator via cfe-commits]

beanz created this revision.
beanz added reviewers: jkorous, bogner.
Herald added subscribers: kadircet, arphaman, dexonsmith, ilya-biryukov, mgorny.
Herald added projects: clang, LLVM.

Clangd's framework is assembled by copying binaries from the lib and bin 
directories into a bundle shape. This results in an invalid bundle code 
signature because the signature only applies to the binaries not the resources.

This patch adds two new options to `llvm_codesign` to enable re-signing the 
library and XPC service as bundles.

The `BUNDLE_PATH` option allow specifying an explicit path to codesign, which 
enables signing bundles which aren't generated using CMake's `FRAMEWORK` or 
`BUNDLE` target properties.

The `FORCE` option allows re-signing binaries that have already been signed. 
This is required for how clangd exposes the clangd library and tools as both 
XPC and non-XPC services using the same binary.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62693

Files:
  clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
  llvm/cmake/modules/AddLLVM.cmake


Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1659,9 +1659,9 @@
   endif()
 endfunction()
 
-# Usage: llvm_codesign(name [ENTITLEMENTS file])
+# Usage: llvm_codesign(name [FORCE] [ENTITLEMENTS file] [BUNDLE_PATH path])
 function(llvm_codesign name)
-  cmake_parse_arguments(ARG "" "ENTITLEMENTS" "" ${ARGN})
+  cmake_parse_arguments(ARG "FORCE" "ENTITLEMENTS;BUNDLE_PATH" "" ${ARGN})
 
   if(NOT LLVM_CODESIGNING_IDENTITY)
 return()
@@ -1691,12 +1691,20 @@
   set(pass_entitlements --entitlements ${ARG_ENTITLEMENTS})
 endif()
 
+if (NOT ARG_BUNDLE_PATH)
+  set(ARG_BUNDLE_PATH $)
+endif()
+
+if(ARG_FORCE)
+  set(force_flag "-f")
+endif()
+
 add_custom_command(
   TARGET ${name} POST_BUILD
   COMMAND ${CMAKE_COMMAND} -E
   env CODESIGN_ALLOCATE=${CMAKE_CODESIGN_ALLOCATE}
   ${CMAKE_CODESIGN} -s ${LLVM_CODESIGNING_IDENTITY}
-  ${pass_entitlements} $
+  ${pass_entitlements} ${force_flag} ${ARG_BUNDLE_PATH}
 )
   endif()
 endfunction()
Index: clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
===
--- clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
+++ clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
@@ -70,4 +70,9 @@
 ${target}
 ${CLANGD_FRAMEWORK_LOCATION}
   )
+
+  # clangd is already signed as a standalone executable, so it must be forced.
+  llvm_codesign(ClangdXPC BUNDLE_PATH 
"${CLANGD_FRAMEWORK_OUT_LOCATION}/XPCServices/${CLANGD_XPC_SERVICE_NAME}.xpc/" 
FORCE)
+  # ClangdXPC library is already signed as a standalone library, so it must be 
forced.
+  llvm_codesign(ClangdXPC BUNDLE_PATH "${CLANGD_FRAMEWORK_LOCATION}" FORCE)
 endmacro(create_clangd_xpc_framework)


Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1659,9 +1659,9 @@
   endif()
 endfunction()
 
-# Usage: llvm_codesign(name [ENTITLEMENTS file])
+# Usage: llvm_codesign(name [FORCE] [ENTITLEMENTS file] [BUNDLE_PATH path])
 function(llvm_codesign name)
-  cmake_parse_arguments(ARG "" "ENTITLEMENTS" "" ${ARGN})
+  cmake_parse_arguments(ARG "FORCE" "ENTITLEMENTS;BUNDLE_PATH" "" ${ARGN})
 
   if(NOT LLVM_CODESIGNING_IDENTITY)
 return()
@@ -1691,12 +1691,20 @@
   set(pass_entitlements --entitlements ${ARG_ENTITLEMENTS})
 endif()
 
+if (NOT ARG_BUNDLE_PATH)
+  set(ARG_BUNDLE_PATH $)
+endif()
+
+if(ARG_FORCE)
+  set(force_flag "-f")
+endif()
+
 add_custom_command(
   TARGET ${name} POST_BUILD
   COMMAND ${CMAKE_COMMAND} -E
   env CODESIGN_ALLOCATE=${CMAKE_CODESIGN_ALLOCATE}
   ${CMAKE_CODESIGN} -s ${LLVM_CODESIGNING_IDENTITY}
-  ${pass_entitlements} $
+  ${pass_entitlements} ${force_flag} ${ARG_BUNDLE_PATH}
 )
   endif()
 endfunction()
Index: clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
===
--- clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
+++ clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
@@ -70,4 +70,9 @@
 ${target}
 ${CLANGD_FRAMEWORK_LOCATION}
   )
+
+  # clangd is already signed as a standalone executable, so it must be forced.
+  llvm_codesign(ClangdXPC BUNDLE_PATH "${CLANGD_FRAMEWORK_OUT_LOCATION}/XPCServices/${CLANGD_XPC_SERVICE_NAME}.xpc/" FORCE)
+  # ClangdXPC library is already signed as a standalone library, so it must be forced.
+  llvm_codesign(ClangdXPC BUNDLE_PATH "${CLANGD_FRAMEWORK_LOC

[PATCH] D62279: Use LTO capable linker

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

This is good to land.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62279/new/

https://reviews.llvm.org/D62279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I have a thought for how we can make the CMake interface for this better. 
Instead of `LLVM_EXTENSIONS` being a global property if it were a pre-defined 
list like `LLVM_ALL_PROJECTS`, then the `LLVM_LINK__INTO_TOOLS` variables 
can be generated by LLVM's configuration instead of relying on Polly's to run. 
If you do that `LLVM_LINK_POLLY_INTO_TOOLS` could be set to `On` even if Polly 
isn't enabled to build, but that actually will be fine if you change the calls 
to `target_link_libraries` in the tools to use a generator expression something 
like: `$<$,Polly>`.

These changes should eliminate the order-dependence on the tools and allow the 
first configuration run to be accurate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61446/new/

https://reviews.llvm.org/D61446



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: llvm/cmake/modules/AddLLVM.cmake:820
+
+set(LLVM_ALL_EXTENSION_TARGETS clang;bugpoint;opt)
+foreach(tool ${LLVM_ALL_EXTENSION_TARGETS})

I don't think we should hard code this list. It would be much better if we add 
an option to `llvm_add_executable` so that extension targets denote themselves.



Comment at: llvm/cmake/modules/AddLLVM.cmake:822
+foreach(tool ${LLVM_ALL_EXTENSION_TARGETS})
+if(TARGET ${tool})
+set_property(TARGET ${tool} APPEND PROPERTY 
LLVM_COMPILER_EXTENSIONS ${llvm_extension_project})

`if(TARGET ...)` is order dependent. That's why you need to change 
tools/CMakeLists.txt, which you won't need to do if you change this to work how 
I suggested in my earlier comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61446/new/

https://reviews.llvm.org/D61446



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: llvm/cmake/modules/AddLLVM.cmake:812
+#   llvm::PassPluginLibraryInfo ${entry_point}();
+add_custom_target(LLVM_PLUGINS)  # target used to hold global properties 
referencable from generator-expression
+function(register_llvm_extension llvm_extension entry_point)

Change this to `llvm-plugins` to match our convention and wrap it in `if (NOT 
TARGET...)` so it doesn't error if AddLLVM is included twice.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61446/new/

https://reviews.llvm.org/D61446



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64580: cmake: Add INSTALL_WITH_TOOLCHAIN option to add_*_library macros

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64580/new/

https://reviews.llvm.org/D64580



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64579: [clang-shlib] Fix clang-shlib for PRIVATE dependencies

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.

Yea, this makes sense even if it is a bit sad. We do use the `--whole-archive` 
approach for `libLLVM`, and it works. I was hoping to avoid it here in part to 
free up the ability to link `libclang_shared` before or in parallel to 
archiving the `libclang*.a` archives.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64579/new/

https://reviews.llvm.org/D64579



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64582: cmake: Fix install of libclang_shared.so when LLVM_INSTALL_TOOLCHAIN_ONLY=ON

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64582/new/

https://reviews.llvm.org/D64582



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64278: Rename libclang_shared to libclang-cpp

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

This is fine with me. I have no real attachment to the name.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64278/new/

https://reviews.llvm.org/D64278



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65270: [CMake] Fix source path generation for install in multi-config (MSBuild)

[Chris Bieneman via Phabricator via cfe-commits]

beanz requested changes to this revision.
beanz added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Headers/CMakeLists.txt:190
 
+if(CMAKE_CONFIGURATION_TYPES)
+  string(REPLACE "${CMAKE_CFG_INTDIR}" "$" output_dir "${output_dir}")

I think this will break Xcode


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65270/new/

https://reviews.llvm.org/D65270



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66068: cmake: Make building clang-shlib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I generally am not a fan of adding more and more options. As long as you're not 
linking the library it won't be generated by any of the check targets. With the 
llvm dylib we've had many issues over the years where changes to LLVM break 
building the dylib and many developers don't build it, so we have to wait for a 
bot to catch it. Making the clang dylib build as part of `all` in every 
possible build configuration is a recognition that this is something we ship 
and we should ensure works.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66068/new/

https://reviews.llvm.org/D66068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66068: cmake: Make building clang-shlib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I think you and I disagree here. General developer workflows don't need to 
include building `all` for every minor change. In my normal workflow I just 
re-run `check-llvm` or `check-clang` over and over again, only building the 
`all` target before I post a patch. With that workflow I only build the library 
once per-patch to ensure that it builds. Which is exactly the goal of not 
having it be included.

If you *really* *really* can't be bothered to ensure that the things we ship 
actually build with your change you can always use the (undocumented, and 
hidden) `CLANG_TOOL_CLANG_SHLIB_BUILD=Off` option. I really don't see a reason 
to add a user-facing option that we don't want people using.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66068/new/

https://reviews.llvm.org/D66068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66068: cmake: Make building clang-shlib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D66068#1626266 , @jvesely wrote:

> That's your workflow. I need to run 'make install' because the modifications 
> are used by an external project. If there's an option to skip shlib during 
> 'make install' I'd be happy to use it.


This is a really odd workflow, and not at all how our project is designed to be 
used. We have CMake exports explicitly to allow building against a build 
directory instead of needing to install.

> I'm pretty sure more people would appreciate not installing large duplicate 
> libraries, but if a hidden option will do, I'll update the patch.

Running `make install` is massively contrary to this stated issue. Our 
`install` target generally installs pretty much everything, which is likely 
duplicating libraries. If you can't change your workflow to not rely on running 
`install`, I'd suggest you use the `LLVM_DISTRIBUTION_COMPONENTS` option 
(http://llvm.org/docs/BuildingADistribution.html), to hand tailor what pieces 
of LLVM you actually care about. That will give you the most optimal build 
cycle.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66068/new/

https://reviews.llvm.org/D66068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66068: cmake: Make building clang-shlib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I want to dissect this a bit.

In D66068#1627451 , @E5ten wrote:

> I am in favour of adding a user-facing option to disable generating this 
> duplicate library for users that don't need it


Why do you call this duplicate? It is unique. There is no other library in the 
clang build that serves the role of this library.

> there should be an option to disable linking a library that takes a long time 
> to link and isn't necessary for a lot of users.

I think this is nuanced. When you say "takes a long time to link", I'm curious 
why you say that. For me it takes 45s to link on my laptop in a Linux VM using 
LLD in a build configuration that also includes all our backends (which is 
kinda a worst-case scenario), and 10s to link if I only include X86. That 
doesn't seem like a super long time, and it doesn't rely on any billion-dollar 
compute farms.

While it does slow down full-build times (slightly), I think the benefit is 
less broken bots which benefits the community as a whole.

Going back to @jvesely's original email, I'm not sure why this adds minutes to 
your build time. I'd be curious if there are other low-hanging fruit that would 
improve your productivity without the community cost of adding new build 
configurations that disable building and testing things that we actually ship.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66068/new/

https://reviews.llvm.org/D66068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66068: cmake: Make building clang-shlib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a reviewer: compnerd.
beanz added a comment.

In D66068#1628617 , @jvesely wrote:

> It duplicates functionality provided by separate/component libraries.


The component libraries can't be used the way this can, and this is intended as 
an installed target, whereas the clang component libraries aren't. These are 
distinct, calling it duplicate is inaccurate and not helpful to the 
conversation. It may be that you don't need this library, or want it, but that 
is different.

> Why can't this be an option the same way I can pick when building llvm?

I'm going to come back to this below.

> Is this a debug build?

It was RelWithDebInfo, so that probably makes a big difference since it 
significantly reduced the amount of debug info being linked.

> do you have any numbers to support that claim?

Sadly we don't keep logs of 5 years worth of build breakages. Since I added the 
LLVM shared library build, we've had a large number of breakages because people 
don't build it, many of the breakages at configuration time.

One of the big problems we have today with the LLVM build system is that the 
number of different configurations is simply massive. I certainly don't have a 
full understanding of all the configuration options people are using and how 
they are using them, and I've spent more time working on the LLVM build system 
than most other contributors. The variety of different configurations which 
conflict or overlap with each other has resulted in the build system being hard 
to maintain and modify.

If we continue to allow each engineer to add options to the build system to 
make their workflow streamlined we will continue adding options until our CMake 
becomes completely unmaintainable. For that reason some of us have been talking 
about a new approach to maintaining the build system. The clang-shlib change 
was one of the first examples of the new approach in action. The general idea 
of the new approach is to not add new configurations. Instead we want to 
encourage people to use existing mechanisms in the build system to improve 
their workflow and iteration times, and I believe we have everything you need 
already.

This does pre-suppose that engineers are willing to alter their workflows 
slightly to rely on different CMake options and build targets. I don't think 
that is an unreasonable position to come from, and frankly I think the inverse 
(that LLVM should adapt to each individual engineer's workflow) is unreasonable.

In your workflow, rather than building `make install`, if you use the CMake 
`LLVM_DISTRIBUTION_COMPONENTS` option to specify which parts of LLVM & Clang 
you need, you can run `make install-distribution` (similarly rather than 
`make`, `make distribution`). This gives you tighter control than any other 
mechanism over what you build and install, and is the preferred way to do what 
you are trying to do (not build some parts of a project).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66068/new/

https://reviews.llvm.org/D66068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63503: cmake: Add CLANG_LINK_CLANG_DYLIB option

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

One comment inline. Otherwise LGTM.




Comment at: clang/CMakeLists.txt:327
+set(CLANG_LINK_CLANG_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL
+"Link tools against libclang_shared.so")
+

We should generate a config error if `LLVM_LINK_LLVM_DYLIB=Off` and 
`CLANG_LINK_CLANG_DYLIB=On`, because that will cause some odd errors.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63503/new/

https://reviews.llvm.org/D63503



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61804: Support building shared and static libraries simultaneously

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I question the general utility of this patch, and I don't really think this 
does anything we want the build system doing.

If you specify `STATIC` and `SHARED` to `llvm_add_library` it creates the 
shared library target as the one that everything links against, which is 
exactly what you don't want. There are two primary reasons why 
`BUILD_SHARED_LIBS` is not recommended for non-development builds both have to 
do with how shared object linking works:

(1) Is that clang when linked against shared libraries is *way* slower (I think 
last I measured was over 10%). This is caused by the need for resolving linkage 
at process launch every time a process is launched.

(2) Is that LLVM relies on a strange mess of static initializers used for 
static registration. The most problematic one is cl::opt. When you link a 
static archive only the object files that contain unresolved symbols get pulled 
in, when you link against a shared object, all of it gets pulled in. Because 
static archives selectively pull in object files (and their static 
initializers) we have had issues in the past where the static build of LLVM 
works, but the shared build produces runtime errors from cl::opt.

I'm curious to understand the problem you are trying to solve. From other posts 
on the list it really sounds to me like you want a variant of libClang that 
exports the entire Clang API surface rather than just the C API. I think there 
are better ways to accomplish that.

Exposing Clang's libraries for piecemeal consumption is a much bigger problem 
than just the build system not building libClang*.so. For example, Clang has no 
equivalent of llvm-config, so there is no external representation of the clang 
dependency graph that users can rely on.

This all aside, I also have issues with this implementation described inline 
below.




Comment at: clang/CMakeLists.txt:451
+option(CLANG_ENABLE_SHARED_LIBRARIES "Build libclang* as shared libraries." ON)
+option(CLANG_ENABLE_STATIC_LIBRARIES "Build libclang* as static libraries." ON)
+

These shouldn't both default to `On`, that is a change in behavior that would 
be a build-time regression for anyone not interested in building shared 
libraries. `STATIC` should default `On`, and `SHARED` default `Off`.

Also you need to check that one of the two options is enabled. If both are 
`Off` confusing things will happen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61804/new/

https://reviews.llvm.org/D61804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61804: Support building shared and static libraries simultaneously

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

As an additional note, Arch linux should not be building clang with 
`BUILD_SHARED_LIBS` nor should it be distributing those ,so files. That isn't a 
supported configuration for Clang deployment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61804/new/

https://reviews.llvm.org/D61804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61804: Support building shared and static libraries simultaneously

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D61804#1499275 , @winksaville wrote:

> When you say you don't think the build system should do this, what do you 
> mean?


`llvm_add_library` supports both, because there are places where we consciously 
choose to build both. That's not the problem. What I don't think the build 
system should do, is take an unsupported distribution format (which is 
unsupported for technical reasons), and make it supported without resolving the 
technical issues.

> With the change I've made when both are specified the default cmake entity, 
> ${name}, is the static libraries. When neither are specified the default is 
> static or shared depending on BUILD_SHARED_LIBS which is unchanged.

I missed that you were renaming the targets before calling `llvm_add_library` 
so that isn't an issue, but there are still lots of issues here. Like, if you 
specify your new option to generate clang static libraries but don't specify 
`BUILD_SHARED_LIBS` or `LLVM_USE_LLVM_DYLIB` each of your clang libraries will 
get statically linked against its own copy of LLVM, which not only makes your 
libraries big, it also makes them not work. You can't link more than one copy 
of LLVM into a single process memory space safely because of LLVM's use of 
global data.

> The problem is that some people want to use shared libraries and some want 
> static.

Why? I generally think most people don't understand the trade-offs, and I'm not 
sure we should feed into that.

> I'm trying to allow both to be built and ask the Arch Linux people to 
> distribute both.

Arch Linux is in an unsupported state, and your patch isn't the way forward.

Let's rewind.

Why do "some people like shared libraries"? There are usually two reasons 
people cite for linking shared libraries. One is reduced binary distribution 
size because you're reducing duplication of code. The other, is the ability to 
update libraries without updating tools.

The later, is not an issue here. LLVM and Clang's C++ libraries don't provide 
any API or ABI stability guarantees across releases, so there is no way you're 
going to reasonably update LLVM or Clang libraries without rebuilding the 
applications using them.

The former can be valuable, but it comes at a cost. Dynamic resolution of 
symbols slows down process launch time, and dynamic resolution of C++ link-once 
ODRs are exceptionally slow. If you are building a compiler, chances are you 
don't want to slow down your process launch that much. But let's pretend you 
don't care. Your solution still isn't the right way to do this.

Creating component shared libraries results in duplicating all the C++ 
link-once ODRs across each shared module. That will not only slow down your 
process launch, but bloat the size of your binary distribution.

In D61804#1499276 , @winksaville wrote:

> It looks to me that llvm, libunwind and libcxx support building both, so the 
> goal isn't unprecedented,


This is very much an apple and oranges comparison. For one, LLVM does not 
support generating component dylibs and shared libs. It supports generating 
libLLVM, a single dylib containing all of the LLVM components rolled together. 
libunwind, libcxx, libcxxabi, are runtime libraries that are designed to allow 
static linkage, and they don't link LLVM.

I apologize that I missed your thread on the dev list, because that would have 
been a much better place to have this conversation. Having gone back and read 
it now, it sounds to me like what you really want is a clang equivalent of 
libLLVM. That is a reasonable and viable thing to want. This patch 
(https://reviews.llvm.org/P8147) is a first stab. I'm happy to prepare that for 
landing in Clang if that meets your needs, and that is a viable way forward.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61804/new/

https://reviews.llvm.org/D61804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61804: Support building shared and static libraries simultaneously

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

My change should not have decreased build time from trunk, nor should it have 
reduced the number of build steps. That patch should generate 
lib/libClang_shared.so, which will export the C++ API. libClang is unchanged 
since changing it would break existing clients of the library.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61804/new/

https://reviews.llvm.org/D61804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz created this revision.
beanz added reviewers: tstellar, winksaville.
Herald added a subscriber: mgorny.
Herald added a project: clang.

This patch adds a libClang_shared library on *nix systems which exports the 
entire C++ API. In order to support this on Windows we should really refactor 
llvm-shlib and share code between the two.

This also uses a slightly different method for generating the shared library, 
which I should back-port to llvm-shlib. Instead of linking the static archives 
and passing linker flags to force loading the whole libraries, this patch 
creates object libraries for every library (which has no cost in the build 
system), and link the object libraries.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61909

Files:
  clang/cmake/modules/AddClang.cmake
  clang/tools/CMakeLists.txt
  clang/tools/clang-shlib/CMakeLists.txt
  clang/tools/clang-shlib/clang-shlib.cpp


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(Clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} 
${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(Clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61804: Support building shared and static libraries simultaneously

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@winksaville that was my bad. I left off the new files in the commit because I 
forgot `git-diff` doesn't grab untracked files...

I've uploaded the patch as a D61909 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61804/new/

https://reviews.llvm.org/D61804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz updated this revision to Diff 199508.
beanz added a comment.

Changed to lowercase 'c' to match other clang libraries.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909

Files:
  clang/cmake/modules/AddClang.cmake
  clang/tools/CMakeLists.txt
  clang/tools/clang-shlib/CMakeLists.txt
  clang/tools/clang-shlib/clang-shlib.cpp


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} 
${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@winksaville whether or not PIC is required for shared libraries varies by 
platform. These days LLVM defaults to -fPIC, and I'm not sure we actually 
support running LLVM without -fPIC on systems that require shared libraries to 
be PIC.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D61909#1502446 , @winksaville wrote:

> Questions:
>
> - Should we only build `libclang_shared.so` if `LLVM_BUILD_LLVM_DYLIB` is ON?


`LLVM_BUILD_LLVM_DYLIB` is actually not the important option to think about 
because it has no impact on this, rather `LLVM_LINK_LLVM_DYLIB`. That really 
depends on what tradeoffs you want. With that option off LLVM will be linked 
statically into the Clang library, which in the common case is fine because you 
can resolve all the LLVM & Clang APIs against it. That will be faster to load, 
but a larger binary distribution. With the option on, libclang_shared will link 
libLLVM, which will result in slower loading times, but smaller distributions. 
Both are workable situations.

> - Should we use link clang-9 to libclang_shared.so when 
> `LLVM_LINK_LLVM_DYLIB` is ON?

No. If we want to support that it should be through its own option which we can 
add in a subsequent patch.

> - Or maybe we should define `BUILD_CLANG_DYLIB` and `LINK_CLANG_DYLIB` or ... 
> ?

I'm not sure there is value in a `BUILD_CLANG_DYLIB` option (and frankly 
`LLVM_BUILD_LLVM_DYLIB` should probably go too). Between when I added the 
support for building libLLVM and now I've shifted my mindset about build system 
functionality. Optimizing the build time of the `all` target seems way less 
worthwhile to me than reducing complication in the build system. Most 
developers iterate by running `check*` targets over and over again rather than 
building `all`, and as long as the dependencies are properly set up adding 
additional targets has little impact on iteration time or perceived build time.

A `CLANG_LINK_CLANG_DYLIB` option probably does make sense to add in order to 
support smaller binary distributions, although I am concerned about 
communicating the impact of that and the `LLVM_LINK_LLVM_DYLIB` options. In the 
past I've seen people do compile-time performance comparisons between GCC and 
Clang using the `BUILD_SHARED_LIBS` option when building Clang, and it leads to 
really misleading results. I would be very frustrated to see a pile of blog 
posts saying that Clang is getting slower just because people are deciding to 
make clang dynamically link its components.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D61909#1503433 , @winksaville wrote:

> IMHO "`BUILD_CLANG_DYLIB`" is needed. As you have it now libclang_shared.so 
> is always builds on UNIX systems, which I believe means that all linux 
> distros would have both increasing their sizes.


Distributions shouldn't be installing `all` unless they really want everything 
and the kitchen sink. We have the `LLVM_DISTRIBUTION_COMPONENTS` so that 
distributions can decide which pieces of the LLVM & Clang distributions they 
want to install. That is the cleanest mechanism for curating an install.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D61909#1503642 , @winksaville wrote:

> Sorry, maybe I didn't make myself clear.


I understood you fine. I don't think you understand the guidance for building 
distributions of LLVM.

Distributions only get libclang_shared if they run the `install` target which 
installs all of LLVM & Clang. The point of `LLVM_DISTRIBUTION_COMPONENTS` is to 
allow people constructing distributions to choose which pieces they want to 
install without introducing a whole lot of overhead in the build system. The 
old method of passing dozens of flags to enable and disable individual pieces 
of the build resulted in a combinatoric explosion in build settings which are 
confusing and ill understood. The new method is one setting that is much 
cleaner to use.

Anyone constructing a distribution should be specifying 
`LLVM_DISTRIBUTION_COMPONENTS` and running the `install-distribution` target.

Given that your comment:

> What I meant was that this change currently uses `if(UNIX)` to generate 
> `libclang_shared.so`, which means "all linux distors" will have both 
> `libclang*.a` and `libclang_shared.so`.

Is only true if the distribution is choosing to run the `install` target, which 
doesn't grant control over all the individual pieces of LLVM & Clang to 
install, and is not the recommended way to construct a distribution.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

There is a simpler example distribution configuration, but sadly there isn't 
documentation. That is something I can fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I should note, this and many more aspects of the build system were topics I 
discussed in a talk at the 2016 LLVM Dev meeting when we switched off autoconf:
https://www.youtube.com/watch?v=StF77Cx7pz8


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D61909#1505083 , @winksaville wrote:

> Please add documentation if you want people to use it :)


I won't disagree that it should be documented, and I already started. I do want 
to point out though that your motivation starting this was Arch linux where 
they are doing exactly what our documentation says not to do. So...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I will land this patch as-is today. I'll try and get a doc patch out for review 
today or tomorrow. @winksaville, I'm going to add a section to the document 
about building LLVM as a shared library for inclusion with tool distributions.

That still shouldn't be done with `BUILD_SHARED_LIBS`, we do have mechanisms in 
LLVM to allow people to control which parts of LLVM get included in libLLVM, 
and we should add similar mechanisms to this new functionality if needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I would leave it out of any distribution (at least for now).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@sylvestre.ledru, I’ve added you on D62040 , 
which is an attempt to document best practices for generating releases.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@E5ten, I see what is going on. Give me 30 minutes and I’ll have a fix pushed.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@E5ten, I just pushed r361271, which should resolve your issue.

I have a better longer-term fix in mind that I'll put up for review this week.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62215: Fixes to distribution example for X86_64 Arch Linux

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

Adding "libcxxabi" to `LLVM_ENABLE_RUNTIMES` is fine, but the other changes to 
the DistributionExample are only needed because you chose to use gold, which is 
a configuration-specific decision that is not representative of how most people 
will build, therefore it shouldn't be in the example.

I'm also not sure the `PLUGIN_TOOL` line is correct. That seems to assume that 
you either set `LLVM_ENABLE_LLVM_DYLIB`, or have libLLVM pre-installed, which I 
don't think most people do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62215/new/

https://reviews.llvm.org/D62215



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D61909#1510955 , @E5ten wrote:

> @beanz Well I took libclang_shared as effectively an equivalent to the 
> libLLVM.so that's created with that dylib option, and when BUILD_SHARED_LIBS 
> is enabled that library is not created, in fact the option to create that 
> library conflicts with BUILD_SHARED_LIBS.


That conflict is because of how I implemented libLLVM, and is something I'm 
looking to change. I'm hoping to put up a patch this week that will update how 
we generate libLLVM to be more like how libclang_shared is built.

> Also when the libs are built shared and also as object libraries that are 
> linked into libclang_shared would that not cause the same libraries to be 
> linked into executables twice, once through the shared libraries and once 
> through libclang_shared?

Object libraries are just collections of object files without linkage 
dependencies. The whole point of implementing libclang_shared in terms of 
object files is to allow constructing libclang_shared even if 
`BUILD_SHARED_LIBS` is enabled, and without using extra linker flags to force 
the linker to treat all the clang static archives as collections of object 
files (we do that in libLLVM using --all_load and --whole-archive flags).

It shouldn't result in duplicating the linkage because libclang contains all 
the code and shouldn't have any external resolutions against the libclang* 
component libraries.

One big reason that I don't want to disable generating libclang_shared (or 
libLLVM) if `BUILD_SHARED_LIBS=On` is because libclang_shared is intended to be 
a shippable binary unlike the `BUILD_SHARED_LIBS` libraries, so it really 
should be enabled in every possible build configuration that we can make it 
work.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D61909#1510975 , @E5ten wrote:

> @beanz But if libclang_shared is intended to be a shippable binary and 
> BUILD_SHARED_LIBS is only intended to be an option used in developer builds, 
> and libclang_shared while not causing conflicts (thanks for the info on how 
> that works by the way) would be redundant in a local developer build one 
> would see BUILD_SHARED_LIBS enabled in wouldn't it? And also to me it doesn't 
> really make sense to enable the intended shippable binary in a build that is 
> specifically enabling a developer option, and so is obviously not intended 
> for shipment (I get that in the arch case it is intended for shipment but 
> that case is them using an option they shouldn't not the option being one 
> that should be used when the built products are intended for redistribution).


Best way to make sure a shippable binary builds and works is for developers to 
be building it. In general we should avoid developers doing things that aren't 
representative of what we expect to be in the hands of users. 
`BUILD_SHARED_LIBS` is an unusual exception to this that I wish would go away. 
The reason we keep it around is because of how significant of a workflow 
improvement it provides.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61909/new/

https://reviews.llvm.org/D61909



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62215: Fixes to distribution example for X86_64 Arch Linux

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D62215#1512543 , @winksaville wrote:

> With this error I guessed that I needed to build `LLVMgold.so`, but then I 
> also determined
>  I needed to link everything with `ld.gold` to complete the build process, 
> which is why I
>  uploaded this patch.


The issue is actually that your system linker isn't compatible with LLVM/master 
for LTO, and the LLVM build system doesn't know how to setup linux system 
linkers automatically. We do properly configure ld64, and lld on Linux and 
Windows if you configure to tell LLVM to use LLD.

> Anyway, since you don't seem to like using `ld.gold`

It isn't that I don't "like" gold, it is that gold isn't installed by default 
on most systems, and I'm pretty sure it doesn't exist for Darwin or Win32.

> So I've found a couple hacks that partially work:
> 
> 1. Build `LLVMgold.so` and use `ld.gold`

This is a viable configuration, but cannot be part of the example because it 
isn't portable.

> 2. Don't use LTO

The real underlying issue is that LLVM's build system isn't producing 
reasonable errors for when the system linker doesn't support LTO, and it 
doesn't seem to know how to configure a gold-based LTO bootstrap.

> CMake Warning at 
> /home/wink/prgs/llvm/llvm-project/libcxx/cmake/Modules/CheckLibcxxAtomic.cmake:51
>  (message):
> 
>   Host compiler must support std::atomic!

This is caused by the issue that I'm trying to address in D62155 
, and will impact the correctness of the 
runtime libraries you build.

> ninja: error: 
> '/home/wink/prgs/llvm/llvm-project/build-dist-a/tools/clang/stage2-bins/lib/libgtest.a',
>  needed by 
> 'compiler-rt/lib/asan/tests/ASAN_INST_TEST_OBJECTS.gtest-all.cc.x86_64-calls.o',
>  missing and no known rule to make it

This looks like a missing build dependency between the runtime `check` target 
and gtest, which should be easy enough to fix. I'll put up a patch.

I actually really appreciate you trying this out and reporting back with such 
detailed feedback on your experience. Thank you for your patience, and I'm 
sorry if I'm being a bit of a pain.

I think the correct fix for this is to use lld on non-Darwin platforms, and 
that can be done by adding the following code to DistributionExample.cmake:

  if (NOT APPLE)
set(BOOTSTRAP_LLVM_ENABLE_LLD On CACHE BOOL "")
  endif()

This will force using LLD on non-Darwin builds. This combined with adding 
"libcxxabi" to `LLVM_ENABLE_RUNTIMES` should actually get your build working 
without any further changes (none of the gold stuff should be needed).

Additionally we should probably consider adding better error checking to the 
bootstrap configurations to error out during configuration on some of these 
problems. For example we should be able to verify that when 
`BOOTSTRAP_LLVM_ENABLE_LTO=On` the following things should be true.

- If your host is Windows, in stage 1 you must be building `lld`, and in stage 
2 you must be linking with it
- If your host is Linux, in stage 1 you must be building `lld`, and in stage 2 
you must be linking with it -or- you must build the Gold plugin in stage1 and 
link with `ld.gold` in stage 2.
- If your host is Darwin, you must not be using `lld`




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62215/new/

https://reviews.llvm.org/D62215



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62215: Fixes to distribution example for X86_64 Arch Linux

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

D62269  should fix the missing dependency on 
`gtest`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62215/new/

https://reviews.llvm.org/D62215



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62215: Fixes to distribution example for X86_64 Arch Linux

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D62215#1512849 , @winksaville wrote:

> IIRC, on linux if LTO is ON I had to use `ld.gold` for both stage 1 and 2, 
> although that maybe
>  just the way it ended up and it wasn't necessary. But I'll test whatever you 
> come up with.


I will try and spin up a VM today to try and test this on Linux using GNU ld 
for the stage 1. I don't usually do much work on Linux, but when I do I always 
setup `lld` as my system linker because it is way more efficient both in speed 
and memory usage. I don't think I've ever managed to get GNU ld to link a 
non-debug build of LLVM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62215/new/

https://reviews.llvm.org/D62215



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62279: Use LTO capable linker

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D62279#1514046 , @winksaville wrote:

> I've add gtest_main and gtest to  CLANG_BOOTSTRAP_TARGETS as a guess, because 
> that's where check-all is defined:
>  ...
>  My guess is likely wrong, what do you advise?


Are you still having that issue after rL361436 
? That should have resolved that problem. 
The issue isn't that gtest is missing from the bootstrap, but rather that it 
was missing from the dependencies for the runtime libraries.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62279/new/

https://reviews.llvm.org/D62279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62279: Use LTO capable linker

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D62279#1514467 , @winksaville wrote:

> Even with both gtest patches it still isn't working, `ninja check-all` is 
> failing as before :(


I wouldn't expect your gtest patch to resolve the issue. The 
`CLANG_BOOTSTRAP_TARGETS` variable is just a list of targets in the stage 2 
build to generate wrapper targets in the stage 1 build for. It allows you to 
run `ninja stage2-${target}` instead of having to go into the `stage2-bins` 
directory and run `ninja ${target}`.

> Is there a way to "force" gtest/gtest_main to get included just so I can see 
> if there is any other `check-all` failures?

It isn't really about including the target. The issue is that there is a 
missing dependency. We need to make sure the `gtest` and `gtest_main` targets 
are built before the `check-runtimes` target (which is part of `check-all`.

> Other suggestions?

My Linux test build is chugging along. It is going to take a while since it is 
in a VM, but once it completes I can look and see if I can work out why 
rL361436  didn't work to add the dependency.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62279/new/

https://reviews.llvm.org/D62279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62279: Use LTO capable linker

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@winksaville I've figured out how to resolve the `gtest` issue, but 
unfortunately that isn't good enough to get the `check-runtimes` target 
working. A change went in back in February (rL353601 
), which breaks running compiler-rt's tests 
when building a distribution in non-trivial ways, which will unfortunately be 
difficult to resolve.

I will land the `gtest` fix sometime today, and I'm going to start working 
toward getting the larger issue resolved, although that is going to take some 
time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62279/new/

https://reviews.llvm.org/D62279



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62343: [cmake] Remove old unused version of FindZ3.cmake from clang [NFC]

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62343/new/

https://reviews.llvm.org/D62343



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67321: Respect CLANG_LINK_CLANG_DYLIB=ON in libclang and c-index-test

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@aaronpuchert sounds like a reasonable approach


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67321/new/

https://reviews.llvm.org/D67321



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67585: [clang] [cmake] Make building dylib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz requested changes to this revision.
beanz added a comment.
This revision now requires changes to proceed.

Please no. You don’t need to build the library. ‘check-clang’ doesn’t depend on 
it, so it should not impact your build and test cycles. We want it included in 
the ‘all’ target for every possible configuration so that it gets tested and 
not broken as frequently as the LLVM dylib target has in the past.

If you don’t want to link it you can avoid linking it by running the ‘check’ 
targets instead of building ‘all’. In general, I believe we need to reduce the 
number of options for configuring the LLVM build. The build system has reached 
a point where the number of possible configurations is so massive that the 
build system is fragile and becoming more challenging to modify.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67585/new/

https://reviews.llvm.org/D67585



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67585: [clang] [cmake] Make building dylib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D67585#1670420 , @mgorny wrote:

> Sure but I want to build-test practically everything else. I wouldn't mind if 
> this didn't basically kill my test system by resource exhaustion


You can use the ‘LLVM_DISTRIBUTION_COMPONENTS’ option to create a build test 
target that includes the things you care to test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67585/new/

https://reviews.llvm.org/D67585



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67585: [clang] [cmake] Make building dylib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a reviewer: compnerd.
beanz added a comment.

In D67585#1670433 , @mgorny wrote:

> This is really much more work than disabling the one component I don't need 
> or care for, especially when I do shared lib build and therefore the 
> additional library is entirely useless.


It is one-time effort to figure out what you "care" about, which makes me 
pretty unsympathetic to the argument that it is much more work. Further, your 
argument for building the `all` target is that you want to build-test things, 
but you have decided this library is "useless" so you don't want to build it. 
How many other things in tree do you deem "useless"?

Using `LLVM_DISTRIBUTION_COMPONENTS` is the best way (and hopefully in the 
future the only supported way) to make an arbitrary choice about what is 
meaningful to you.

I believe the fundamental direction of the build system needs to be different 
from what it has been in the past, and I've had conversations with other 
contributors that agree. My plan is to have a larger discussion at the 
Developer Meeting in October, and formulate a document describing the direction 
and guidelines for the build system. Presently the build system is growing 
along a trajectory that is unsustainable and actively harmful to its 
maintainability. My belief is that the build system should have less options, 
not more. In particular we should avoid options that conflict with other 
options creating invalid build configurations, which this patch does (see 
`CLANG_LINK_CLANG_DYLIB`). I also believe that we should not add options for 
each individual's preferred workflow, instead we need to coalesce around a more 
limited set of supported workflows. Following this direction will reduce the 
maintenance cost of the build system by making it easier and less risky to make 
changes, and it will provide a general improvement to the project by reducing 
the matrix of possible configurations that require distinct testing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67585/new/

https://reviews.llvm.org/D67585



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67585: [clang] [cmake] Make building dylib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D67585#1670491 , @mgorny wrote:

> But in that case, we should aim for consistency, i.e. remove the matching 
> option from LLVM.


Yes! I want to do that too.

After the mono-repo transition is finalized I think we need to invest time in 
cleaning up the LLVM build system and removing a bunch of legacy functionality. 
At the same time I think we should move the system in a new architectural 
direction. This is why I'm targeting to have the discussions at the Developer 
Meeting in October, which corresponds with the mono-repo becoming the source of 
truth.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67585/new/

https://reviews.llvm.org/D67585



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68413: [clang] [cmake] Add distribution install targets for remaining components

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68413/new/

https://reviews.llvm.org/D68413



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68430: Don't use object libraries with Xcode

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

This has the side-effect of making `libclang_cpp` effectively empty when you 
build with Xcode.

What exactly does Xcode do with `OBJECT` libraries?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68430/new/

https://reviews.llvm.org/D68430



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68429: [clang] [cmake] Use add_clang_tool() to install all tools

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

Are these tools intended to be installed? I thought they were developer focused 
tools. I don't think we should make install targets for things that the project 
doesn't want to support publicly.

This also changes the behavior of the `install` target to include these extra 
tools, which will increase the size of installs that many people are using 
today with these tools. At the very least if we go down this direction we need 
to add some option like `LLVM_INSTALL_TOOLCHAIN_ONLY` so that we don't 
radically alter the current `install` behavior.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68429/new/

https://reviews.llvm.org/D68429



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68430: Don't use object libraries with Xcode

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D68430#1693964 , @jordan_rose wrote:

> I'm not quite sure //what// it's doing. The executable targets end up trying 
> to link against the static libraries anyway, which of course haven't been 
> built. It's possible that this is because the LIBTYPE is both STATIC and 
> OBJECT and if it were just OBJECT we might be better off, but I'm not sure if 
> Xcode's IDE features will be happy with a target that doesn't actually 
> produce a library. I can try it if you want, though.


I don't think that is necessary, it sounds like we just need to disable this 
functionality if Xcode is the build tool. That is unfortunate, but Xcode's 
build system is problematic to work with.

> (I did look at CMake's Xcode generator logic for OBJECT targets and it looks 
> completely bonkers to me, as if it's still building the static library but 
> then removing it to make sure it's not depended on or something. Even if it 
> builds cleanly I don't trust it to do dependency analysis correctly.)

Fair. Unfortunately Xcode's build dependencies aren't as expressive as other 
build systems, which causes some unfortunate limitations on what it can do.

> 
> 
>> This has the side-effect of making `libclang_cpp` effectively empty when you 
>> build with Xcode.
> 
> I can remove that target if you want, or have it link the libraries normally.

clang_cpp can't link the libraries "normally" because it has no unresolved 
symbols to force the contents of the libraries to link. I don't like it, but I 
think the best option is to disable clang_cpp under Xcode. You can add `AND 
XCODE` to the `if` on line 2 of clang/tools/clang-shlib/CMakeLists.txt, and 
that should do the trick.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68430/new/

https://reviews.llvm.org/D68430



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a reviewer: bruno.
beanz added a subscriber: bruno.
beanz added a comment.

@dexonsmith is often super busy, so @bruno may be able to weigh in instead.


Repository:
  rC Clang

https://reviews.llvm.org/D45639



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68430: Don't use object libraries with Xcode

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

That's fine. That said, there are things that just can't be done, or don't work 
well, in the Xcode and Visual Studio generators, so we have precedent for 
disabling functionality based on those generators.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68430/new/

https://reviews.llvm.org/D68430



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68429: [clang] [cmake] Use add_clang_tool() to install all tools

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

fair enough


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68429/new/

https://reviews.llvm.org/D68429



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68448: [clang-tools-extra] [cmake] Link against libclang-cpp whenever possible

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68448/new/

https://reviews.llvm.org/D68448



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68448: [clang-tools-extra] [cmake] Link against libclang-cpp whenever possible

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D68448#1694284 , @sylvestre.ledru 
wrote:

> More dynamic, less static usage :)


More dynamic, less static isn't always a good thing. It saves disk space at the 
expense of performance. The option that controls this is 
`CLANG_LINK_CLANG_DYLIB`, which is mentioned in the clang 9.0 release notes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68448/new/

https://reviews.llvm.org/D68448



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80492: Avoid linking libdl unless needed

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

looks reasonable


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80492/new/

https://reviews.llvm.org/D80492



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84565: [Darwin] [Driver] Clang should invoke dsymutil for lto builds -g*

[Chris Bieneman via Phabricator via cfe-commits]

beanz created this revision.
beanz added reviewers: bogner, compnerd, aprantl, arphaman.
Herald added subscribers: dexonsmith, inglorion.
Herald added a reviewer: JDevlieghere.
Herald added a project: clang.

Clang should always add a dsymutil step whenever debug information is
generated and the compiler is acting as the linker driver with
temporary object files.

https://bugs.llvm.org/show_bug.cgi?id=46841


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84565

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/darwin-dsymutil.c


Index: clang/test/Driver/darwin-dsymutil.c
===
--- clang/test/Driver/darwin-dsymutil.c
+++ clang/test/Driver/darwin-dsymutil.c
@@ -47,3 +47,11 @@
 
 // Check that we don't crash when translating arguments for dsymutil.
 // RUN: %clang -m32 -arch x86_64 -g %s -###
+
+// Check that we run dsymutil when -flto is specified and no source is provided
+// RUN: touch %t.o
+// RUN: %clang -target x86_64-apple-darwin10 -ccc-print-bindings \
+// RUN:   -arch x86_64 %t.o -g -flto 2> %t
+// RUN: FileCheck -check-prefix=CHECK-DSYMUTIL-LTO < %t %s
+
+// CHECK-DSYMUTIL-LTO: "/usr/bin/dsymutil" "-o" "a.out.dSYM" "a.out"
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2033,13 +2033,17 @@
 Arg *A = Args.getLastArg(options::OPT_g_Group);
 bool enablesDebugInfo = A && !A->getOption().matches(options::OPT_g0) &&
 !A->getOption().matches(options::OPT_gstabs);
+bool isGeneratingTemporaryObject =
+ContainsCompileOrAssembleAction(Actions.back()) || LTOMode != 
LTOK_None;
 if ((enablesDebugInfo || willEmitRemarks(Args)) &&
-ContainsCompileOrAssembleAction(Actions.back())) {
+isGeneratingTemporaryObject) {
 
   // Add a 'dsymutil' step if necessary, when debug info is enabled and we
-  // have a compile input. We need to run 'dsymutil' ourselves in such 
cases
-  // because the debug info will refer to a temporary object file which
-  // will be removed at the end of the compilation process.
+  // are linking a temporary object. This occurs when we have a compiler
+  // or assmbler input or if LTO is enabled. We need to run 'dsymutil'
+  // ourselves in such cases because the debug info will refer to the
+  // temporary object file which will be removed at the end of the
+  // compilation process.
   if (Act->getType() == types::TY_Image) {
 ActionList Inputs;
 Inputs.push_back(Actions.back());


Index: clang/test/Driver/darwin-dsymutil.c
===
--- clang/test/Driver/darwin-dsymutil.c
+++ clang/test/Driver/darwin-dsymutil.c
@@ -47,3 +47,11 @@
 
 // Check that we don't crash when translating arguments for dsymutil.
 // RUN: %clang -m32 -arch x86_64 -g %s -###
+
+// Check that we run dsymutil when -flto is specified and no source is provided
+// RUN: touch %t.o
+// RUN: %clang -target x86_64-apple-darwin10 -ccc-print-bindings \
+// RUN:   -arch x86_64 %t.o -g -flto 2> %t
+// RUN: FileCheck -check-prefix=CHECK-DSYMUTIL-LTO < %t %s
+
+// CHECK-DSYMUTIL-LTO: "/usr/bin/dsymutil" "-o" "a.out.dSYM" "a.out"
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2033,13 +2033,17 @@
 Arg *A = Args.getLastArg(options::OPT_g_Group);
 bool enablesDebugInfo = A && !A->getOption().matches(options::OPT_g0) &&
 !A->getOption().matches(options::OPT_gstabs);
+bool isGeneratingTemporaryObject =
+ContainsCompileOrAssembleAction(Actions.back()) || LTOMode != LTOK_None;
 if ((enablesDebugInfo || willEmitRemarks(Args)) &&
-ContainsCompileOrAssembleAction(Actions.back())) {
+isGeneratingTemporaryObject) {
 
   // Add a 'dsymutil' step if necessary, when debug info is enabled and we
-  // have a compile input. We need to run 'dsymutil' ourselves in such cases
-  // because the debug info will refer to a temporary object file which
-  // will be removed at the end of the compilation process.
+  // are linking a temporary object. This occurs when we have a compiler
+  // or assmbler input or if LTO is enabled. We need to run 'dsymutil'
+  // ourselves in such cases because the debug info will refer to the
+  // temporary object file which will be removed at the end of the
+  // compilation process.
   if (Act->getType() == types::TY_Image) {
 ActionList Inputs;
 Inputs.push_back(Actions.back());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84565: [Darwin] [Driver] Clang should invoke dsymutil for lto builds -g*

[Chris Bieneman via Phabricator via cfe-commits]

beanz updated this revision to Diff 280641.
beanz added a comment.

Fixing bad test case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84565/new/

https://reviews.llvm.org/D84565

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/darwin-dsymutil.c


Index: clang/test/Driver/darwin-dsymutil.c
===
--- clang/test/Driver/darwin-dsymutil.c
+++ clang/test/Driver/darwin-dsymutil.c
@@ -47,3 +47,11 @@
 
 // Check that we don't crash when translating arguments for dsymutil.
 // RUN: %clang -m32 -arch x86_64 -g %s -###
+
+// Check that we run dsymutil when -flto is specified and no source is provided
+// RUN: touch %t.o
+// RUN: %clang -target x86_64-apple-darwin10 -ccc-print-bindings \
+// RUN:   -arch x86_64 %t.o -g -flto 2> %t
+// RUN: FileCheck -check-prefix=CHECK-DSYMUTIL-LTO < %t %s
+
+// CHECK-DSYMUTIL-LTO: x86_64-apple-darwin10" - "darwin::Dsymutil", inputs: 
["a.out"], output: "a.out.dSYM"
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2033,13 +2033,17 @@
 Arg *A = Args.getLastArg(options::OPT_g_Group);
 bool enablesDebugInfo = A && !A->getOption().matches(options::OPT_g0) &&
 !A->getOption().matches(options::OPT_gstabs);
+bool isGeneratingTemporaryObject =
+ContainsCompileOrAssembleAction(Actions.back()) || LTOMode != 
LTOK_None;
 if ((enablesDebugInfo || willEmitRemarks(Args)) &&
-ContainsCompileOrAssembleAction(Actions.back())) {
+isGeneratingTemporaryObject) {
 
   // Add a 'dsymutil' step if necessary, when debug info is enabled and we
-  // have a compile input. We need to run 'dsymutil' ourselves in such 
cases
-  // because the debug info will refer to a temporary object file which
-  // will be removed at the end of the compilation process.
+  // are linking a temporary object. This occurs when we have a compiler
+  // or assmbler input or if LTO is enabled. We need to run 'dsymutil'
+  // ourselves in such cases because the debug info will refer to the
+  // temporary object file which will be removed at the end of the
+  // compilation process.
   if (Act->getType() == types::TY_Image) {
 ActionList Inputs;
 Inputs.push_back(Actions.back());


Index: clang/test/Driver/darwin-dsymutil.c
===
--- clang/test/Driver/darwin-dsymutil.c
+++ clang/test/Driver/darwin-dsymutil.c
@@ -47,3 +47,11 @@
 
 // Check that we don't crash when translating arguments for dsymutil.
 // RUN: %clang -m32 -arch x86_64 -g %s -###
+
+// Check that we run dsymutil when -flto is specified and no source is provided
+// RUN: touch %t.o
+// RUN: %clang -target x86_64-apple-darwin10 -ccc-print-bindings \
+// RUN:   -arch x86_64 %t.o -g -flto 2> %t
+// RUN: FileCheck -check-prefix=CHECK-DSYMUTIL-LTO < %t %s
+
+// CHECK-DSYMUTIL-LTO: x86_64-apple-darwin10" - "darwin::Dsymutil", inputs: ["a.out"], output: "a.out.dSYM"
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2033,13 +2033,17 @@
 Arg *A = Args.getLastArg(options::OPT_g_Group);
 bool enablesDebugInfo = A && !A->getOption().matches(options::OPT_g0) &&
 !A->getOption().matches(options::OPT_gstabs);
+bool isGeneratingTemporaryObject =
+ContainsCompileOrAssembleAction(Actions.back()) || LTOMode != LTOK_None;
 if ((enablesDebugInfo || willEmitRemarks(Args)) &&
-ContainsCompileOrAssembleAction(Actions.back())) {
+isGeneratingTemporaryObject) {
 
   // Add a 'dsymutil' step if necessary, when debug info is enabled and we
-  // have a compile input. We need to run 'dsymutil' ourselves in such cases
-  // because the debug info will refer to a temporary object file which
-  // will be removed at the end of the compilation process.
+  // are linking a temporary object. This occurs when we have a compiler
+  // or assmbler input or if LTO is enabled. We need to run 'dsymutil'
+  // ourselves in such cases because the debug info will refer to the
+  // temporary object file which will be removed at the end of the
+  // compilation process.
   if (Act->getType() == types::TY_Image) {
 ActionList Inputs;
 Inputs.push_back(Actions.back());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84565: [Darwin] [Driver] Clang should invoke dsymutil for lto builds -g*

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@JDevlieghere you are right, I'm missing the change to how clang determines it 
needs to pass the linker the object file path. Update coming momentarily.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84565/new/

https://reviews.llvm.org/D84565



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84565: [Darwin] [Driver] Clang should invoke dsymutil for lto builds -g*

[Chris Bieneman via Phabricator via cfe-commits]

beanz updated this revision to Diff 280647.
beanz added a comment.

Updated to also pass the linker -object_path_lto


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84565/new/

https://reviews.llvm.org/D84565

Files:
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/test/Driver/darwin-dsymutil.c


Index: clang/test/Driver/darwin-dsymutil.c
===
--- clang/test/Driver/darwin-dsymutil.c
+++ clang/test/Driver/darwin-dsymutil.c
@@ -47,3 +47,13 @@
 
 // Check that we don't crash when translating arguments for dsymutil.
 // RUN: %clang -m32 -arch x86_64 -g %s -###
+
+// Check that clang provides the linker with a temporary object file path and
+// runs dsymutil when -flto is specified and no source is provided
+// RUN: touch %t.o
+// RUN: %clang -target x86_64-apple-darwin10 -arch x86_64 %t.o -g -flto -### \
+// RUN:   2> %t
+// RUN: FileCheck -check-prefix=CHECK-DSYMUTIL-LTO < %t %s
+
+// CHECK-DSYMUTIL-LTO: "/usr/bin/ld" "-demangle" "-object_path_lto"
+// CHECK-DSYMUTIL-LTO: "/usr/bin/dsymutil" "-o" "a.out.dSYM" "a.out"
Index: clang/lib/Driver/ToolChains/Darwin.h
===
--- clang/lib/Driver/ToolChains/Darwin.h
+++ clang/lib/Driver/ToolChains/Darwin.h
@@ -59,7 +59,8 @@
 };
 
 class LLVM_LIBRARY_VISIBILITY Linker : public MachOTool {
-  bool NeedsTempPath(const InputInfoList &Inputs) const;
+  bool NeedsTempPath(const InputInfoList &Inputs,
+ const llvm::opt::ArgList &Args) const;
   void AddLinkArgs(Compilation &C, const llvm::opt::ArgList &Args,
llvm::opt::ArgStringList &CmdArgs,
const InputInfoList &Inputs, unsigned Version[5]) const;
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -167,7 +167,13 @@
 CmdArgs.push_back("-force_cpusubtype_ALL");
 }
 
-bool darwin::Linker::NeedsTempPath(const InputInfoList &Inputs) const {
+bool darwin::Linker::NeedsTempPath(const InputInfoList &Inputs,
+   const ArgList &Args) const {
+  Arg *A = Args.getLastArg(options::OPT_g_Group);
+  if (A && !A->getOption().matches(options::OPT_g0) &&
+  !A->getOption().matches(options::OPT_gstabs))
+return true;
+
   // We only need to generate a temp path for LTO if we aren't compiling object
   // files. When compiling source files, we run 'dsymutil' after linking. We
   // don't run 'dsymutil' when compiling object files.
@@ -222,7 +228,7 @@
options::OPT_fno_application_extension, false))
 CmdArgs.push_back("-application_extension");
 
-  if (D.isUsingLTO() && Version[0] >= 116 && NeedsTempPath(Inputs)) {
+  if (D.isUsingLTO() && Version[0] >= 116 && NeedsTempPath(Inputs, Args)) {
 std::string TmpPathName;
 if (D.getLTOMode() == LTOK_Full) {
   // If we are using full LTO, then automatically create a temporary file
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2033,13 +2033,17 @@
 Arg *A = Args.getLastArg(options::OPT_g_Group);
 bool enablesDebugInfo = A && !A->getOption().matches(options::OPT_g0) &&
 !A->getOption().matches(options::OPT_gstabs);
+bool isGeneratingTemporaryObject =
+ContainsCompileOrAssembleAction(Actions.back()) || isUsingLTO();
 if ((enablesDebugInfo || willEmitRemarks(Args)) &&
-ContainsCompileOrAssembleAction(Actions.back())) {
+isGeneratingTemporaryObject) {
 
   // Add a 'dsymutil' step if necessary, when debug info is enabled and we
-  // have a compile input. We need to run 'dsymutil' ourselves in such 
cases
-  // because the debug info will refer to a temporary object file which
-  // will be removed at the end of the compilation process.
+  // are linking a temporary object. This occurs when we have a compiler
+  // or assmbler input or if LTO is enabled. We need to run 'dsymutil'
+  // ourselves in such cases because the debug info will refer to the
+  // temporary object file which will be removed at the end of the
+  // compilation process.
   if (Act->getType() == types::TY_Image) {
 ActionList Inputs;
 Inputs.push_back(Actions.back());


Index: clang/test/Driver/darwin-dsymutil.c
===
--- clang/test/Driver/darwin-dsymutil.c
+++ clang/test/Driver/darwin-dsymutil.c
@@ -47,3 +47,13 @@
 
 // Check that we don't crash when translating arguments for dsymutil.
 // RUN: %clang -m32 -arch x86_64 -g %s -###
+
+// Check that clang provides the linker with a temporary object file path a

[PATCH] D84565: [Darwin] [Driver] Clang should invoke dsymutil for lto builds -g*

[Chris Bieneman via Phabricator via cfe-commits]

beanz marked an inline comment as done.
beanz added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:2037
+bool isGeneratingTemporaryObject =
+ContainsCompileOrAssembleAction(Actions.back()) || LTOMode != 
LTOK_None;
 if ((enablesDebugInfo || willEmitRemarks(Args)) &&

dsanders wrote:
> This should fix the same issue as the workaround I was using in D84127.
> 
> I'm not entirely convinced that this is all of it though. It seems strange to 
> me that these two scripts produce different build products (aside from 
> ret0.o):
> ```
> $ rm -rf external ret0 ret0.*
> $ echo 'int main() { return 0; }' > ret0.c
> $ use_cflags="--sysroot=$SYSROOT -g"
> $ ../build/bin/clang -o ret0 ret0.c ${=use_cflags}
> $ ls
> ret0  ret0.cret0.dSYM
> ```
> ```
> $ rm -rf external ret0 ret0.*
> $ echo 'int main() { return 0; }' > ret0.c
> $ use_cflags="--sysroot=$SYSROOT -g"
> $ ../build/bin/clang -c -o ret0.o ret0.c ${=use_cflags}
> $ ../build/bin/clang -o ret0 ret0.o ${=use_cflags}
> $ ls
> ret0   ret0.c ret0.o
> ```
> I'd expect them to either consistently extract the dSYM or consistently lipo 
> the slices together including the debug info. I don't have a strong opinion 
> about which is right, it's just the inconsistency that's bothering me
The reason they behave differently is because in the case where clang compiles 
+ links in the same invocation the intermediate object files are deleted. In 
MachO we do not link debug information in ld, instead we do it in dsymutil. 
dsymutil cannot run across a temporary object file that has been deleted, so 
the clang driver runs it for you.

Clang doesn't run it in the case of just linking object files because it takes 
time and isn't strictly required as long as you keep all your object files 
around so that your debugger can find the unlinked debug info in the object 
files. This is why one of the common ways people speed up debug builds with 
mach-o is to skip dSYM generation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84565/new/

https://reviews.llvm.org/D84565



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78033: [cmake] Restrict symbols exported from libclang-cpp

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I think this has some unintended consequences. If your tool wants to use 
libLLVM and libClang you really need libClang to be linked against libLLVM, 
otherwise you're actually just hiding the problem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78033/new/

https://reviews.llvm.org/D78033



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77156: [CMAKE] Plumb include_directories() into tablegen()

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.
Herald added a subscriber: frgossen.

I'm not sure this is actually the right approach. Adding the directory's 
`INCLUDE_DIRECTORIES` option also adds any path added with 
`include_directories`, which includes system paths and other locations that 
shouldn't ever have tablegen files.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77156/new/

https://reviews.llvm.org/D77156



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85247: [Darwin] [Driver] Clang should invoke dsymutil for multiarch builds

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: clang/test/Driver/darwin-dsymutil.c:74
+// CHECK-DSYMUTIL-MULTIARCH: "/usr/bin/ld" "-demangle" "-object_path_lto"
+// CHECK-DSYMUTIL-MULTIARCH: "/usr/bin/dsymutil" "-o" "a.out.dSYM" "a.out"

aprantl wrote:
> Is a.out the `lipo`ed fat binary? If yes this LGTM
Yes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85247/new/

https://reviews.llvm.org/D85247

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39930: [CMake] Use libc++ and compiler-rt as default libraries in Fuchsia toolchain

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@phosek, I would love to see compiler-rt fully refactored so that we target one 
triple at a time always. I'd also love to see the clang driver refactored so 
that the code to resolve paths to runtime libraries was shared so we follow the 
same conventions across all driver implementations.

These are both large projects with a lot of stakeholders, and I don't expect 
that I'll be much use on it anytime soon.


Repository:
  rL LLVM

https://reviews.llvm.org/D39930



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40258: [CMake] Support side-by-side checkouts in multi-stage build

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rL LLVM

https://reviews.llvm.org/D40258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40280: [CMake][libcxx] Include AddLLVM needed for tests in the right context

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rL LLVM

https://reviews.llvm.org/D40280



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40257: [CMake] Use LIST_SEPARATOR rather than escaping in ExternalProject_Add

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rL LLVM

https://reviews.llvm.org/D40257



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40675: [clang] Use add_llvm_install_targets

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D40675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40681: [libc++abi] Add install-cxxabi-stripped target

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM.


https://reviews.llvm.org/D40681



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits