Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb/Interpreter] Make Scripted*Interface base class abstract #71465

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

medismailben
Copy link
Member

@medismailben medismailben commented Nov 7, 2023

This patch makes the various Scripted Interface base class abstract by making the CreatePluginObject method pure virtual.

This means that we cannot construct a Scripted Interface base class instance, so this patch also updates the various ScriptedInterpreter::CreateScripted*Interface methods to return a nullptr instead.`

This patch also removes the ScriptedPlatformInterface member from the ScriptInterpreter class since it the interpreter can be owned by the ScriptedPlatform instance itself, like we do for ScriptedProcess objects.

@llvmbot
Copy link
Member

llvmbot commented Nov 7, 2023

@llvm/pr-subscribers-lldb

Author: Med Ismail Bennani (medismailben)

Changes

This patch makes the various Scripted Interface base class abstract by making the CreatePluginObject method pure virtual.

This means that we cannot construct a Scripted Interface base class instance, so this patch also updates the various ScriptedInterpreter::CreateScripted*Interface methods to return a nullptr instead.`


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

6 Files Affected:

  • (modified) lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h (+1-3)
  • (modified) lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h (+1-3)
  • (modified) lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h (+1-3)
  • (modified) lldb/include/lldb/Interpreter/ScriptInterpreter.h (+6-10)
  • (modified) lldb/source/Interpreter/ScriptInterpreter.cpp (+3-6)
  • (modified) lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp (-2)
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
index 2dcbb47ffa6de85..7feaa01fe89b860 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
@@ -22,9 +22,7 @@ class ScriptedPlatformInterface : virtual public ScriptedInterface {
   virtual llvm::Expected<StructuredData::GenericSP>
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
                      StructuredData::DictionarySP args_sp,
-                     StructuredData::Generic *script_obj = nullptr) {
-    return {llvm::make_error<UnimplementedError>()};
-  }
+                     StructuredData::Generic *script_obj = nullptr) = 0;
 
   virtual StructuredData::DictionarySP ListProcesses() { return {}; }
 
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
index a429cacd862f121..10203b1f8baa7aa 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
@@ -24,9 +24,7 @@ class ScriptedProcessInterface : virtual public ScriptedInterface {
   virtual llvm::Expected<StructuredData::GenericSP>
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
                      StructuredData::DictionarySP args_sp,
-                     StructuredData::Generic *script_obj = nullptr) {
-    return {llvm::make_error<UnimplementedError>()};
-  }
+                     StructuredData::Generic *script_obj = nullptr) = 0;
 
   virtual StructuredData::DictionarySP GetCapabilities() { return {}; }
 
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h
index 107e593b5561ef7..a7cfc690b67dc74 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h
@@ -23,9 +23,7 @@ class ScriptedThreadInterface : virtual public ScriptedInterface {
   virtual llvm::Expected<StructuredData::GenericSP>
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
                      StructuredData::DictionarySP args_sp,
-                     StructuredData::Generic *script_obj = nullptr) {
-    return {llvm::make_error<UnimplementedError>()};
-  }
+                     StructuredData::Generic *script_obj = nullptr) = 0;
 
   virtual lldb::tid_t GetThreadID() { return LLDB_INVALID_THREAD_ID; }
 
diff --git a/lldb/include/lldb/Interpreter/ScriptInterpreter.h b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
index 0146eeb86262003..b941f6012a117b6 100644
--- a/lldb/include/lldb/Interpreter/ScriptInterpreter.h
+++ b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
@@ -151,10 +151,7 @@ class ScriptInterpreter : public PluginInterface {
     eScriptReturnTypeOpaqueObject
   };
 
-  ScriptInterpreter(
-      Debugger &debugger, lldb::ScriptLanguage script_lang,
-      lldb::ScriptedPlatformInterfaceUP scripted_platform_interface_up =
-          std::make_unique<ScriptedPlatformInterface>());
+  ScriptInterpreter(Debugger &debugger, lldb::ScriptLanguage script_lang);
 
   virtual StructuredData::DictionarySP GetInterpreterInfo();
 
@@ -559,19 +556,19 @@ class ScriptInterpreter : public PluginInterface {
   lldb::ScriptLanguage GetLanguage() { return m_script_lang; }
 
   virtual lldb::ScriptedProcessInterfaceUP CreateScriptedProcessInterface() {
-    return std::make_unique<ScriptedProcessInterface>();
+    return {};
   }
 
   virtual lldb::ScriptedThreadInterfaceSP CreateScriptedThreadInterface() {
-    return std::make_shared<ScriptedThreadInterface>();
+    return {};
   }
 
   virtual lldb::OperatingSystemInterfaceSP CreateOperatingSystemInterface() {
-    return std::make_shared<OperatingSystemInterface>();
+    return {};
   }
 
-  ScriptedPlatformInterface &GetScriptedPlatformInterface() {
-    return *m_scripted_platform_interface_up;
+  virtual lldb::ScriptedPlatformInterfaceUP GetScriptedPlatformInterface() {
+    return {};
   }
 
   virtual StructuredData::ObjectSP
@@ -599,7 +596,6 @@ class ScriptInterpreter : public PluginInterface {
 protected:
   Debugger &m_debugger;
   lldb::ScriptLanguage m_script_lang;
-  lldb::ScriptedPlatformInterfaceUP m_scripted_platform_interface_up;
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Interpreter/ScriptInterpreter.cpp b/lldb/source/Interpreter/ScriptInterpreter.cpp
index fb3fa74d0b97804..8dd499ce819a789 100644
--- a/lldb/source/Interpreter/ScriptInterpreter.cpp
+++ b/lldb/source/Interpreter/ScriptInterpreter.cpp
@@ -27,12 +27,9 @@
 using namespace lldb;
 using namespace lldb_private;
 
-ScriptInterpreter::ScriptInterpreter(
-    Debugger &debugger, lldb::ScriptLanguage script_lang,
-    lldb::ScriptedPlatformInterfaceUP scripted_platform_interface_up)
-    : m_debugger(debugger), m_script_lang(script_lang),
-      m_scripted_platform_interface_up(
-          std::move(scripted_platform_interface_up)) {}
+ScriptInterpreter::ScriptInterpreter(Debugger &debugger,
+                                     lldb::ScriptLanguage script_lang)
+    : m_debugger(debugger), m_script_lang(script_lang) {}
 
 void ScriptInterpreter::CollectDataForBreakpointCommandCallback(
     std::vector<std::reference_wrapper<BreakpointOptions>> &bp_options_vec,
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index 953f8b3aba18f79..ef7a2c128a22074 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -427,8 +427,6 @@ ScriptInterpreterPythonImpl::ScriptInterpreterPythonImpl(Debugger &debugger)
       m_active_io_handler(eIOHandlerNone), m_session_is_active(false),
       m_pty_secondary_is_open(false), m_valid_session(true), m_lock_count(0),
       m_command_thread_state(nullptr) {
-  m_scripted_platform_interface_up =
-      std::make_unique<ScriptedPlatformPythonInterface>(*this);
 
   m_dictionary_name.append("_dict");
   StreamString run_string;

Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This more closely matches my intuition for how I would expect this inheritance hierarchy to be set up.

If I wanted to add ScriptedLuaProcessInterface (as an example), what would I need to do?

@medismailben
Copy link
Member Author

If I wanted to add ScriptedLuaProcessInterface (as an example), what would I need to do?

In the current state, you'd just need to implement at lease ScriptedProcessLuaInterface::CreatePluginObject to construct the object. But after I land #71260, you'll also have to implement ScriptedProcessLuaInterface::GetAbstractMethods.

@medismailben medismailben force-pushed the abstract-base-scripted-class branch from e34f071 to 54bde6b Compare November 7, 2023 05:25
This patch makes the various Scripted Interface base class abstract by
making the `CreatePluginObject` method pure virtual.

This means that we cannot construct a Scripted Interface base class
instance, so this patch also updates the various
`ScriptedInterpreter::CreateScripted*Interface` methods to return a
`nullptr` instead.`

This patch also removes the `ScriptedPlatformInterface` member from the
`ScriptInterpreter` class since it the interpreter can be owned by the
`ScriptedPlatform` instance itself, like we do for `ScriptedProcess` objects.

Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
@medismailben medismailben force-pushed the abstract-base-scripted-class branch from 54bde6b to 2267c5f Compare November 7, 2023 17:45
@medismailben medismailben merged commit 7991412 into llvm:main Nov 7, 2023
3 checks passed
@luporl
Copy link
Contributor

luporl commented Nov 8, 2023

It seems this broke the lldb-aarch64-windows buildbot:
https://lab.llvm.org/buildbot/#/builders/219/builds/6801

@medismailben
Copy link
Member Author

medismailben commented Nov 8, 2023

It seems this broke the lldb-aarch64-windows buildbot: https://lab.llvm.org/buildbot/#/builders/219/builds/6801

@luporl No, this didn't break that bot, Expr/TestIRMemoryMapWindows.test was already failing in https://lab.llvm.org/buildbot/#/builders/219/builds/6786

@luporl
Copy link
Contributor

luporl commented Nov 8, 2023

@medismailben Yes, it did break that bot.
https://lab.llvm.org/buildbot/#/builders/219/builds/6786 is the first build that includes your commit and when Expr/TestIRMemoryMapWindows.test started failing.
https://lab.llvm.org/buildbot/#/builders/219/builds/6785 is a build failure that was fixed by 3267cd3.

To confirm it, I've forced builds of particular revisions. https://lab.llvm.org/buildbot/#/builders/219/builds/6800 is right before your commit and passes and https://lab.llvm.org/buildbot/#/builders/219/builds/6801 adds only your commit and fails.

@medismailben
Copy link
Member Author

medismailben commented Nov 8, 2023

@medismailben Yes, it did break that bot. https://lab.llvm.org/buildbot/#/builders/219/builds/6786 is the first build that includes your commit and when Expr/TestIRMemoryMapWindows.test started failing. https://lab.llvm.org/buildbot/#/builders/219/builds/6785 is a build failure that was fixed by 3267cd3.

To confirm it, I've forced builds of particular revisions. https://lab.llvm.org/buildbot/#/builders/219/builds/6800 is right before your commit and passes and https://lab.llvm.org/buildbot/#/builders/219/builds/6801 adds only your commit and fails.

Ok, I'll take a look

@medismailben
Copy link
Member Author

@luporl I found the issue. #71744 should fix it.

@luporl
Copy link
Contributor

luporl commented Nov 9, 2023

Thanks for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants