Reland "[lldb] Reland 2402b3213c2f with /H to debug the windows build issue

[DavidSpickett]

This reverts commit 9effefb.

With the include order in ScriptedProcessPythonInterface.cpp fixed (though I cannot explain exactly why it works) and removes the /H flag intended for debugging this issue.

I think it is something to do with Process.h pulling in PosixApi.h somewhere along the line, and including Process.h after lldb-python.h means that NO_PID_T is defined to prevent a redefinition of pid_t.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: David Spickett (DavidSpickett)

Changes

This reverts commit 9effefb.

With the include order in ScriptedProcessPythonInterface.cpp fixed (though I cannot explain exactly why it works) and removes the /H flag intended for debugging this issue.

I think it is something to do with Process.h pulling in PosixApi.h somewhere along the line, and including Process.h after lldb-python.h means that NO_PID_T is defined to prevent a redefinition of pid_t.

---

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

5 Files Affected:

• (modified) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/CMakeLists.txt (+1-1)
• (added) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/CMakeLists.txt (+16)
• (renamed) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/ScriptedProcessPythonInterface.cpp (+35-8)
• (renamed) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/ScriptedProcessPythonInterface.h (+15-3)
• (modified) lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp (+1-1)

diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/CMakeLists.txt b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/CMakeLists.txt
index 8c7e92bead32c..eb22a960b5345 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/CMakeLists.txt
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/CMakeLists.txt
@@ -21,7 +21,6 @@ endif()
 
 add_lldb_library(lldbPluginScriptInterpreterPythonInterfaces
   ScriptedPythonInterface.cpp
-  ScriptedProcessPythonInterface.cpp
   ScriptedThreadPythonInterface.cpp
 
   LINK_LIBS
@@ -38,5 +37,6 @@ add_lldb_library(lldbPluginScriptInterpreterPythonInterfaces
 
 add_subdirectory(OperatingSystemPythonInterface)
 add_subdirectory(ScriptedPlatformPythonInterface)
+add_subdirectory(ScriptedProcessPythonInterface)
 add_subdirectory(ScriptedThreadPlanPythonInterface)
 
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/CMakeLists.txt b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/CMakeLists.txt
new file mode 100644
index 0000000000000..66ed041853f67
--- /dev/null
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/CMakeLists.txt
@@ -0,0 +1,16 @@
+add_lldb_library(lldbPluginScriptInterpreterPythonScriptedProcessPythonInterface PLUGIN
+
+  ScriptedProcessPythonInterface.cpp
+
+  LINK_LIBS
+    lldbCore
+    lldbHost
+    lldbInterpreter
+    lldbTarget
+    lldbPluginScriptInterpreterPython
+    ${Python3_LIBRARIES}
+    ${LLDB_LIBEDIT_LIBS}
+
+  LINK_COMPONENTS
+    Support
+  )
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/ScriptedProcessPythonInterface.cpp
similarity index 84%
rename from lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.cpp
rename to lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/ScriptedProcessPythonInterface.cpp
index 313c597ce48f3..c744d7028d04e 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/ScriptedProcessPythonInterface.cpp
@@ -6,22 +6,27 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Host/Config.h"
-#if LLDB_ENABLE_PYTHON
-// LLDB Python header must be included first
-#include "../lldb-python.h"
-#endif
-#include "lldb/Target/Process.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-enumerations.h"
 
 #if LLDB_ENABLE_PYTHON
 
-#include "../SWIGPythonBridge.h"
-#include "../ScriptInterpreterPythonImpl.h"
+// clang-format off
+// LLDB Python header must be included first
+#include "../../lldb-python.h"
+
+#include "../../SWIGPythonBridge.h"
+#include "../../ScriptInterpreterPythonImpl.h"
+#include "../ScriptedThreadPythonInterface.h"
 #include "ScriptedProcessPythonInterface.h"
-#include "ScriptedThreadPythonInterface.h"
+
+// Included in this position to prevent redefinition of pid_t on Windows.
+#include "lldb/Target/Process.h"
+//clang-format off
+
 #include <optional>
 
 using namespace lldb;
@@ -29,6 +34,8 @@ using namespace lldb_private;
 using namespace lldb_private::python;
 using Locker = ScriptInterpreterPythonImpl::Locker;
 
+LLDB_PLUGIN_DEFINE_ADV(ScriptedProcessPythonInterface, ScriptInterpreterPythonScriptedProcessPythonInterface)
+
 ScriptedProcessPythonInterface::ScriptedProcessPythonInterface(
     ScriptInterpreterPythonImpl &interpreter)
     : ScriptedProcessInterface(), ScriptedPythonInterface(interpreter) {}
@@ -208,4 +215,24 @@ StructuredData::DictionarySP ScriptedProcessPythonInterface::GetMetadata() {
   return dict;
 }
 
+void ScriptedProcessPythonInterface::Initialize() {
+  const std::vector<llvm::StringRef> ci_usages = {
+      "process attach -C <script-name> [-k key -v value ...]",
+      "process launch -C <script-name> [-k key -v value ...]"};
+  const std::vector<llvm::StringRef> api_usages = {
+      "SBAttachInfo.SetScriptedProcessClassName",
+      "SBAttachInfo.SetScriptedProcessDictionary",
+      "SBTarget.Attach",
+      "SBLaunchInfo.SetScriptedProcessClassName",
+      "SBLaunchInfo.SetScriptedProcessDictionary",
+      "SBTarget.Launch"};
+  PluginManager::RegisterPlugin(
+      GetPluginNameStatic(), llvm::StringRef("Mock process state"),
+      CreateInstance, eScriptLanguagePython, {ci_usages, api_usages});
+}
+
+void ScriptedProcessPythonInterface::Terminate() {
+  PluginManager::UnregisterPlugin(CreateInstance);
+}
+
 #endif
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/ScriptedProcessPythonInterface.h
similarity index 88%
rename from lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h
rename to lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/ScriptedProcessPythonInterface.h
index c75caa9340f25..bb27734739f43 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface/ScriptedProcessPythonInterface.h
@@ -10,16 +10,18 @@
 #define LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_INTERFACES_SCRIPTEDPROCESSPYTHONINTERFACE_H
 
 #include "lldb/Host/Config.h"
+#include "lldb/Interpreter/Interfaces/ScriptedProcessInterface.h"
 
 #if LLDB_ENABLE_PYTHON
 
-#include "ScriptedPythonInterface.h"
-#include "lldb/Interpreter/Interfaces/ScriptedProcessInterface.h"
+#include "../ScriptedPythonInterface.h"
+
 #include <optional>
 
 namespace lldb_private {
 class ScriptedProcessPythonInterface : public ScriptedProcessInterface,
-                                       public ScriptedPythonInterface {
+                                       public ScriptedPythonInterface,
+                                       public PluginInterface {
 public:
   ScriptedProcessPythonInterface(ScriptInterpreterPythonImpl &interpreter);
 
@@ -67,6 +69,16 @@ class ScriptedProcessPythonInterface : public ScriptedProcessInterface,
 
   StructuredData::DictionarySP GetMetadata() override;
 
+  static void Initialize();
+
+  static void Terminate();
+
+  static llvm::StringRef GetPluginNameStatic() {
+    return "ScriptedProcessPythonInterface";
+  }
+
+  llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
+
 private:
   lldb::ScriptedThreadInterfaceSP CreateScriptedThreadInterface() override;
 };
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index d34fdf14122f2..a78c76b5f94ff 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -16,7 +16,7 @@
 
 #include "Interfaces/OperatingSystemPythonInterface/OperatingSystemPythonInterface.h"
 #include "Interfaces/ScriptedPlatformPythonInterface/ScriptedPlatformPythonInterface.h"
-#include "Interfaces/ScriptedProcessPythonInterface.h"
+#include "Interfaces/ScriptedProcessPythonInterface/ScriptedProcessPythonInterface.h"
 #include "Interfaces/ScriptedThreadPlanPythonInterface/ScriptedThreadPlanPythonInterface.h"
 #include "PythonDataObjects.h"
 #include "PythonReadline.h"

[DavidSpickett]

I tried making lldb-pthon.h the literal first include of the file but this doesn't work. When it gets to SWIGPythonBridge.h the redefinition happens again, no idea why, I'd think NO_PID_T would still be defined.

[DavidSpickett]

With this scripting template list shows:

(lldb) scripting template list
Available scripted extension templates:
  Name: OperatingSystemPythonInterface
  Language: Python
  Description: Mock thread state
  API Usages: None
  Command Interpreter Usages:
    settings set target.process.python-os-plugin-path <script-path>
    settings set process.experimental.os-plugin-reports-all-threads [0/1]

  Name: ScriptedPlatformPythonInterface
  Language: Python
  Description: Mock platform and interact with its processes.
  API Usages: None
  Command Interpreter Usages: None

  Name: ScriptedProcessPythonInterface
  Language: Python
  Description: Mock process state
  API Usages:
    SBAttachInfo.SetScriptedProcessClassName
    SBAttachInfo.SetScriptedProcessDictionary
    SBTarget.Attach
    SBLaunchInfo.SetScriptedProcessClassName
    SBLaunchInfo.SetScriptedProcessDictionary
    SBTarget.Launch
  Command Interpreter Usages:
    process attach -C <script-name> [-k key -v value ...]
    process launch -C <script-name> [-k key -v value ...]

  Name: ScriptedThreadPlanPythonInterface
  Language: Python
  Description: Alter thread stepping logic and stop reason
  API Usages: SBThread.StepUsingScriptedThreadPlan
  Command Interpreter Usages: thread step-scripted -C <script-name> [-k key -v value ...]

ScriptedProcessPythonInterface has been added.

It also passes check-lldb but that's expected as there's no new tests?

[medismailben]

@DavidSpickett This looks great to me! Thank you so much for taking the time to fix this!

Indeed for now, there is no test but I'll just add a shell test to make sure that the output matches my expectations.

Thanks!

[DavidSpickett]

I'm not going to merge this myself as I'm finishing for the week, but you can if you want to get that test added sooner.

[medismailben]

Sounds good, I'll merge it and add the test. If the bot fails for whatever reason, I'll revert it. Thanks!

[slydiman]

After this patch it is very hard to build lldb (debug version) on Windows.
We got the errors like the following

D:\llvm-project\lldb\source\Plugins\ScriptInterpreter\Python\Interfaces\ScriptedThreadPlanPythonInterface\ScriptedThreadPlanPythonInterface.cpp: 
fatal error C1041: cannot open program database 'D:\build-lldb\tools\lldb\source\Plugins\ScriptInterpreter\Python\Interfaces\ScriptedThreadPlanPythonInterface\CMakeFiles\lldbPluginScriptInterpreterPythonScriptedThreadPlanPythonInterface.dir\lldbPluginScriptInterpreterPythonScriptedThreadPlanPythonInterface.pdb'; 
if multiple CL.EXE write to the same .PDB file, please use /FS

Note MAX_PATH is 260 on Windows. But the length of the path to .pdb is 262.
It is necessary to use 7 chars or less folder in the root of a disk to build lldb (debug version) on Windows using Microsoft VC++.
You must reduce the length of the paths.

[DavidSpickett]

@medismailben how might we reduce the path length?

Maybe /interfaces/ content could be moved into the parent folder, or Python dropped from some of the cpp file names (as they're in a python subfolder anyway)? Headers are better left verbose so we can see what they are from the include site. Or Python -> Py, but that's a lot more file names to change.

[labath]

lldbPluginScriptInterpreterPythonScriptedThreadPlanPythonInterface quite a mouthful for just a single cpp file. Maybe all of the interfaces could be a part of a single library?

That would end up with something like:

D:\build-lldb\tools\lldb\source\Plugins\ScriptInterpreter\Python\Interfaces\CMakeFiles\lldbPluginScriptInterpreterPythonInterfaces.dir\lldbPluginScriptInterpreterPythonInterfaces.pdb

.. which is 80 chars shorter?

[medismailben]

lldbPluginScriptInterpreterPythonScriptedThreadPlanPythonInterface quite a mouthful for just a single cpp file. Maybe all of the interfaces could be a part of a single library?

That would end up with something like:

D:\build-lldb\tools\lldb\source\Plugins\ScriptInterpreter\Python\Interfaces\CMakeFiles\lldbPluginScriptInterpreterPythonInterfaces.dir\lldbPluginScriptInterpreterPythonInterfaces.pdb

.. which is 80 chars shorter?

They need to be in separate libraries because I use the PluginManager::Register method to register each plugin for them to be shown in the scripting extension list command. We could may be change the library name to be shorter but that would "break" the naming convention I guess.

[medismailben]

Originally, they were built in a single library and I added a cmake variable to make it work but @bulbazord & @JDevlieghere were not too happy with that approach. I think their argument was that if they're each different plugins, they need to be built into separate libraries and we should not make an exception at the cmake level. See #97273 (comment) for more context.

[labath]

Okay, that sort of makes sense. However, unless you actually want to be able to disable each of these plugins independently (which it sounds like you don't), then this is a very.. baroque way to achieve the desired effect (to have multiple (plugin) classes registered with the plugin manager).

All of that cmake plugin logic is just a very elaborate way to invoke SomeClass::Initialize(). Plugins are registered through PluginManager::RegisterPlugin. While the usual case is that the Initialize function performs a single RegisterPlugin call (and pretty much nothing else), nothing actually depends or enforces that, and we do have "plugins" where a single plugin library registers more than plugin class (e.g. lldbPluginPlatformMacOSX). I think it would be perfectly reasonable to register all of these classes from ScriptInterpreterPythonInterfaces::Initialize (and maybe even from ScriptInterpreterPython::Initialize).

[medismailben]

Okay, that sort of makes sense. However, unless you actually want to be able to disable each of these plugins independently (which it sounds like you don't), then this is a very.. baroque way to achieve the desired effect (to have multiple (plugin) classes registered with the plugin manager).

All of that cmake plugin logic is just a very elaborate way to invoke SomeClass::Initialize(). Plugins are registered through PluginManager::RegisterPlugin. While the usual case is that the Initialize function performs a single RegisterPlugin call (and pretty much nothing else), nothing actually depends or enforces that, and we do have "plugins" where a single plugin library registers more than plugin class (e.g. lldbPluginPlatformMacOSX). I think it would be perfectly reasonable to register all of these classes from ScriptInterpreterPythonInterfaces::Initialize (and maybe even from ScriptInterpreterPython::Initialize).

I was on vacation last week, I'll try to give a stab a this tomorrow, thanks for the suggestions :)

[medismailben]

@labath I've implemented your suggestion in #104896 and it seems to work fine on macOS.

@DavidSpickett @slydiman if you could try building this commit on windows an either run scripting extension list or run this test lldb/test/Shell/Commands/command-scripting-template-list.test to make sure it works fine, that'd be very helpful.