Tell the user which pathname was invalid...

[jingham]

Summary

When there's an error with the pathname in "command script import" lldb just says "error: module importing failed: invalid pathname" but doesn't actually tell you what the bad pathname was. When you're loading some python module that imports a bunch of other modules, that makes it hard to figure out which one was bad.

I changed it to either say "empty path" if that was the error, or report the pathname otherwise. I added a test as well.

[hawkinsw]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

2717

Sorry for dropping in unsolicited, but would this be another error that we would want to add more specificity for?

[DavidSpickett]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

2640

Is this testable? Maybe not because the top level command always wants some string, unless "" is accepted.

[jingham]

I added the pathname to the "invalid directory" output, but when the test passes an invalid directory we still execute the "invalid pathname" branch but not the "invalid directory" branch. I don't know enough about the llvm filesystem stuff to know why that is...

I did add a test for the "empty path" however.

[JDevlieghere]

LGTM

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

bool ScriptInterpreterPythonImpl::LoadScriptingModule(
   namespace fs = llvm::sys::fs;
   namespace path = llvm::sys::path;
 
   ExecuteScriptOptions exc_options = ExecuteScriptOptions()
                                          .SetEnableIO(!options.GetSilent())
                                          .SetSetLLDBGlobals(false);
 
   if (!pathname || !pathname[0]) {
-    error.SetErrorString("invalid pathname");
+    error.SetErrorString("empty path");
     return false;
   }
 
   llvm::Expected<std::unique_ptr<ScriptInterpreterIORedirect>>
       io_redirect_or_error = ScriptInterpreterIORedirect::Create(
           exc_options.GetEnableIO(), m_debugger, /*result=*/nullptr);
 
   if (!io_redirect_or_error) {
if (extra_search_dir) {
     std::error_code ec = status(module_file.GetPath(), st);
 
     if (ec || st.type() == fs::file_type::status_error ||
         st.type() == fs::file_type::type_unknown ||
         st.type() == fs::file_type::file_not_found) {
       // if not a valid file of any sort, check if it might be a filename still
       // dot can't be used but / and \ can, and if either is found, reject
       if (strchr(pathname, '\\') || strchr(pathname, '/')) {
-        error.SetErrorString("invalid pathname");
+        error.SetErrorStringWithFormatv("invalid pathname '{0}'", pathname);
         return false;
       }
       // Not a filename, probably a package of some sort, let it go through.
       possible_package = true;
     } else if (is_directory(st) || is_regular_file(st)) {
       if (module_file.GetDirectory().IsEmpty()) {
         error.SetErrorString("invalid directory name");
         return false;
       }
       if (llvm::Error e =
               ExtendSysPath(module_file.GetDirectory().GetCString())) {
         error = std::move(e);
         return false;
       }
       module_name = module_file.GetFilename().GetCString();

lldb/test/API/commands/command/script/import/TestImport.py

def run_test(self):
         self.addTearDownHook(cleanup)
 
         self.runCmd("command script import ./foo/foo.py --allow-reload")
         self.runCmd("command script import ./foo/foo2.py --allow-reload")
         self.runCmd("command script import ./foo/bar/foobar.py --allow-reload")
         self.runCmd("command script import ./bar/bar.py --allow-reload")
 
         self.expect("command script import ./nosuchfile.py",
-                    error=True, startstr='error: module importing failed')
+                    error=True, startstr="error: module importing failed: invalid pathname './nosuchfile.py'")
         self.expect("command script import ./nosuchfolder/",
                     error=True, startstr='error: module importing failed')
         self.expect("command script import ./foo/foo.py", error=False)
-
         self.runCmd("command script import --allow-reload ./thepackage")
         self.expect("TPcommandA", substrs=["hello world A"])
         self.expect("TPcommandB", substrs=["hello world B"])
 
         self.runCmd("script import dummymodule")
         self.expect("command script import ./dummymodule.py", error=False)
         self.expect(
             "command script import --allow-reload ./dummymodule.py",