[ExpressionParser] Test GetClangResourceDir

[xiaobai]

Summary

I'm doing this because I plan on implementing ComputeClangDirectory on windows
so that GetClangResourceDir will work. Additionally, I made
test_paths make sure that the directory member of the returned FileSpec is not
none. This will fail on windows since ComputeClangDirectory isn't implemented
yet.

[teemperor]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

Doesn't that mean that we no longer test any of the paths on windows? E.g. if someone breaks lldb.ePathTypeLLDBShlibDir we will not see this test failing on Windows. I would prefer of extracting the ClangDir test into it's own method that we mark as failing.

[xiaobai]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

Technically it is being tested but it is expected to fail. That being said, I don't mind extracting the ClangDir test into its own method while I work on implementing it, but I don't think it needs its own test. Maybe after I implement GetClangResourceDir on windows I can merge the tests? :P

[labath]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

That sounds fine, though I have to say that these tests don't really test much. There is a test for the MacOS functionality here in unittests/Expression/ClangParserTest.cpp, which is much more detailed. It would be great if you could add a windows version there too.

[xiaobai]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

I originally added this test there, but testing things there as-is is presents a disadvantage: The unittests don't link against liblldb but the individual lldb libraries, so the methods that figure out where the Clang Resource Dir (and pretty much any other directory for that matter) will report something like this:
/path/to/llvm/build/tools/lldb/unittests/lib/clang/9.0.0. So, the best this test could do is set that the filespec had a directory and not a filename set, which is what this test already does.

That being said, the way MacOS is being tested is much nicer. I could do a small refactor so the non-MacOS posix implementation is closer to the MacOS implementation and add a Linux test (and eventually a windows test) to unittests/Expression/ClangParserTest.cpp as well. What do you think?

[labath]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

Sounds like a plan.

[xiaobai]

Address suggestions and add unit test

[xiaobai]

Reword comment to be clearer

[xiaobai]

Looks like the test I wrote doesn't pass on MacOS, I need to investigate.

[xiaobai]

Minor changes to make MacOS work

[labath]

Looks fine to me, but maybe @aprantl could give this a quick look, as he's the one (IIRC) who introduced this function.

[xiaobai]

@aprantl: Do you mind taking a look at this when you get a moment?

[aprantl]

Looks mostly good!

source/Plugins/ExpressionParser/Clang/ClangHost.cpp

59

While you are at it, could you rename this DefaultComputeClangResourceDirectory?
And add a doxygen comment that says it will compute the calng resource directory assuming that clang was installed to the same prefix as lldb?

[xiaobai]

{Default,}ComputeClangDirectory -> {Default,}ComputeClangResourceDir
Added a comment explaining exactly how DefaultComputeClangResourceDir works

[stella.stamenova]

This introduced a build break on Windows (which happened to coincide with another build break).

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/2317

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

 from lldbsuite.test import lldbutil
 
 
 class TestPaths(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     @no_debug_info_test
     def test_paths(self):
         '''Test to make sure no file names are set in the lldb.SBFileSpec objects returned by lldb.SBHostOS.GetLLDBPath() for paths that are directories'''
         dir_path_types = [lldb.ePathTypeLLDBShlibDir,
                           lldb.ePathTypeSupportExecutableDir,
                           lldb.ePathTypeHeaderDir,
                           lldb.ePathTypePythonDir,
                           lldb.ePathTypeLLDBSystemPlugins,
                           lldb.ePathTypeLLDBUserPlugins,
                           lldb.ePathTypeLLDBTempSystemDir]
 
         for path_type in dir_path_types:
             f = lldb.SBHostOS.GetLLDBPath(path_type)
             # No directory path types should have the filename set
             self.assertTrue(f.GetFilename() is None)
 
+    # TODO: Merge into test_path when GetClangResourceDir is implemented on
+    # windows
+    @expectedFailureAll(oslist=["windows"])
+    @no_debug_info_test
+    def test_clang_dir_path(self):
+      '''Test to make sure clang dir is set correctly'''
+      clang_dir = lldb.SBHostOS.GetLLDBPath(lldb.ePathTypeClangDir)
+      self.assertFalse(clang_dir.GetDirectory() is None)
+
     @no_debug_info_test
     def test_directory_doesnt_end_with_slash(self):
         current_directory_spec = lldb.SBFileSpec(os.path.curdir)
         current_directory_string = current_directory_spec.GetDirectory()
         self.assertNotEqual(current_directory_string[-1:], '/')
         pass
 
     @skipUnlessPlatform(["windows"])

source/Plugins/ExpressionParser/Clang/ClangHost.h

 //===-- ClangHost.h ---------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_PLUGINS_EXPRESSIONPARSER_CLANG_CLANGHOST_H
 #define LLDB_PLUGINS_EXPRESSIONPARSER_CLANG_CLANGHOST_H
 
 namespace lldb_private {
 
 class FileSpec;
 
-#if defined(__APPLE__)
+#if !defined(_WIN32)
 bool ComputeClangDirectory(FileSpec &lldb_shlib_spec, FileSpec &file_spec,
                            bool verify);
 #endif
 
 FileSpec GetClangResourceDir();
 
 } // namespace lldb_private
 
 #endif

source/Plugins/ExpressionParser/Clang/ClangHost.cpp

 #include <string>
 
 using namespace lldb_private;
 
 #if defined(_WIN32)
 static bool ComputeClangDirectory(FileSpec &file_spec) { return false; }
 #else
 static bool DefaultComputeClangDirectory(FileSpec &file_spec) {
-  return HostInfoPosix::ComputePathRelativeToLibrary(
-      file_spec, (llvm::Twine("/lib") + CLANG_LIBDIR_SUFFIX + "/clang/" +
-                  CLANG_VERSION_STRING)
-                     .str());
+  llvm::SmallString<128> relative_path;
+  llvm::sys::path::append(relative_path,
+                          llvm::Twine("lib") + CLANG_LIBDIR_SUFFIX, "clang",
+                          CLANG_VERSION_STRING);
+  return HostInfoPosix::ComputePathRelativeToLibrary(file_spec, relative_path);
 }
 
-#if defined(__APPLE__)
+static bool ComputeClangDirectory(FileSpec &file_spec) {
+  if (FileSpec lldb_file_spec = HostInfo::GetShlibDir())
+    return ComputeClangDirectory(lldb_file_spec, file_spec, true);
+  return false;
+}
 
 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;
 }
 
 bool lldb_private::ComputeClangDirectory(FileSpec &lldb_shlib_spec,
                                          FileSpec &file_spec, bool verify) {
+#if !defined(__APPLE__)
+  std::string raw_path = lldb_shlib_spec.GetPath();
+  llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
+
+  llvm::SmallString<128> clang_dir(parent_dir);
+  llvm::sys::path::append(clang_dir, llvm::Twine("lib") + CLANG_LIBDIR_SUFFIX,
+                          "clang", CLANG_VERSION_STRING);
+  if (!verify || VerifyClangPath(clang_dir)) {
+    file_spec.GetDirectory().SetString(clang_dir);
+    FileSystem::Instance().Resolve(file_spec);
+    return true;
+  }
+  return DefaultComputeClangDirectory(file_spec);
+#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")
if (parent != r_end && *parent == "SharedFrameworks") {
     raw_path.resize(rev_it - r_end);
   }
 
   // Fall back to the Clang resource directory inside the framework.
   raw_path.append("LLDB.framework/Resources/Clang");
   file_spec.SetFile(raw_path.c_str(), FileSpec::Style::native);
   FileSystem::Instance().Resolve(file_spec);
   return true;
-}
-
-static bool ComputeClangDirectory(FileSpec &file_spec) {
-  if (FileSpec lldb_file_spec = HostInfo::GetShlibDir())
-    return ComputeClangDirectory(lldb_file_spec, file_spec, true);
-  return false;
-}
-#else  // __APPLE__
-
-// All non-Apple posix systems.
-static bool ComputeClangDirectory(FileSpec &file_spec) {
-  return DefaultComputeClangDirectory(file_spec);
-}
 #endif // __APPLE__
+}
 #endif // _WIN32
 
 FileSpec lldb_private::GetClangResourceDir() {
   static FileSpec g_cached_resource_dir;
   static llvm::once_flag g_once_flag;
   llvm::call_once(g_once_flag, []() {
     ::ComputeClangDirectory(g_cached_resource_dir);
     Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
     if (log)
       log->Printf("GetClangResourceDir() => '%s'",
                   g_cached_resource_dir.GetPath().c_str());
   });
   return g_cached_resource_dir;
 }

unittests/Expression/ClangParserTest.cpp

 //===-- ClangParserTest.cpp --------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/Basic/Version.h"
+
 #include "Plugins/ExpressionParser/Clang/ClangHost.h"
 #include "TestingSupport/TestUtilities.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/lldb-defines.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
 
 namespace {
 struct ClangHostTest : public testing::Test {
   static void SetUpTestCase() {
     FileSystem::Initialize();
     HostInfo::Initialize();
   }
   static void TearDownTestCase() {
     HostInfo::Terminate();
     FileSystem::Terminate();
   }
 };
 } // namespace
 
-#ifdef __APPLE__
+#if !defined(_WIN32)
 static std::string ComputeClangDir(std::string lldb_shlib_path,
                                    bool verify = false) {
   FileSpec clang_dir;
   FileSpec lldb_shlib_spec(lldb_shlib_path);
   ComputeClangDirectory(lldb_shlib_spec, clang_dir, verify);
   return clang_dir.GetPath();
 }
 
+TEST_F(ClangHostTest, ComputeClangDirectory) {
+  std::string path_to_liblldb = "/foo/bar/lib/";
+  std::string path_to_clang_dir = "/foo/bar/lib/clang/" CLANG_VERSION_STRING;
+  EXPECT_EQ(ComputeClangDir(path_to_liblldb), path_to_clang_dir);
+
+  // The path doesn't really exist, so verify shouldn't return
+  // path_to_clang_dir.
+  EXPECT_NE(ComputeClangDir(path_to_liblldb, true),
+            ComputeClangDir(path_to_liblldb));
+}
+
+#if defined(__APPLE__)
 TEST_F(ClangHostTest, MacOSX) {
   // This returns whatever the POSIX fallback returns.
   std::string posix = "/usr/lib/liblldb.dylib";
   EXPECT_FALSE(ComputeClangDir(posix).empty());
 
   std::string build =
       "/lldb-macosx-x86_64/Library/Frameworks/LLDB.framework/Versions/A";
   std::string build_clang =
std::string cltools_clang =
       "/Library/Developer/CommandLineTools/Library/PrivateFrameworks/"
       "LLDB.framework/Resources/Clang";
   EXPECT_EQ(ComputeClangDir(cltools), cltools_clang);
 
   // Test that a bogus path is detected.
   EXPECT_NE(ComputeClangDir(GetInputFilePath(xcode), true),
             ComputeClangDir(GetInputFilePath(xcode)));
 }
-#endif
+#endif // __APPLE__
+#endif // !_WIN32