[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-09 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov created this revision.
ilya-biryukov added reviewers: sammccall, arphaman.
Herald added a reviewer: EricWF.
Herald added subscribers: kadircet, christof, ioeric.

When they read compiler args from compile_commands.json.
This change allows to run clang-based tools, like clang-tidy or clangd,
built from head using the compile_commands.json file produced for XCode
toolchains.

On MacOS clang can find the C++ standard library relative to the
compiler installation dir.

The logic to do this was based on resource dir as an approximation of
where the compiler is installed. This broke the tools that read
'compile_commands.json' and don't ship with the compiler, as they
typically change resource dir.

To workaround this, we now use compiler install dir detected by the driver
to better mimic the behavior of the original compiler when replaying the
compilations using other tools.


Repository:
  rC Clang

https://reviews.llvm.org/D54310

Files:
  include/clang/Lex/HeaderSearchOptions.h
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/CreateInvocationFromCommandLine.cpp
  lib/Frontend/InitHeaderSearch.cpp
  lib/Tooling/Tooling.cpp
  test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
  test/Tooling/clang-check-mac-libcxx.cpp

Index: test/Tooling/clang-check-mac-libcxx.cpp
===
--- /dev/null
+++ test/Tooling/clang-check-mac-libcxx.cpp
@@ -0,0 +1,16 @@
+// Clang on MacOS can find libc++ living beside the installed compiler.
+// This test makes sure our libTooling-based tools emulate this properly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+//
+// Install the mock libc++ (simulates the libc++ directory structure).
+// RUN: cp -r %S/Inputs/mock-libcxx %t/
+//
+// Pretend clang is installed beside the mock library that we provided.
+// RUN: echo '[{"directory":"%t","command":"%t/mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-check -p "%t" "%t/test.cpp"
+
+#include 
+vector v;
Index: test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
===
--- /dev/null
+++ test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
@@ -0,0 +1 @@
+class vector {};
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -323,6 +323,9 @@
 Invocation->getPreprocessorOpts().addRemappedFile(It.getKey(),
   Input.release());
   }
+  // Patch up the install dir, so we find the same standard library as the
+  // original compiler on MacOS.
+  Invocation->getHeaderSearchOpts().InstallDir = Driver->getInstalledDir();
   return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
std::move(PCHContainerOps));
 }
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -476,14 +476,9 @@
   if (triple.isOSDarwin()) {
 // On Darwin, libc++ may be installed alongside the compiler in
 // include/c++/v1.
-if (!HSOpts.ResourceDir.empty()) {
-  // Remove version from foo/lib/clang/version
-  StringRef NoVer = llvm::sys::path::parent_path(HSOpts.ResourceDir);
-  // Remove clang from foo/lib/clang
-  StringRef Lib = llvm::sys::path::parent_path(NoVer);
-  // Remove lib from foo/lib
-  SmallString<128> P = llvm::sys::path::parent_path(Lib);
-
+if (!HSOpts.InstallDir.empty()) {
+  // Get from foo/bin to foo.
+  SmallString<128> P(llvm::sys::path::parent_path(HSOpts.InstallDir));
   // Get foo/include/c++/v1
   llvm::sys::path::append(P, "include", "c++", "v1");
   AddUnmappedPath(P, CXXSystem, false);
Index: lib/Frontend/CreateInvocationFromCommandLine.cpp
===
--- lib/Frontend/CreateInvocationFromCommandLine.cpp
+++ lib/Frontend/CreateInvocationFromCommandLine.cpp
@@ -11,17 +11,18 @@
 //
 //===--===//
 
-#include "clang/Frontend/Utils.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Action.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
-#include "clang/Driver/Action.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/Tool.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
+#include "clang/Frontend/Utils.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Host.h"
+#include "llvm/Support/Path.h"
 using namespace clang;
 using namespace llvm::opt;
 
@@ -102,5 +103,8 @@

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-09 Thread Sam McCall via Phabricator via cfe-commits 
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This is hacky, but solves an important problem. If you know of a good reviewer 
for this, you may want a second opinion!




Comment at: include/clang/Lex/HeaderSearchOptions.h:111
 
+  /// Compiler install dir as detected by the Driver.
+  /// Only used to add include dirs for libc++ on Darwin. Please avoid relying

Could you add "typically contains bin/clang, lib/libclang_rt, and 
include/"?



Comment at: lib/Frontend/CompilerInvocation.cpp:1776
   Opts.ResourceDir = Args.getLastArgValue(OPT_resource_dir);
+  Opts.InstallDir = Opts.ResourceDir;
 

(is this needed? I guess you fall back to this if you don't create the CI from 
the command line?)


Repository:
  rC Clang

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-09 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov updated this revision to Diff 173364.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

- Update the comment.


Repository:
  rC Clang

https://reviews.llvm.org/D54310

Files:
  include/clang/Lex/HeaderSearchOptions.h
  lib/Frontend/CreateInvocationFromCommandLine.cpp
  lib/Frontend/InitHeaderSearch.cpp
  lib/Tooling/Tooling.cpp
  test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
  test/Tooling/clang-check-mac-libcxx.cpp

Index: test/Tooling/clang-check-mac-libcxx.cpp
===
--- /dev/null
+++ test/Tooling/clang-check-mac-libcxx.cpp
@@ -0,0 +1,16 @@
+// Clang on MacOS can find libc++ living beside the installed compiler.
+// This test makes sure our libTooling-based tools emulate this properly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+//
+// Install the mock libc++ (simulates the libc++ directory structure).
+// RUN: cp -r %S/Inputs/mock-libcxx %t/
+//
+// Pretend clang is installed beside the mock library that we provided.
+// RUN: echo '[{"directory":"%t","command":"%t/mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-check -p "%t" "%t/test.cpp"
+
+#include 
+vector v;
Index: test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
===
--- /dev/null
+++ test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
@@ -0,0 +1 @@
+class vector {};
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -323,6 +323,9 @@
 Invocation->getPreprocessorOpts().addRemappedFile(It.getKey(),
   Input.release());
   }
+  // Patch up the install dir, so we find the same standard library as the
+  // original compiler on MacOS.
+  Invocation->getHeaderSearchOpts().InstallDir = Driver->getInstalledDir();
   return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
std::move(PCHContainerOps));
 }
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -476,14 +476,9 @@
   if (triple.isOSDarwin()) {
 // On Darwin, libc++ may be installed alongside the compiler in
 // include/c++/v1.
-if (!HSOpts.ResourceDir.empty()) {
-  // Remove version from foo/lib/clang/version
-  StringRef NoVer = llvm::sys::path::parent_path(HSOpts.ResourceDir);
-  // Remove clang from foo/lib/clang
-  StringRef Lib = llvm::sys::path::parent_path(NoVer);
-  // Remove lib from foo/lib
-  SmallString<128> P = llvm::sys::path::parent_path(Lib);
-
+if (!HSOpts.InstallDir.empty()) {
+  // Get from foo/bin to foo.
+  SmallString<128> P(llvm::sys::path::parent_path(HSOpts.InstallDir));
   // Get foo/include/c++/v1
   llvm::sys::path::append(P, "include", "c++", "v1");
   AddUnmappedPath(P, CXXSystem, false);
Index: lib/Frontend/CreateInvocationFromCommandLine.cpp
===
--- lib/Frontend/CreateInvocationFromCommandLine.cpp
+++ lib/Frontend/CreateInvocationFromCommandLine.cpp
@@ -11,17 +11,18 @@
 //
 //===--===//
 
-#include "clang/Frontend/Utils.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Action.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
-#include "clang/Driver/Action.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/Tool.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
+#include "clang/Frontend/Utils.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Host.h"
+#include "llvm/Support/Path.h"
 using namespace clang;
 using namespace llvm::opt;
 
@@ -102,5 +103,8 @@
  CCArgs.size(),
  *Diags))
 return nullptr;
+  // Patch up the install dir, so we find the same standard library as the
+  // original compiler on MacOS.
+  CI->getHeaderSearchOpts().InstallDir = TheDriver.getInstalledDir();
   return CI;
 }
Index: include/clang/Lex/HeaderSearchOptions.h
===
--- include/clang/Lex/HeaderSearchOptions.h
+++ include/clang/Lex/HeaderSearchOptions.h
@@ -108,6 +108,13 @@
   /// etc.).
   std::string ResourceDir;
 
+  /// Compiler install dir as detected by the Driver.
+  /// This is typically the directory that contains the clang executable, i.e.
+  /// the 'bin/' subdir of a clang distribution.
+  /// Only used to a

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-09 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

In https://reviews.llvm.org/D54310#1292924, @sammccall wrote:

> If you know of a good reviewer for this, you may want a second opinion!


This would definitely be nice, but I also don't know who'd be the proper person 
to review this.
I'll take a pause until Monday, maybe @arphaman or someone else from the 
community can suggest a better way of fixing this or proper reviewers for this 
change.




Comment at: include/clang/Lex/HeaderSearchOptions.h:111
 
+  /// Compiler install dir as detected by the Driver.
+  /// Only used to add include dirs for libc++ on Darwin. Please avoid relying

sammccall wrote:
> Could you add "typically contains bin/clang, lib/libclang_rt, and 
> include/"?
This is actually the 'bin' subdirectory, I've added a comment about that but 
you see a better way to communicate this, please let me know! E.g. actually 
mentioning the parallel lib/ and include/ subdirs. 



Comment at: lib/Frontend/CompilerInvocation.cpp:1776
   Opts.ResourceDir = Args.getLastArgValue(OPT_resource_dir);
+  Opts.InstallDir = Opts.ResourceDir;
 

sammccall wrote:
> (is this needed? I guess you fall back to this if you don't create the CI 
> from the command line?)
Thanks for noticing this!
This is a leftover from a previous iteration, it's actually incorrect now since 
we rely on `InstallDir` pointing to a different place from the resource dir.
Removed it.


Repository:
  rC Clang

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov updated this revision to Diff 173660.
ilya-biryukov added a comment.

- Added a test with a compiler path relative to the working dir


Repository:
  rC Clang

https://reviews.llvm.org/D54310

Files:
  include/clang/Lex/HeaderSearchOptions.h
  lib/Frontend/CreateInvocationFromCommandLine.cpp
  lib/Frontend/InitHeaderSearch.cpp
  lib/Tooling/Tooling.cpp
  test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
  test/Tooling/clang-check-mac-libcxx-abspath.cpp
  test/Tooling/clang-check-mac-libcxx-relpath.cpp

Index: test/Tooling/clang-check-mac-libcxx-relpath.cpp
===
--- /dev/null
+++ test/Tooling/clang-check-mac-libcxx-relpath.cpp
@@ -0,0 +1,17 @@
+// Clang on MacOS can find libc++ living beside the installed compiler.
+// This test makes sure our libTooling-based tools emulate this properly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+//
+// Install the mock libc++ (simulates the libc++ directory structure).
+// RUN: cp -r %S/Inputs/mock-libcxx %t/
+//
+// Pretend clang is installed beside the mock library that we provided.
+// RUN: echo '[{"directory":"%t","command":"mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: cp "%s" "%t/test.cpp"
+// clang-check will produce an error code if the mock library is not found.
+// RUN: clang-check -p "%t" "%t/test.cpp"
+
+#include 
+vector v;
Index: test/Tooling/clang-check-mac-libcxx-abspath.cpp
===
--- /dev/null
+++ test/Tooling/clang-check-mac-libcxx-abspath.cpp
@@ -0,0 +1,17 @@
+// Clang on MacOS can find libc++ living beside the installed compiler.
+// This test makes sure our libTooling-based tools emulate this properly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+//
+// Install the mock libc++ (simulates the libc++ directory structure).
+// RUN: cp -r %S/Inputs/mock-libcxx %t/
+//
+// Pretend clang is installed beside the mock library that we provided.
+// RUN: echo '[{"directory":"%t","command":"%t/mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: cp "%s" "%t/test.cpp"
+// clang-check will produce an error code if the mock library is not found.
+// RUN: clang-check -p "%t" "%t/test.cpp"
+
+#include 
+vector v;
Index: test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
===
--- /dev/null
+++ test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
@@ -0,0 +1 @@
+class vector {};
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -327,6 +327,9 @@
 Invocation->getPreprocessorOpts().addRemappedFile(It.getKey(),
   Input.release());
   }
+  // Patch up the install dir, so we find the same standard library as the
+  // original compiler on MacOS.
+  Invocation->getHeaderSearchOpts().InstallDir = Driver->getInstalledDir();
   return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
std::move(PCHContainerOps));
 }
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -476,14 +476,9 @@
   if (triple.isOSDarwin()) {
 // On Darwin, libc++ may be installed alongside the compiler in
 // include/c++/v1.
-if (!HSOpts.ResourceDir.empty()) {
-  // Remove version from foo/lib/clang/version
-  StringRef NoVer = llvm::sys::path::parent_path(HSOpts.ResourceDir);
-  // Remove clang from foo/lib/clang
-  StringRef Lib = llvm::sys::path::parent_path(NoVer);
-  // Remove lib from foo/lib
-  SmallString<128> P = llvm::sys::path::parent_path(Lib);
-
+if (!HSOpts.InstallDir.empty()) {
+  // Get from foo/bin to foo.
+  SmallString<128> P(llvm::sys::path::parent_path(HSOpts.InstallDir));
   // Get foo/include/c++/v1
   llvm::sys::path::append(P, "include", "c++", "v1");
   AddUnmappedPath(P, CXXSystem, false);
Index: lib/Frontend/CreateInvocationFromCommandLine.cpp
===
--- lib/Frontend/CreateInvocationFromCommandLine.cpp
+++ lib/Frontend/CreateInvocationFromCommandLine.cpp
@@ -11,17 +11,18 @@
 //
 //===--===//
 
-#include "clang/Frontend/Utils.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Action.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
-#include "clang/Driver/Action.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346652: Make clang-based tools find libc++ on MacOS 
(authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D54310

Files:
  cfe/trunk/include/clang/Lex/HeaderSearchOptions.h
  cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp
  cfe/trunk/lib/Frontend/InitHeaderSearch.cpp
  cfe/trunk/lib/Tooling/Tooling.cpp
  cfe/trunk/test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
  cfe/trunk/test/Tooling/clang-check-mac-libcxx-abspath.cpp
  cfe/trunk/test/Tooling/clang-check-mac-libcxx-relpath.cpp

Index: cfe/trunk/lib/Frontend/InitHeaderSearch.cpp
===
--- cfe/trunk/lib/Frontend/InitHeaderSearch.cpp
+++ cfe/trunk/lib/Frontend/InitHeaderSearch.cpp
@@ -476,14 +476,9 @@
   if (triple.isOSDarwin()) {
 // On Darwin, libc++ may be installed alongside the compiler in
 // include/c++/v1.
-if (!HSOpts.ResourceDir.empty()) {
-  // Remove version from foo/lib/clang/version
-  StringRef NoVer = llvm::sys::path::parent_path(HSOpts.ResourceDir);
-  // Remove clang from foo/lib/clang
-  StringRef Lib = llvm::sys::path::parent_path(NoVer);
-  // Remove lib from foo/lib
-  SmallString<128> P = llvm::sys::path::parent_path(Lib);
-
+if (!HSOpts.InstallDir.empty()) {
+  // Get from foo/bin to foo.
+  SmallString<128> P(llvm::sys::path::parent_path(HSOpts.InstallDir));
   // Get foo/include/c++/v1
   llvm::sys::path::append(P, "include", "c++", "v1");
   AddUnmappedPath(P, CXXSystem, false);
Index: cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp
===
--- cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp
+++ cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp
@@ -11,17 +11,18 @@
 //
 //===--===//
 
-#include "clang/Frontend/Utils.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Driver/Action.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
-#include "clang/Driver/Action.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/Tool.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
+#include "clang/Frontend/Utils.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Host.h"
+#include "llvm/Support/Path.h"
 using namespace clang;
 using namespace llvm::opt;
 
@@ -102,5 +103,8 @@
  CCArgs.size(),
  *Diags))
 return nullptr;
+  // Patch up the install dir, so we find the same standard library as the
+  // original compiler on MacOS.
+  CI->getHeaderSearchOpts().InstallDir = TheDriver.getInstalledDir();
   return CI;
 }
Index: cfe/trunk/lib/Tooling/Tooling.cpp
===
--- cfe/trunk/lib/Tooling/Tooling.cpp
+++ cfe/trunk/lib/Tooling/Tooling.cpp
@@ -327,6 +327,9 @@
 Invocation->getPreprocessorOpts().addRemappedFile(It.getKey(),
   Input.release());
   }
+  // Patch up the install dir, so we find the same standard library as the
+  // original compiler on MacOS.
+  Invocation->getHeaderSearchOpts().InstallDir = Driver->getInstalledDir();
   return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
std::move(PCHContainerOps));
 }
Index: cfe/trunk/include/clang/Lex/HeaderSearchOptions.h
===
--- cfe/trunk/include/clang/Lex/HeaderSearchOptions.h
+++ cfe/trunk/include/clang/Lex/HeaderSearchOptions.h
@@ -108,6 +108,13 @@
   /// etc.).
   std::string ResourceDir;
 
+  /// Compiler install dir as detected by the Driver.
+  /// This is typically the directory that contains the clang executable, i.e.
+  /// the 'bin/' subdir of a clang distribution.
+  /// Only used to add include dirs for libc++ on Darwin. Please avoid relying
+  /// on this field for other purposes.
+  std::string InstallDir;
+
   /// The directory used for the module cache.
   std::string ModuleCachePath;
 
Index: cfe/trunk/test/Tooling/clang-check-mac-libcxx-relpath.cpp
===
--- cfe/trunk/test/Tooling/clang-check-mac-libcxx-relpath.cpp
+++ cfe/trunk/test/Tooling/clang-check-mac-libcxx-relpath.cpp
@@ -0,0 +1,17 @@
+// Clang on MacOS can find libc++ living beside the installed compiler.
+// This test makes sure our libTooling-based tools emulate this properly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+//
+// Install the mock libc++ (simulates the libc++ directory structure).
+// RUN: cp -r %S/Inputs/mock-libcxx %t/
+

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread Jonas Devlieghere via Phabricator via cfe-commits 
JDevlieghere added a comment.

I reverted this because it broke the LLDB bots on GreenDragon: 
http://green.lab.llvm.org/green/view/LLDB/


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread George Karpenkov via Phabricator via cfe-commits 
george.karpenkov added a comment.

I don't quite understand the need for this patch.
If we are talking about a binary built from source, wouldn't it make more sense 
to build libcxx from source as well?
If libcxx is built from source in the same repo, clang-based tools do find it.


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread George Karpenkov via Phabricator via cfe-commits 
george.karpenkov added a comment.

I suspect it broke 
http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/14090/console as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread George Karpenkov via Phabricator via cfe-commits 
george.karpenkov added a comment.

In the future, for macOS-specific changes I think it would be better to wait 
for a sign-off from at least one maintainer who is an expert on Apple tools.


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread Mailing List "llvm-commits" via Phabricator via cfe-commits 
llvm-commits added a comment.



- F7538987: msg-17782-346.txt 


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added a comment.

Apologies for not seeing this earlier.
I agree with George, this behavior doesn't seem right to me.

> The logic to do this was based on resource dir as an approximation of
>  where the compiler is installed. This broke the tools that read
>  'compile_commands.json' and don't ship with the compiler, as they
>  typically change resource dir.

In that case I think the tool should adjust the `-resource-dir` to point to the 
appropriate `-resource-dir` for the compiler in the toolchain. However, that 
comes with its own problems, as the builtin headers might be incompatible 
between the tool and the compiler in the toolchain. In that case I think it 
would be preferable for the tool to ship with its own libc++ headers in 
addition to the builtin headers rather than implementing this behavior. If 
that's unacceptable I think we should adjust libTooling to add a search path 
for the libc++ headers when the tool detects that it's running outside of the 
toolchain on Darwin (i.e. 'clang' doesn't exist in the same directory as the 
tool, and '../include/c++' doesn't exist as well)  instead of changing this 
behavior.


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

In https://reviews.llvm.org/D54310#1296080, @arphaman wrote:

> Apologies for not seeing this earlier.


No worries, thanks for the input!

>> The logic to do this was based on resource dir as an approximation of
>>  where the compiler is installed. This broke the tools that read
>>  'compile_commands.json' and don't ship with the compiler, as they
>>  typically change resource dir.
> 
> In that case I think the tool should adjust the `-resource-dir` to point to 
> the appropriate `-resource-dir` for the compiler in the toolchain. However, 
> that comes with its own problems, as the builtin headers might be 
> incompatible between the tool and the compiler in the toolchain.

Exactly the problem we're facing. Since the -resource-dir is primarily used to 
find the builtin headers, we **have** to override it in the tools to avoid 
breaking them.

> In that case I think it would be preferable for the tool to ship with its own 
> libc++ headers in addition to the builtin headers rather than implementing 
> this behavior.

There's value in providing the users with warnings/errors from exactly the same 
version of the C++ standard library that's used by the compiler, so I would 
avoid going down this path if possible.

> If that's unacceptable I think we should adjust libTooling to add a search 
> path for the libc++ headers when the tool detects that it's running outside 
> of the toolchain on Darwin (i.e. 'clang' doesn't exist in the same directory 
> as the tool, and '../include/c++' doesn't exist as well)  instead of changing 
> this behavior.

This was the original intention of the patch, but I don't think we should limit 
this to libTooling.
I'll have a look at what exactly lldb broke and will come up with a fix to this 
behavior.


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

Re: [PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread Ilya Biryukov via cfe-commits 
Will take a look, thanks for the revert.

On Mon, Nov 12, 2018 at 6:13 PM Jonas Devlieghere via Phabricator <
revi...@reviews.llvm.org> wrote:

> JDevlieghere added a comment.
>
> I reverted this because it broke the LLDB bots on GreenDragon:
> http://green.lab.llvm.org/green/view/LLDB/
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D54310
>
>
>
>

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

Re: [PATCH] D54310: Make clang-based tools find libc++ on MacOS

2018-11-12 Thread Sam McCall via cfe-commits 
On Mon, Nov 12, 2018, 20:53 George Karpenkov via Phabricator <
revi...@reviews.llvm.org wrote:

> george.karpenkov added a comment.
>
> I don't quite understand the need for this patch.
> If we are talking about a binary built from source, wouldn't it make more
> sense to build libcxx from source as well?
>
I'm not sure what you mean by "built from source" - all programs are?
The intent is to distribute as binary (I think), and not distribute libcxx.
It's important the system stdlib is used, as that's the one consistent with
the CDB and the actual build.

If libcxx is built from source in the same repo, clang-based tools do find
> it.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D54310
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits