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

DavidSpickett · 2024-08-02T13:31:52Z

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.

…sue" 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 · 2024-08-02T13:32:34Z

@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 · 2024-08-02T13:33:05Z

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 · 2024-08-02T13:35:37Z

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 · 2024-08-02T15:44:26Z

@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 · 2024-08-02T15:46:38Z

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 · 2024-08-02T15:48:51Z

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

This patch adds a shell test to verify the output of the `scripting template list` command as discussed in llvm#101672. Signed-off-by: Med Ismail Bennani <[email protected]>

…sue (llvm#101672) 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.

slydiman · 2024-08-14T17:56:34Z

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 · 2024-08-15T10:26:49Z

@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 · 2024-08-15T12:36:43Z

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 · 2024-08-16T02:07:24Z

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 · 2024-08-16T04:03:52Z

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 · 2024-08-16T07:34:04Z

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 · 2024-08-19T06:12:09Z

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 :)

This patch tries to fix an issue with the windows debug builds where the PDB file for python scripted interfaces cannot be opened since its path length exceed the windows `MAX_PATH` limit: llvm#101672 (comment) This patch addresses the issue by building all the interfaces as a single library plugin that initiliazes each component as part of its `Initialize` method, instead of building each interface as its own library plugin. This keeps the build artifact path length smaller while respecting the naming convention and without making any exception in the build system. Fixes llvm#104895. Signed-off-by: Med Ismail Bennani <[email protected]>

medismailben · 2024-08-20T05:42:04Z

@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.

This patch tries to fix an issue with the windows debug builds where the PDB file for python scripted interfaces cannot be opened since its path length exceed the windows `MAX_PATH` limit: #101672 (comment) This patch addresses the issue by building all the interfaces as a single library plugin that initiliazes each component as part of its `Initialize` method, instead of building each interface as its own library plugin. This keeps the build artifact path length smaller while respecting the naming convention and without making any exception in the build system. Fixes #104895. Signed-off-by: Med Ismail Bennani <[email protected]>

This patch tries to fix an issue with the windows debug builds where the PDB file for python scripted interfaces cannot be opened since its path length exceed the windows `MAX_PATH` limit: llvm#101672 (comment) This patch addresses the issue by building all the interfaces as a single library plugin that initiliazes each component as part of its `Initialize` method, instead of building each interface as its own library plugin. This keeps the build artifact path length smaller while respecting the naming convention and without making any exception in the build system. Fixes llvm#104895. Signed-off-by: Med Ismail Bennani <[email protected]>

DavidSpickett requested a review from medismailben August 2, 2024 13:31

DavidSpickett requested a review from JDevlieghere as a code owner August 2, 2024 13:31

llvmbot added the lldb label Aug 2, 2024

DavidSpickett changed the title ~~Reland "[lldb] Reland 2402b3213c2f with /H to debug the windows bui…~~ Reland "[lldb] Reland 2402b3213c2f with /H to debug the windows build issue Aug 2, 2024

DavidSpickett mentioned this pull request Aug 2, 2024

[lldb/Commands] Add scripting template list command with auto discovery #97273

Merged

medismailben approved these changes Aug 2, 2024

View reviewed changes

medismailben merged commit 9d07f43 into llvm:main Aug 2, 2024
8 checks passed

medismailben mentioned this pull request Aug 2, 2024

[lldb/test] Add test for the scripting template list command #101726

Merged

DavidSpickett deleted the lldb-scripted branch August 15, 2024 08:18

This was referenced Aug 20, 2024

After this patch it is very hard to build lldb (debug version) on Windows. #104895

Closed

[lldb] Fix windows debug build after 9d07f43 #104896

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DavidSpickett commented Aug 2, 2024 •

edited

Loading

llvmbot commented Aug 2, 2024

DavidSpickett commented Aug 2, 2024

DavidSpickett commented Aug 2, 2024

medismailben commented Aug 2, 2024

DavidSpickett commented Aug 2, 2024

medismailben commented Aug 2, 2024

slydiman commented Aug 14, 2024 •

edited

Loading

DavidSpickett commented Aug 15, 2024

labath commented Aug 15, 2024

medismailben commented Aug 16, 2024

medismailben commented Aug 16, 2024 •

edited

Loading

labath commented Aug 16, 2024

medismailben commented Aug 19, 2024 •

edited

Loading

medismailben commented Aug 20, 2024

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

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

Conversation

DavidSpickett commented Aug 2, 2024 • edited Loading

llvmbot commented Aug 2, 2024

DavidSpickett commented Aug 2, 2024

DavidSpickett commented Aug 2, 2024

medismailben commented Aug 2, 2024

DavidSpickett commented Aug 2, 2024

medismailben commented Aug 2, 2024

slydiman commented Aug 14, 2024 • edited Loading

DavidSpickett commented Aug 15, 2024

labath commented Aug 15, 2024

medismailben commented Aug 16, 2024

medismailben commented Aug 16, 2024 • edited Loading

labath commented Aug 16, 2024

medismailben commented Aug 19, 2024 • edited Loading

medismailben commented Aug 20, 2024

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

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

DavidSpickett commented Aug 2, 2024 •

edited

Loading

slydiman commented Aug 14, 2024 •

edited

Loading

medismailben commented Aug 16, 2024 •

edited

Loading

medismailben commented Aug 19, 2024 •

edited

Loading