Make LLDB's clang module cache path customizable

[aprantl]

Summary

This patch makes LLDB's clang module cache path customizable via settings set target.clang-modules-cache-path <path> and uses it in the LLDB testsuite to reuse the same location inside the build directory for LLDB and clang.

[jingham]

Think about whether it would be better to have GetClangModulesCachePath calculate the fallback modules path rather than having the client do it?

source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

595–601

Is there a reason not to have GetClangModulesCachePath do this? This is roughly the "default value" of the value. Is there a good reason to make clients compute that?

I presume you are computing this here because clang doesn't offer an API to do that?

[aprantl]

Address most review feedback.

[aprantl]

source/Plugins/ExpressionParser/Clang/CMakeLists.txt

26 ↗

(On Diff #133636)

I checked and this does not affects LLDB's binary size in any measurable way.

source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

595–601

Is there a reason not to have GetClangModulesCachePath do this?

Yes. It is supposed to return the value of the *property*. Also, I assume that Target doesn't necessarily link against Clang, so calling into a Clang API there seems to be a layering violation.

[davide]

LGTM modulo minor.

source/Plugins/ExpressionParser/Clang/CMakeLists.txt

26 ↗

(On Diff #133636)

This doesn't surprise me, as the driver is very small.

source/Target/Target.cpp

3949

add an assertion message if you don't mind?

[jingham]

This looks fine to me.

[aprantl]

source/Target/Target.cpp

3949

This code is mechanically duplicated from the other functions in this file, and I was actually planning on making another pass that auto-generates all of the properties and the accessor functions from a .inc file using an llvm-style HANDLE_foo() macro.

[davide]

source/Target/Target.cpp

3949

Sure, even better :) It would be awesome if lldb didn't do its own command line handling but would use something like cl::opt, but I guess that's a battle for another day.

[jingham]

I don't think there's any battle. I wouldn't mind moving the command parsing code into llvm if there's a suitable replacement.

But so far as I can see llvm's command line parsing was primarily intended for parsing args for shell tools. So it lacks features required for a replacement to lldb's command line parser.

The lldb interactive command interpreter absolutely needs completion callbacks. Nobody would be happy with lldb if it didn't complete filenames and symbols and the commands themselves. We also do shortest match rather than exact matches which folks find very handy. And you have to be able to provide our alias system which is also very heavily used. If somebody wants to add all these features to the llvm command line parser, then we could use it in lldb. I'm not sure that that would be a great idea, it might be seen to overly complicate the shell tool only parser.

Anyway, I don't see a drop-in replacement in llvm.

[zturner]

It's probably possible to make it work, but as Jim said, there's no drop in replacement currently. There's bits and pieces of stuff that, with a dedicated effort, could be improved to the point of being sufficient, though.

[jingham]

To me, the determinant here is whether the llvm project sees many more interactive tools in its future. If lldb is the only one, then moving all this functionality to llvm seems like effort better spent elsewhere. But if there are other interactive tools in the offing, it would be great to have all such tools have a common behavior, and so there's good motivation for doing this work.

[labath]

There are several components in the "command line parsing"

1. argparse: parsing a single string into the individual words (processing quotes, backslashes, etc.)
2. resolving the "command" part of the command line (the "frame variable" part of "frame variable --raw-output FOO")
3. resolving the arguments of a command (the "--raw-output FOO" part)

These parts compose quite nicely, and can be individually replaced. I'll briefly speak about each one :

(1) is very specific to our interactive use case (for command line utilities, the shell/os does the parsing for us). There also isn't anything nearly similar in llvm (afaik).
(2) is also quite specific to how lldb commands are structured, and there also isn't a suitable llvm replacement. This is the level at which the "command aliases" are resolved.
(3) is a fairly generic piece of functionality -- we even implement it by delegating to getopt (in a scarily un-thread-safe way). For this there are *two* competing implementations in llvm: llvm::cl and llvm::opt. Of these the first one is not suitable for our use case right now as it relies on global variables. Making it work probably would be a bit or work. However, using the second one should be fairly easy, and it should be generic-enough (it's the thing that handles crazy gcc command lines).

The reason I'm spelling it out this way is because I think you were mostly talking about (3), but some of the difficulties Jim pointed out (command aliases) just aren't relevant there. I don't think llvm::opt handles "shortest match" options yet, but that should be fairly easy to add. On the flip side, the library has recently gained argument suggestion capabilities (error: unknown argument '-gllbb', did you mean '-glldb'), so we would get that for free. Making sure tab completion works there would require a bit of thought, but I think this should not interfere with this library too much (after all, we use getopt now, and it does not support tab completion either).

include/lldb/Target/Target.h

public:
   bool GetSkipPrologue() const;
 
   PathMappingList &GetSourcePathMap() const;
 
   FileSpecList &GetExecutableSearchPaths();
 
   FileSpecList &GetDebugFileSearchPaths();
 
+  FileSpec &GetClangModulesCachePath();
+
   FileSpecList &GetClangModuleSearchPaths();
 
   bool GetEnableAutoImportClangModules() const;
 
   bool GetEnableAutoApplyFixIts() const;
 
   bool GetEnableNotifyAboutFixIts() const;
 

packages/Python/lldbsuite/test/lldbtest.py

def setUp(self):
             # build a framework, and fallback to standard *nix behavior by
             # setting framework_dir and dsym to None.
             if not os.path.exists(self.dsym):
                 self.framework_dir = None
                 self.dsym = None
                 self.darwinWithFramework = False
         self.makeBuildDir()
 
+        # set the clang modules cache path.
+        mod_cache = os.path.join(self.getBuildDir(), "module-cache")
+        self.runCmd("settings set target.clang-modules-cache-path " + mod_cache)
+
     def setAsync(self, value):
         """ Sets async mode to True/False and ensures it is reset after the testcase completes."""
         old_async = self.dbg.GetAsync()
         self.dbg.SetAsync(value)
         self.addTearDownHook(lambda: self.dbg.SetAsync(old_async))
 
     def cleanupSubprocesses(self):
         # Ensure any subprocesses are cleaned up

source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

ClangModulesDeclVendor::Create(Target &target) {
   target.GetPlatform()->AddClangModuleCompilationOptions(
       &target, compiler_invocation_arguments);
 
   compiler_invocation_arguments.push_back(ModuleImportBufferName);
 
   // Add additional search paths with { "-I", path } or { "-F", path } here.
 
   {
-    llvm::SmallString<128> DefaultModuleCache;
+    llvm::SmallString<128> Path;
+    target.GetClangModulesCachePath().GetPath(Path);
+    if (Path.empty()) {
+      // This code is copied from the Clang driver.
       const bool erased_on_reboot = false;
-    llvm::sys::path::system_temp_directory(erased_on_reboot,
-                                           DefaultModuleCache);
-    llvm::sys::path::append(DefaultModuleCache, "org.llvm.clang");
-    llvm::sys::path::append(DefaultModuleCache, "ModuleCache");
+      llvm::sys::path::system_temp_directory(erased_on_reboot, Path);
+      llvm::sys::path::append(Path, "org.llvm.clang");
+      llvm::sys::path::append(Path, "ModuleCache");
+    }
     std::string module_cache_argument("-fmodules-cache-path=");
-    module_cache_argument.append(DefaultModuleCache.str().str());
+    module_cache_argument.append(Path.str());
     compiler_invocation_arguments.push_back(module_cache_argument);
   }
 
   FileSpecList &module_search_paths = target.GetClangModuleSearchPaths();
 
   for (size_t spi = 0, spe = module_search_paths.GetSize(); spi < spe; ++spi) {
     const FileSpec &search_path = module_search_paths.GetFileSpecAtIndex(spi);
 

source/Target/Target.cpp

static PropertyDefinition g_properties[] = {
     {"display-expression-in-crashlogs", OptionValue::eTypeBoolean, false, false,
      nullptr, nullptr, "Expressions that crash will show up in crash logs if "
                        "the host system supports executable specific crash log "
                        "strings and this setting is set to true."},
     {"trap-handler-names", OptionValue::eTypeArray, true,
      OptionValue::eTypeString, nullptr, nullptr,
      "A list of trap handler function names, e.g. a common Unix user process "
      "one is _sigtramp."},
+    {"clang-modules-cache-path",
+     OptionValue::eTypeFileSpec, false, 0, nullptr, nullptr,
+     "The path to the clang modules cache directory (-fmodules-cache-path)."},
     {"display-runtime-support-values", OptionValue::eTypeBoolean, false, false,
      nullptr, nullptr, "If true, LLDB will show variables that are meant to "
                        "support the operation of a language's runtime "
                        "support."},
     {"non-stop-mode", OptionValue::eTypeBoolean, false, 0, nullptr, nullptr,
      "Disable lock-step debugging, instead control threads independently."},
     {nullptr, OptionValue::eTypeInvalid, false, 0, nullptr, nullptr, nullptr}};
 
enum {
   ePropertyUseHexImmediates,
   ePropertyHexImmediateStyle,
   ePropertyUseFastStepping,
   ePropertyLoadScriptFromSymbolFile,
   ePropertyLoadCWDlldbinitFile,
   ePropertyMemoryModuleLoadLevel,
   ePropertyDisplayExpressionsInCrashlogs,
   ePropertyTrapHandlerNames,
+  ePropertyClangModulesCachePath,
   ePropertyDisplayRuntimeSupportValues,
   ePropertyNonStopModeEnabled,
   ePropertyExperimental
 };
 
 class TargetOptionValueProperties : public OptionValueProperties {
 public:
   TargetOptionValueProperties(const ConstString &name)
FileSpecList &TargetProperties::GetDebugFileSearchPaths() {
   const uint32_t idx = ePropertyDebugFileSearchPaths;
   OptionValueFileSpecList *option_value =
       m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
                                                                    false, idx);
   assert(option_value);
   return option_value->GetCurrentValue();
 }
 
+FileSpec &TargetProperties::GetClangModulesCachePath() {
+  const uint32_t idx = ePropertyClangModulesCachePath;
+  OptionValueFileSpec *option_value =
+      m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpec(nullptr, false,
+                                                               idx);
+  assert(option_value);
+  return option_value->GetCurrentValue();
+}
+
 FileSpecList &TargetProperties::GetClangModuleSearchPaths() {
   const uint32_t idx = ePropertyClangModuleSearchPaths;
   OptionValueFileSpecList *option_value =
       m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
                                                                    false, idx);
   assert(option_value);
   return option_value->GetCurrentValue();
 }