This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by xiaobai on Mar 22 2019, 11:20 AM.

Event Timeline

xiaobai created this revision.Mar 22 2019, 11:20 AM

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 added inline comments.Mar 22 2019, 2:27 PM
source/Plugins/ExpressionParser/Clang/ClangHost.cpp
67

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

73

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;
    }
}

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
67

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

73

Yeah sounds good to me. Seems more manageable.

xiaobai updated this revision to Diff 192185.Mar 25 2019, 12:53 PM
  • 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.

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 accepted this revision.Mar 25 2019, 1:28 PM

Please add a comment about verify being only for tests.

This revision is now accepted and ready to land.Mar 25 2019, 1:28 PM
xiaobai updated this revision to Diff 192204.Mar 25 2019, 2:23 PM

Added comment about verify

jingham accepted this revision.Mar 25 2019, 5:54 PM

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

source/Plugins/ExpressionParser/Clang/ClangHost.cpp
41

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 added inline comments.Mar 26 2019, 11:42 AM
source/Plugins/ExpressionParser/Clang/ClangHost.cpp
41

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

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 2:02 PM
Herald added a subscriber: teemperor. · View Herald Transcript