[CommandInterpreter] Refactor SourceInitFile

[JDevlieghere]

Summary

I was looking at the current implementation of SourceInitFile and there were a few things that made this function hard to read. The code to find the ~/.lldbinit file is duplicated across the cwd and non-cwd branch. The ./.lldbinit is once computed by resolving .lldbinit and once by resolving ./.lldbinit. Furthermore it wasn't clear to me what happened when you're sourcing the .lldbinit file in the current working directory. Apparently we do nothing when we property to control that is set to warn (makes sense) and we don't care when the property is set to true (debatable). Finally, there were at least two branches where the status of the CommandReturnObject were not set.

Anyway, this is an attempt to simplify this code a bit. It's still more complex than I had hoped.

Please let me know if you think it's more readable or not.

[JDevlieghere]

lldb/source/Interpreter/CommandInterpreter.cpp

2118

This should also have an assert(!m_skip_lldbinit_files);

2163

This should be reflowed.

[labath]

I think this is a bit more readable. I've included some suggestions which I think could make it even better.

Since you're already rewriting this code, this might be a good time to raise a point I wanted to bring up some day. Should we be using FileSpec for code like this? The code already uses a combination of llvm and lldb path utilities, and I would argue that we should use llvm all the way for code like this (except that we go through the FileSystem abstraction for the reproducer stuff). I have two reasons for that:

• FileSpec is designed for efficient matching of abstract file paths which may not even exist on the host system. As such, this code will result in a bunch of strings being added to the global string pool for no reason. And in this case, you're definitely working files which do exist (or may exist) on the host. In fact, FileSpec now contains code which performs path simplifications which are not completely sound given a concrete filesystem -- it will simplify a path like bar/../foo to foo, which is not correct if bar is a symlink.
• Since we started supporting windows paths, the FileSpec class offers more freedom than it is needed here. Specifically, it gives you the ability to return a foreign-syntax path as the "lldbinit" location. Therefore, in the spirit of using the most specific type which is sufficient to accomplish a given task, I think we should use a plain string here.

lldb/source/Interpreter/CommandInterpreter.cpp

2149–2150

Make this a switch ?

2163

What might help readability is moving the long block of text to a separate static variable declared before the function. That way the text does not obscure the logic of the function.

2191–2196

This should be done with strings and stringrefs

[xiaobai]

Seems like an overall improvement to the structure of this code. At the high level, the structure feels easier to understand.

In D61994#1504336, @labath wrote:

I think this is a bit more readable. I've included some suggestions which I think could make it even better.

Since you're already rewriting this code, this might be a good time to raise a point I wanted to bring up some day. Should we be using FileSpec for code like this? The code already uses a combination of llvm and lldb path utilities, and I would argue that we should use llvm all the way for code like this (except that we go through the FileSystem abstraction for the reproducer stuff). I have two reasons for that:

• FileSpec is designed for efficient matching of abstract file paths which may not even exist on the host system. As such, this code will result in a bunch of strings being added to the global string pool for no reason. And in this case, you're definitely working files which do exist (or may exist) on the host. In fact, FileSpec now contains code which performs path simplifications which are not completely sound given a concrete filesystem -- it will simplify a path like bar/../foo to foo, which is not correct if bar is a symlink.

+1

• Since we started supporting windows paths, the FileSpec class offers more freedom than it is needed here. Specifically, it gives you the ability to return a foreign-syntax path as the "lldbinit" location. Therefore, in the spirit of using the most specific type which is sufficient to accomplish a given task, I think we should use a plain string here.

I'm especially concerned about windows support, so I also agree with this.

lldb/source/Interpreter/CommandInterpreter.cpp

2149–2150

+1

2163

I agree with Pavel, that would indeed make things more readable.

[JDevlieghere]

Address code review feedback

[labath]

I think this looks better than the original. Given that the only callers of SourceInitFile pass a constant value for the InitFileLocation argument, what I'd consider also doing is getting rid of the function and the enum, and making SourceInitFileHome/SourceInitFileCwd the main entry points.

One day I'm going to start a thread about reducing the use of FileSpecs in the FileSystem api...

[JDevlieghere]

Remove unneeded use of FileSpec. I wanted to do this yesterday but didn't get around to it...

lldb/include/lldb/Interpreter/CommandInterpreter.h

public:
   enum CommandTypes {
     eCommandTypesBuiltin = 0x0001, // native commands such as "frame"
     eCommandTypesUserDef = 0x0002, // scripted commands
     eCommandTypesAliases = 0x0004, // aliases such as "po"
     eCommandTypesHidden = 0x0008,  // commands prefixed with an underscore
     eCommandTypesAllThem = 0xFFFF  // all commands
   };
 
+  enum class InitFileLocation {
+    eHome, ///< ~/.lldbinit
+    eCwd,  ///< ./.lldbinit
+  };
+
   CommandInterpreter(Debugger &debugger, bool synchronous_execution);
 
   ~CommandInterpreter() override;
 
   // These two functions fill out the Broadcaster interface:
 
   static ConstString &GetStaticBroadcasterClass();
 
   ConstString &GetBroadcasterClass() const override {
     return GetStaticBroadcasterClass();
   }
 
-  void SourceInitFile(bool in_cwd, CommandReturnObject &result);
+  void SourceInitFile(InitFileLocation location, CommandReturnObject &result);
 
   bool AddCommand(llvm::StringRef name, const lldb::CommandObjectSP &cmd_sp,
                   bool can_replace);
 
   bool AddUserCommand(llvm::StringRef name, const lldb::CommandObjectSP &cmd_sp,
                       bool can_replace);
 
   lldb::CommandObjectSP GetCommandSPExact(llvm::StringRef cmd,
lldb::CommandObjectSP GetCommandSP(llvm::StringRef cmd,
                                      bool include_aliases = true,
                                      bool exact = true,
                                      StringList *matches = nullptr,
                                      StringList *descriptions = nullptr) const;
 
 private:
   Status PreprocessCommand(std::string &command);
 
+  void SourceInitFileCwd(CommandReturnObject &result);
+  void SourceInitFileHome(CommandReturnObject &result);
+  void SourceInitFile(FileSpec file, CommandReturnObject &result);
+
   // Completely resolves aliases and abbreviations, returning a pointer to the
   // final command object and updating command_line to the fully substituted
   // and translated command.
   CommandObject *ResolveCommandImpl(std::string &command_line,
                                     CommandReturnObject &result);
 
   void FindCommandsForApropos(llvm::StringRef word, StringList &commands_found,
                               StringList &commands_help,

lldb/source/API/SBCommandInterpreter.cpp

LLDB_RECORD_METHOD(void, SBCommandInterpreter, SourceInitFileInHomeDirectory,
                      (lldb::SBCommandReturnObject &), result);
 
   result.Clear();
   if (IsValid()) {
     TargetSP target_sp(m_opaque_ptr->GetDebugger().GetSelectedTarget());
     std::unique_lock<std::recursive_mutex> lock;
     if (target_sp)
       lock = std::unique_lock<std::recursive_mutex>(target_sp->GetAPIMutex());
-    m_opaque_ptr->SourceInitFile(false, result.ref());
+    m_opaque_ptr->SourceInitFile(CommandInterpreter::InitFileLocation::eHome,
+                                 result.ref());
   } else {
     result->AppendError("SBCommandInterpreter is not valid");
     result->SetStatus(eReturnStatusFailed);
   }
 }
 
 void SBCommandInterpreter::SourceInitFileInCurrentWorkingDirectory(
     SBCommandReturnObject &result) {
   LLDB_RECORD_METHOD(void, SBCommandInterpreter,
                      SourceInitFileInCurrentWorkingDirectory,
                      (lldb::SBCommandReturnObject &), result);
 
   result.Clear();
   if (IsValid()) {
     TargetSP target_sp(m_opaque_ptr->GetDebugger().GetSelectedTarget());
     std::unique_lock<std::recursive_mutex> lock;
     if (target_sp)
       lock = std::unique_lock<std::recursive_mutex>(target_sp->GetAPIMutex());
-    m_opaque_ptr->SourceInitFile(true, result.ref());
+    m_opaque_ptr->SourceInitFile(CommandInterpreter::InitFileLocation::eCwd,
+                                 result.ref());
   } else {
     result->AppendError("SBCommandInterpreter is not valid");
     result->SetStatus(eReturnStatusFailed);
   }
 }
 
 SBBroadcaster SBCommandInterpreter::GetBroadcaster() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBBroadcaster, SBCommandInterpreter,

lldb/source/Interpreter/CommandInterpreter.cpp

 using namespace lldb_private;
 
 static const char *k_white_space = " \t\v";
 
 static constexpr bool NoGlobalSetting = true;
 static constexpr uintptr_t DefaultValueTrue = true;
 static constexpr uintptr_t DefaultValueFalse = false;
 static constexpr const char *NoCStrDefault = nullptr;
+static constexpr const char *InitFileWarning =
+    "There is a .lldbinit file in the current directory which is not being "
+    "read.\n"
+    "To silence this warning without sourcing in the local .lldbinit,\n"
+    "add the following to the lldbinit file in your home directory:\n"
+    "    settings set target.load-cwd-lldbinit false\n"
+    "To allow lldb to source .lldbinit files in the current working "
+    "directory,\n"
+    "set the value of this variable to true.  Only do so if you understand "
+    "and\n"
+    "accept the security risk.";
 
 static constexpr PropertyDefinition g_properties[] = {
     {"expand-regex-aliases", OptionValue::eTypeBoolean, NoGlobalSetting,
      DefaultValueFalse, NoCStrDefault, {},
      "If true, regular expression alias commands will show the "
      "expanded command that will be executed. This can be used to "
      "debug new regular expression alias commands."},
     {"prompt-on-quit", OptionValue::eTypeBoolean, NoGlobalSetting,

       if (cptr[0] == '\0')
         position = atoi(start);
     }
   }
 
   return position;
 }
 
-void CommandInterpreter::SourceInitFile(bool in_cwd,
+static FileSpec GetHomeInitFile(llvm::StringRef suffix = {}) {
+  std::string init_file_name = ".lldbinit";
+  if (!suffix.empty()) {
+    init_file_name.append("-");
+    init_file_name.append(suffix.str());
+  }
+
+  llvm::SmallString<128> buffer;
+  llvm::sys::path::home_directory(buffer);
+  llvm::sys::path::append(buffer, init_file_name);
+
+  FileSpec init_file(buffer.c_str());
+  FileSystem::Instance().Resolve(init_file);
+  return init_file;
+}
+
+static FileSpec GetCwdInitFile() {
+  FileSpec init_file(".lldbinit");
+  FileSystem::Instance().Resolve(init_file);
+  return init_file;
+}
+
+static LoadCWDlldbinitFile ShouldLoadCwdInitFile() {
+  lldb::TargetPropertiesSP properties = Target::GetGlobalProperties();
+  if (!properties)
+    return eLoadCWDlldbinitFalse;
+  return properties->GetLoadCWDlldbinitFile();
+}
+
+void CommandInterpreter::SourceInitFile(FileSpec file,
                                         CommandReturnObject &result) {
-  FileSpec init_file;
-  if (in_cwd) {
-    lldb::TargetPropertiesSP properties = Target::GetGlobalProperties();
-    if (properties) {
-      // In the current working directory we don't load any program specific
-      // .lldbinit files, we only look for a ".lldbinit" file.
-      if (m_skip_lldbinit_files)
-        return;
+  assert(!m_skip_lldbinit_files);
 
-      LoadCWDlldbinitFile should_load = properties->GetLoadCWDlldbinitFile();
-      if (should_load == eLoadCWDlldbinitWarn) {
-        FileSpec dot_lldb(".lldbinit");
-        FileSystem::Instance().Resolve(dot_lldb);
-        llvm::SmallString<64> home_dir_path;
-        llvm::sys::path::home_directory(home_dir_path);
-        FileSpec homedir_dot_lldb(home_dir_path.c_str());
-        homedir_dot_lldb.AppendPathComponent(".lldbinit");
-        FileSystem::Instance().Resolve(homedir_dot_lldb);
-        if (FileSystem::Instance().Exists(dot_lldb) &&
-            dot_lldb.GetDirectory() != homedir_dot_lldb.GetDirectory()) {
-          result.AppendErrorWithFormat(
-              "There is a .lldbinit file in the current directory which is not "
-              "being read.\n"
-              "To silence this warning without sourcing in the local "
-              ".lldbinit,\n"
-              "add the following to the lldbinit file in your home directory:\n"
-              "    settings set target.load-cwd-lldbinit false\n"
-              "To allow lldb to source .lldbinit files in the current working "
-              "directory,\n"
-              "set the value of this variable to true.  Only do so if you "
-              "understand and\n"
-              "accept the security risk.");
-          result.SetStatus(eReturnStatusFailed);
-          return;
-        }
-      } else if (should_load == eLoadCWDlldbinitTrue) {
-        init_file.SetFile("./.lldbinit", FileSpec::Style::native);
-        FileSystem::Instance().Resolve(init_file);
-      }
-    }
-  } else {
-    // If we aren't looking in the current working directory we are looking in
-    // the home directory. We will first see if there is an application
-    // specific ".lldbinit" file whose name is "~/.lldbinit" followed by a "-"
-    // and the name of the program. If this file doesn't exist, we fall back to
-    // just the "~/.lldbinit" file. We also obey any requests to not load the
-    // init files.
-    llvm::SmallString<64> home_dir_path;
-    llvm::sys::path::home_directory(home_dir_path);
-    FileSpec profilePath(home_dir_path.c_str());
-    profilePath.AppendPathComponent(".lldbinit");
-    std::string init_file_path = profilePath.GetPath();
-
-    if (!m_skip_app_init_files) {
-      FileSpec program_file_spec(HostInfo::GetProgramFileSpec());
-      const char *program_name = program_file_spec.GetFilename().AsCString();
-
-      if (program_name) {
-        char program_init_file_name[PATH_MAX];
-        ::snprintf(program_init_file_name, sizeof(program_init_file_name),
-                   "%s-%s", init_file_path.c_str(), program_name);
-        init_file.SetFile(program_init_file_name, FileSpec::Style::native);
-        FileSystem::Instance().Resolve(init_file);
-        if (!FileSystem::Instance().Exists(init_file))
-          init_file.Clear();
-      }
+  if (!FileSystem::Instance().Exists(file)) {
+    result.SetStatus(eReturnStatusSuccessFinishNoResult);
+    return;
+  }
+
+  // Use HandleCommand to 'source' the given file; this will do the actual
+  // broadcasting of the commands back to any appropriate listener (see
+  // CommandObjectSource::Execute for more details).
+  const bool saved_batch = SetBatchCommandMode(true);
+  ExecutionContext *ctx = nullptr;
+  CommandInterpreterRunOptions options;
+  options.SetSilent(true);
+  options.SetPrintErrors(true);
+  options.SetStopOnError(false);
+  options.SetStopOnContinue(true);
+  HandleCommandsFromFile(file, ctx, options, result);
+  SetBatchCommandMode(saved_batch);
+}
+
+void CommandInterpreter::SourceInitFileCwd(CommandReturnObject &result) {
+  assert(!m_skip_lldbinit_files);
+
+  FileSpec init_file = GetCwdInitFile();
+  if (!FileSystem::Instance().Exists(init_file)) {
+    result.SetStatus(eReturnStatusSuccessFinishNoResult);
+    return;
+  }
+
+  LoadCWDlldbinitFile should_load = ShouldLoadCwdInitFile();
+
+  switch (should_load) {
+  case eLoadCWDlldbinitFalse:
+    result.SetStatus(eReturnStatusSuccessFinishNoResult);
+    break;
+  case eLoadCWDlldbinitTrue:
+    SourceInitFile(init_file, result);
+    break;
+  case eLoadCWDlldbinitWarn: {
+    FileSpec home_init_file = GetHomeInitFile();
+    if (init_file.GetDirectory() == home_init_file.GetDirectory()) {
+      result.SetStatus(eReturnStatusSuccessFinishNoResult);
+    } else {
+      result.AppendErrorWithFormat(InitFileWarning);
+      result.SetStatus(eReturnStatusFailed);
     }
+  }
+  }
+}
+
+/// We will first see if there is an application specific ".lldbinit" file
+/// whose name is "~/.lldbinit" followed by a "-" and the name of the program.
+/// If this file doesn't exist, we fall back to just the "~/.lldbinit" file.
+void CommandInterpreter::SourceInitFileHome(CommandReturnObject &result) {
+  assert(!m_skip_lldbinit_files);
+  FileSpec init_file = GetHomeInitFile();
 
-    if (!init_file && !m_skip_lldbinit_files)
-      init_file.SetFile(init_file_path, FileSpec::Style::native);
+  if (!m_skip_app_init_files) {
+    llvm::StringRef program_name =
+        HostInfo::GetProgramFileSpec().GetFilename().GetStringRef();
+    FileSpec program_init_file = GetHomeInitFile(program_name);
+    if (FileSystem::Instance().Exists(program_init_file))
+      init_file = program_init_file;
   }
 
-  // If the file exists, tell HandleCommand to 'source' it; this will do the
-  // actual broadcasting of the commands back to any appropriate listener (see
-  // CommandObjectSource::Execute for more details).
+  SourceInitFile(init_file, result);
+}
 
-  if (FileSystem::Instance().Exists(init_file)) {
-    const bool saved_batch = SetBatchCommandMode(true);
-    CommandInterpreterRunOptions options;
-    options.SetSilent(true);
-    options.SetPrintErrors(true);
-    options.SetStopOnError(false);
-    options.SetStopOnContinue(true);
-
-    HandleCommandsFromFile(init_file,
-                           nullptr, // Execution context
-                           options, result);
-    SetBatchCommandMode(saved_batch);
-  } else {
-    // nothing to be done if the file doesn't exist
+void CommandInterpreter::SourceInitFile(InitFileLocation location,
+                                        CommandReturnObject &result) {
+  if (m_skip_lldbinit_files) {
     result.SetStatus(eReturnStatusSuccessFinishNoResult);
+    return;
+  }
+
+  switch (location) {
+  case InitFileLocation::eHome:
+    SourceInitFileHome(result);
+    break;
+  case InitFileLocation::eCwd:
+    SourceInitFileCwd(result);
+    break;
   }
 }
 
 const char *CommandInterpreter::GetCommandPrefix() {
   const char *prefix = GetDebugger().GetIOHandlerCommandPrefix();
   return prefix == NULL ? "" : prefix;
 }