Commit 99b7b41e authored by Raphael Isemann's avatar Raphael Isemann
Browse files

[lldb][import-std-module] Do some basic file checks before trying to import a module 

Currently when LLDB has enough data in the debug information to import the `std` module,
it will just try to import it. However when debugging libraries where the sources aren't
available anymore, importing the module will generate a confusing diagnostic that
the module couldn't be built.

For the fallback mode (where we retry failed expressions with the loaded module), this
will cause the second expression to fail with a module built error instead of the
actual parsing issue in the user expression.

This patch adds checks that ensures that we at least have any source files in the found
include paths before we try to import the module. This prevents the module from being
loaded in the situation described above which means we don't emit the bogus 'can't
import module' diagnostic and also don't waste any time retrying the expression in the
fallback mode.

For the unit tests I did some refactoring as they now require a VFS with the files in it
and not just the paths. The Python test just builds a binary with a fake C++ module,
then deletes the module before debugging.

Fixes rdar://73264458

Reviewed By: JDevlieghere

Differential Revision: https://reviews.llvm.org/D95096
parent ff41ae8b

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp

+33 −3
Original line number Diff line number Diff line
@@ -57,9 +57,38 @@ bool CppModuleConfiguration::analyzeFile(const FileSpec &f) { 
  return true;

}



/// Utility function for just appending two paths.

static std::string MakePath(llvm::StringRef lhs, llvm::StringRef rhs) {

  llvm::SmallString<256> result(lhs);

  llvm::sys::path::append(result, rhs);

  return std::string(result);

}



bool CppModuleConfiguration::hasValidConfig() {

  // We all these include directories to have a valid usable configuration.

  return m_c_inc.Valid() && m_std_inc.Valid();

  // We need to have a C and C++ include dir for a valid configuration.

  if (!m_c_inc.Valid() || !m_std_inc.Valid())

    return false;



  // Do some basic sanity checks on the directories that we don't activate

  // the module when it's clear that it's not usable.

  const std::vector<std::string> files_to_check = {

      // * Check that the C library contains at least one random C standard

      //   library header.

      MakePath(m_c_inc.Get(), "stdio.h"),

      // * Without a libc++ modulemap file we can't have a 'std' module that

      //   could be imported.

      MakePath(m_std_inc.Get(), "module.modulemap"),

      // * Check for a random libc++ header (vector in this case) that has to

      //   exist in a working libc++ setup.

      MakePath(m_std_inc.Get(), "vector"),

  };



  for (llvm::StringRef file_to_check : files_to_check) {

    if (!FileSystem::Instance().Exists(file_to_check))

      return false;

  }



  return true;

}



CppModuleConfiguration::CppModuleConfiguration(
@@ -78,7 +107,8 @@ CppModuleConfiguration::CppModuleConfiguration( 
    m_resource_inc = std::string(resource_dir.str());



    // This order matches the way Clang orders these directories.

    m_include_dirs = {m_std_inc.Get(), m_resource_inc, m_c_inc.Get()};

    m_include_dirs = {m_std_inc.Get().str(), m_resource_inc,

                      m_c_inc.Get().str()};

    m_imported_modules = {"std"};

  }

}

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h

+1 −4
Original line number Diff line number Diff line
@@ -32,7 +32,7 @@ class CppModuleConfiguration { 
    /// the path was already set.

    LLVM_NODISCARD bool TrySet(llvm::StringRef path);

    /// Return the path if there is one.

    std::string Get() const {

    llvm::StringRef Get() const {

      assert(m_valid && "Called Get() on an invalid SetOncePath?");

      return m_path;

    }
@@ -57,9 +57,6 @@ class CppModuleConfiguration { 


public:

  /// Creates a configuration by analyzing the given list of used source files.

  ///

  /// Currently only looks at the used paths and doesn't actually access the

  /// files on the disk.

  explicit CppModuleConfiguration(const FileSpecList &support_files);

  /// Creates an empty and invalid configuration.

  CppModuleConfiguration() {}

lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/algorithm

+1 −1
Original line number Diff line number Diff line
@@ -2,7 +2,7 @@ 
// library module so the actual contents of the module are missing.

#ifdef ENABLE_STD_CONTENT



#include "libc_header.h"

#include "stdio.h"



namespace std {

  inline namespace __1 {

lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/libc_header.h→lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/stdio.h

+0 −0

File moved.

View file