[ExpressionParser] Add swift-lldb case for finding clang resource dir

[xiaobai]

Summary

I'm adding this to reduce the difference between swift-lldb and
llvm.org's lldb.

[jingham]

The behavior when verify is false seems a little weird to me. In that case you will just always get the $install_dir/lib/clang/$clang_version path. That's fairly different from the verify = true case. OTOH, I couldn't see anybody passing verify = false anywhere. Do we need that?

[compnerd]

source/Plugins/ExpressionParser/Clang/ClangHost.cpp

84

Does swift-lldb never honour LIBDIR? That is, can it never end up in lib64?

90

I think it would be nicer if you could create a static list of the paths and loop over it:

static const StringRef kResourceDirSuffixes[] = {
  "lib" TO_STRING(CLANG_LIBDIR_SUFFIX) "/clang" CLANG_VERSION_STRING,
  "lib" TO_STRING(CLANG_LIBDIR_SUFFIX) "/lldb/clang",
};

for (const auto &Suffix : kResourceDirSuffixes) {
    llvm::SmallString<256> clang_dir(parent_dir);
    llvm::SmallString<32> relative_path(Suffix);
    llvm::sys::path::native(relative_path);
    llvm::sys::path::append(clang_dir, relative_path);
    if (!verify || VerifyClangPath(clang_dir)) {
      file_spec.GetDirectory().SetString(clang_dir);
      FileSystem::Instance().Resolve(file_spec);
      return true;
    }
}

[xiaobai]

In D59708#1439872, @jingham wrote:

The behavior when verify is false seems a little weird to me. In that case you will just always get the $install_dir/lib/clang/$clang_version path. That's fairly different from the verify = true case. OTOH, I couldn't see anybody passing verify = false anywhere. Do we need that?

The only use of verify = false is in the ClangHostTest (in the poorly named unittests/Expression/ClangParserTest.cpp). We pass a dummy path with verify set to false in order to make sure that it can correctly append lib/clang/$clang_version. Then we pass verify = true and make sure that it doesn't actually exist.

source/Plugins/ExpressionParser/Clang/ClangHost.cpp

84

Oh, it absolutely does. I'll fix that!

90

Yeah sounds good to me. Seems more manageable.

[xiaobai]

• Respect LIBDIR for swift-lldb case
• Loop over a list of known directory suffixes
• Change default behavior to not set file_spec and return false upon failure.

[xiaobai]

I changed the default behavior here setting file_spec to some known default unverified value to not setting it at all. It previously relied on there being a relative path to the clang resource dir, of which there are now two to pick from. Additionally, we pass in the directory containing liblldb to ComputeClangResourceDirectory now. The default behavior of HostInfo::ComputePathRelativeToLibrary was just computing it again and munging paths without verifying it. I think that if we fail to find the clang resource directory, the logs here should be sufficient to get started debugging.

[compnerd]

Please add a comment about verify being only for tests.

[xiaobai]

Added comment about verify

[jingham]

Except for a typo and a little quibble about a comment, this looks okay to me.

source/Plugins/ExpressionParser/Clang/ClangHost.cpp

42

spelling: parameter

But also, you should say something more informative, like:

If verify is true, the first candidate resource directory will be returned. This mode is only used for testing

[xiaobai]

source/Plugins/ExpressionParser/Clang/ClangHost.cpp

42

I'll adjust this before I commit. Thanks for pointing that out.

source/Plugins/ExpressionParser/Clang/ClangHost.cpp

 #include "clang/Basic/Version.h"
 #include "clang/Config/config.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 
+#include "lldb/Host/Config.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 
 #include <string>
 
 using namespace lldb_private;
 
 static bool VerifyClangPath(const llvm::Twine &clang_path) {
   if (FileSystem::Instance().IsDirectory(clang_path))
     return true;
   Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
   if (log)
     log->Printf("VerifyClangPath(): "
                 "failed to stat clang resource directory at \"%s\"",
                 clang_path.str().c_str());
   return false;
 }
 
 ///
 /// This will compute the clang resource directory assuming that clang was
 /// installed with the same prefix as lldb.
 ///
+/// If verify is true, the first candidate resource directory will be returned.
+/// This mode is only used for testing.
+///
 static bool DefaultComputeClangResourceDirectory(FileSpec &lldb_shlib_spec,
-                                         FileSpec &file_spec, bool verify) {
+                                                 FileSpec &file_spec,
+                                                 bool verify) {
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
   std::string raw_path = lldb_shlib_spec.GetPath();
   llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
 
-  llvm::SmallString<256> clang_dir(parent_dir);
-  llvm::SmallString<32> relative_path;
-  llvm::sys::path::append(relative_path,
-                          llvm::Twine("lib") + CLANG_LIBDIR_SUFFIX, "clang",
-                          CLANG_VERSION_STRING);
+  static const llvm::StringRef kResourceDirSuffixes[] = {
+      // LLVM.org's build of LLDB uses the clang resource directory placed
+      // in $install_dir/lib{,64}/clang/$clang_version.
+      "lib" CLANG_LIBDIR_SUFFIX "/clang/" CLANG_VERSION_STRING,
+      // swift-lldb uses the clang resource directory copied from swift, which
+      // by default is placed in $install_dir/lib{,64}/lldb/clang. LLDB places
+      // it there, so we use LLDB_LIBDIR_SUFFIX.
+      "lib" LLDB_LIBDIR_SUFFIX "/lldb/clang",
+  };
 
+  for (const auto &Suffix : kResourceDirSuffixes) {
+    llvm::SmallString<256> clang_dir(parent_dir);
+    llvm::SmallString<32> relative_path(Suffix);
+    llvm::sys::path::native(relative_path);
     llvm::sys::path::append(clang_dir, relative_path);
     if (!verify || VerifyClangPath(clang_dir)) {
+      if (log)
+        log->Printf("DefaultComputeClangResourceDir: Setting ClangResourceDir "
+                    "to \"%s\", verify = %s",
+                    clang_dir.str().str().c_str(), verify ? "true" : "false");
       file_spec.GetDirectory().SetString(clang_dir);
       FileSystem::Instance().Resolve(file_spec);
       return true;
     }
+  }
 
-  return HostInfo::ComputePathRelativeToLibrary(file_spec, relative_path);
+  return false;
 }
 
 bool lldb_private::ComputeClangResourceDirectory(FileSpec &lldb_shlib_spec,
                                          FileSpec &file_spec, bool verify) {
 #if !defined(__APPLE__)
   return DefaultComputeClangResourceDirectory(lldb_shlib_spec, file_spec,
                                               verify);
 #else
   std::string raw_path = lldb_shlib_spec.GetPath();
 
   auto rev_it = llvm::sys::path::rbegin(raw_path);
   auto r_end = llvm::sys::path::rend(raw_path);
 
   // Check for a Posix-style build of LLDB.
   while (rev_it != r_end) {
     if (*rev_it == "LLDB.framework")
       break;
     ++rev_it;

unittests/Expression/ClangParserTest.cpp

#if !defined(_WIN32)
   std::string path_to_clang_dir = "/foo/bar/lib/clang/" CLANG_VERSION_STRING;
 #else
   std::string path_to_liblldb = "C:\\foo\\bar\\lib";
   std::string path_to_clang_dir = "C:\\foo\\bar\\lib\\clang\\" CLANG_VERSION_STRING;
 #endif
   EXPECT_EQ(ComputeClangResourceDir(path_to_liblldb), path_to_clang_dir);
 
   // The path doesn't really exist, so setting verify to true should make
-  // ComputeClangResourceDir to not give you path_to_clang_dir.
-  EXPECT_NE(ComputeClangResourceDir(path_to_liblldb, true),
-            ComputeClangResourceDir(path_to_liblldb));
+  // ComputeClangResourceDir not give you path_to_clang_dir.
+  EXPECT_NE(ComputeClangResourceDir(path_to_liblldb, true), path_to_clang_dir);
 }
 
 #if defined(__APPLE__)
 TEST_F(ClangHostTest, MacOSX) {
   // This returns whatever the POSIX fallback returns.
   std::string posix = "/usr/lib/liblldb.dylib";
   EXPECT_FALSE(ComputeClangResourceDir(posix).empty());