[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 ↗

(On Diff #199753)

This should also have an assert(!m_skip_lldbinit_files);

2163 ↗

(On Diff #199753)

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 ↗

(On Diff #199753)

Make this a switch ?

2163 ↗

(On Diff #199753)

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 ↗

(On Diff #199753)

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 ↗

(On Diff #199753)

+1

2163 ↗

(On Diff #199753)

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/trunk/include/lldb/Interpreter/CommandInterpreter.h

public:
   // 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 SourceInitFileCwd(CommandReturnObject &result);
+  void SourceInitFileHome(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 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/trunk/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->SourceInitFileHome(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->SourceInitFileCwd(result.ref());
   } else {
     result->AppendError("SBCommandInterpreter is not valid");
     result->SetStatus(eReturnStatusFailed);
   }
 }
 
 SBBroadcaster SBCommandInterpreter::GetBroadcaster() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBBroadcaster, SBCommandInterpreter,

lldb/trunk/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 (isdigit(cptr[0])) {
       if (cptr[0] == '\0')
         position = atoi(start);
     }
   }
 
   return position;
 }
 
-void CommandInterpreter::SourceInitFile(bool in_cwd,
-                                        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;
-
-      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();
+static void GetHomeInitFile(llvm::SmallVectorImpl<char> &init_file,
+                            llvm::StringRef suffix = {}) {
+  std::string init_file_name = ".lldbinit";
+  if (!suffix.empty()) {
+    init_file_name.append("-");
+    init_file_name.append(suffix.str());
+
+  llvm::sys::path::home_directory(init_file);
+  llvm::sys::path::append(init_file, init_file_name);
+
 
-      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();
-      }
-    }
+static void GetCwdInitFile(llvm::SmallVectorImpl<char> &init_file) {
+  llvm::StringRef s = ".lldbinit";
+  init_file.assign(s.begin(), s.end());
+
+static LoadCWDlldbinitFile ShouldLoadCwdInitFile() {
+  lldb::TargetPropertiesSP properties = Target::GetGlobalProperties();
+  if (!properties)
+    return eLoadCWDlldbinitFalse;
+  return properties->GetLoadCWDlldbinitFile();
 
-    if (!init_file && !m_skip_lldbinit_files)
-      init_file.SetFile(init_file_path, FileSpec::Style::native);
+void CommandInterpreter::SourceInitFile(FileSpec file,
+                                        CommandReturnObject &result) {
+  assert(!m_skip_lldbinit_files);
+
+  if (!FileSystem::Instance().Exists(file)) {
+    result.SetStatus(eReturnStatusSuccessFinishNoResult);
+    return;
   }
 
-  // If the file exists, tell HandleCommand to 'source' it; this will do the
-  // actual broadcasting of the commands back to any appropriate listener (see
+  // 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).
-
-  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);
+  ExecutionContext *ctx = nullptr;
+  HandleCommandsFromFile(file, ctx, options, result);
+}
+
+void CommandInterpreter::SourceInitFileCwd(CommandReturnObject &result) {
+  if (m_skip_lldbinit_files) {
+    result.SetStatus(eReturnStatusSuccessFinishNoResult);
+    return;
+  }
+
+  llvm::SmallString<128> init_file;
+  GetCwdInitFile(init_file);
+  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(FileSpec(init_file.str()), result);
+    break;
+  case eLoadCWDlldbinitWarn: {
+    llvm::SmallString<128> home_init_file;
+    GetHomeInitFile(home_init_file);
+    if (llvm::sys::path::parent_path(init_file) ==
+        llvm::sys::path::parent_path(home_init_file)) {
+      result.SetStatus(eReturnStatusSuccessFinishNoResult);
     } else {
-    // nothing to be done if the file doesn't exist
+      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) {
+  if (m_skip_lldbinit_files) {
     result.SetStatus(eReturnStatusSuccessFinishNoResult);
+    return;
   }
+
+  llvm::SmallString<128> init_file;
+  GetHomeInitFile(init_file);
+
+  if (!m_skip_app_init_files) {
+    llvm::StringRef program_name =
+        HostInfo::GetProgramFileSpec().GetFilename().GetStringRef();
+    llvm::SmallString<128> program_init_file;
+    GetHomeInitFile(program_init_file, program_name);
+    if (FileSystem::Instance().Exists(program_init_file))
+      init_file = program_init_file;
+  }
+
+  SourceInitFile(FileSpec(init_file.str()), result);
 }
 
 const char *CommandInterpreter::GetCommandPrefix() {
   const char *prefix = GetDebugger().GetIOHandlerCommandPrefix();
   return prefix == NULL ? "" : prefix;
 }
 
 PlatformSP CommandInterpreter::GetPlatform(bool prefer_target_platform) {