[Lldb-commits] [lldb] [lldb][PlatformDarwin][NFC] Factor sanitization of Python module names into helper function (PR #185627)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

<details>
<summary>Changes</summary>

I'm planning on re-using this logic for another API. This patch creates a `SanitizedScriptingModuleName` that encapsulates the logic that checks whether a file name would fail to be loaded by a `ScriptInterpreter`. I called it something more generic despite it being `Python` specific at the moment, in case the FIXME is eventually going to be addressed.

We have existing unit-tests that check this logic, so I'm relying on that test coverage to give us confidence that this still works as expected.

---
Full diff: https://github.com/llvm/llvm-project/pull/185627.diff


1 Files Affected:

- (modified) lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp (+60-25) 


``````````diff
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
index a7e28eb1bcc7e..62c1c7c017b4e 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -80,6 +80,59 @@ static Status ExceptionMaskValidator(const char *string, void *unused) {
   return {};
 }
 
+namespace {
+/// Holds an lldb_private::Module name and a "sanitized" version
+/// of it for the purposes of loading a script of that name by
+/// the relevant ScriptInterpreter.
+///
+/// E.g., for Python the sanitized name can't include:
+/// * Special characters: '-', ' ', '.'
+/// * Python keywords
+class SanitizedScriptingModuleName {
+public:
+  SanitizedScriptingModuleName(llvm::StringRef name,
+                               ScriptInterpreter *script_interpreter)
+      : m_original_name(name), m_sanitized_name(name.str()) {
+    // FIXME: for Python, don't allow certain characters in imported module
+    // filenames. Theoretically, different scripting languages may have
+    // different sets of forbidden tokens in filenames, and that should
+    // be dealt with by each ScriptInterpreter. For now, just replace dots
+    // with underscores. In order to support anything other than Python
+    // this will need to be reworked.
+    llvm::replace(m_sanitized_name, '.', '_');
+    llvm::replace(m_sanitized_name, ' ', '_');
+    llvm::replace(m_sanitized_name, '-', '_');
+
+    if (script_interpreter &&
+        script_interpreter->IsReservedWord(m_sanitized_name.c_str())) {
+      m_sanitized_name.insert(m_sanitized_name.begin(), '_');
+      m_name_is_keyword = true;
+    }
+  }
+
+  /// Returns \c true if this name is a keyword in the associated scripting
+  /// language.
+  bool IsKeyword() const { return m_name_is_keyword; }
+
+  /// Returns \c true if the original name has been sanitized (i.e., required
+  /// changes).
+  bool RequiredSanitization() const {
+    return m_sanitized_name != m_original_name;
+  }
+
+  llvm::StringRef GetSanitizedName() const { return m_sanitized_name; }
+  llvm::StringRef GetOriginalName() const { return m_original_name; }
+
+private:
+  llvm::StringRef m_original_name;
+  std::string m_sanitized_name;
+
+  /// \c true if m_sanitized_name is a keyword for the ScriptInterpreter
+  /// language associated with this SanitizedScriptingModuleName.
+  bool m_name_is_keyword = false;
+};
+} // namespace
+
 /// Destructor.
 ///
 /// The destructor is virtual since this class is designed to be
@@ -201,27 +254,9 @@ FileSpecList PlatformDarwin::LocateExecutableScriptingResourcesFromDSYM(
     const FileSpec &symfile_spec) {
   FileSpecList file_list;
   while (module_spec.GetFilename()) {
-    std::string module_basename(module_spec.GetFilename().GetCString());
-    std::string original_module_basename(module_basename);
-
-    bool was_keyword = false;
-
-    // FIXME: for Python, don't allow certain characters in imported module
-    // filenames. Theoretically, different scripting languages may have
-    // different sets of forbidden tokens in filenames, and that should
-    // be dealt with by each ScriptInterpreter. For now, just replace dots
-    // with underscores. In order to support anything other than Python
-    // this will need to be reworked.
-    llvm::replace(module_basename, '.', '_');
-    llvm::replace(module_basename, ' ', '_');
-    llvm::replace(module_basename, '-', '_');
-    ScriptInterpreter *script_interpreter =
-        target.GetDebugger().GetScriptInterpreter();
-    if (script_interpreter &&
-        script_interpreter->IsReservedWord(module_basename.c_str())) {
-      module_basename.insert(module_basename.begin(), '_');
-      was_keyword = true;
-    }
+    SanitizedScriptingModuleName sanitized_name(
+        module_spec.GetFilename().GetStringRef(),
+        target.GetDebugger().GetScriptInterpreter());
 
     StreamString path_string;
     StreamString original_path_string;
@@ -231,10 +266,10 @@ FileSpecList PlatformDarwin::LocateExecutableScriptingResourcesFromDSYM(
     // file exists
     path_string.Format("{0}/../Python/{1}.py",
                        symfile_spec.GetDirectory().GetStringRef(),
-                       module_basename);
+                       sanitized_name.GetSanitizedName());
     original_path_string.Format("{0}/../Python/{1}.py",
                                 symfile_spec.GetDirectory().GetStringRef(),
-                                original_module_basename);
+                                sanitized_name.GetOriginalName());
 
     FileSpec script_fspec(path_string.GetString());
     FileSystem::Instance().Resolve(script_fspec);
@@ -244,9 +279,9 @@ FileSpecList PlatformDarwin::LocateExecutableScriptingResourcesFromDSYM(
     // if we did some replacements of reserved characters, and a
     // file with the untampered name exists, then warn the user
     // that the file as-is shall not be loaded
-    if (module_basename != original_module_basename &&
+    if (sanitized_name.RequiredSanitization() &&
         FileSystem::Instance().Exists(orig_script_fspec)) {
-      const char *reason_for_complaint = was_keyword
+      const char *reason_for_complaint = sanitized_name.IsKeyword()
                                              ? "conflicts with a keyword"
                                              : "contains reserved characters";
       if (FileSystem::Instance().Exists(script_fspec))

``````````

</details>


https://github.com/llvm/llvm-project/pull/185627