[Lldb-commits] [PATCH] D91810: [lldb] [Process/FreeBSD] Fix 'process connect' plugin choice

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D91810#2407483 , @mgorny wrote:

> In D91810#2407461 , @labath wrote:
>
>> I think that the real fix here would be to change the plugin selection logic 
>> (Target::CreateProcess) to convey the fact that we're looking for a 
>> connectable plugin. The method already contains a `FileSpec *core_file` 
>> argument, which I guess is needed to select the proper core file subclass, 
>> so adding a can_connect argument would not be unreasonable (though not 
>> exactly pretty either).
>
> Would maybe a static class variable ('property'?) be cleaner?

It might, though I am not sure how would you make that interact with the 
current selection logic. And I think you'll still need to pass a flag all the 
way to Process::FindPlugin, as that's the thing which iterates over registered 
classes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91810/new/

https://reviews.llvm.org/D91810

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D91810: [lldb] [Process/FreeBSD] Fix 'process connect' plugin choice

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D91810#2407461 , @labath wrote:

> I think that the real fix here would be to change the plugin selection logic 
> (Target::CreateProcess) to convey the fact that we're looking for a 
> connectable plugin. The method already contains a `FileSpec *core_file` 
> argument, which I guess is needed to select the proper core file subclass, so 
> adding a can_connect argument would not be unreasonable (though not exactly 
> pretty either).

Would maybe a static class variable ('property'?) be cleaner?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91810/new/

https://reviews.llvm.org/D91810

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D91810: [lldb] [Process/FreeBSD] Fix 'process connect' plugin choice

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This is all weird (not in a "your fault"  way). ProcessFreeBSD is decidedly a 
local-only plugin. It could never successfully complete the "process connect" 
command, regardless of whether we have some other remote plugin or not.

I think that the real fix here would be to change the plugin selection logic 
(Target::CreateProcess) to convey the fact that we're looking for a connectable 
plugin. The method already contains a `FileSpec *core_file` argument, which I 
guess is needed to select the proper core file subclass, so adding a 
can_connect argument would not be unreasonable (though not exactly pretty 
either).

Though, for the purposes of this test, just adding a "--plugin gdb-remote" 
argument to the "process connect" command should be sufficient as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91810/new/

https://reviews.llvm.org/D91810

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D91810: [lldb] [Process/FreeBSD] Fix 'process connect' plugin choice

[Ed Maste via Phabricator via lldb-commits]

emaste accepted this revision.
emaste added a comment.
This revision is now accepted and ready to land.

Looks ok to me


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91810/new/

https://reviews.llvm.org/D91810

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D91810: [lldb] [Process/FreeBSD] Fix 'process connect' plugin choice

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: labath, emaste, krytarowski.
Herald added a subscriber: arichardson.
mgorny requested review of this revision.

Fix ProcessFreeBSD::CanDebug() to indicate whether the legacy plugin
is actually being used.  This is necessary to prevent LLDB from wrongly
selecting the legacy plugin when searching for a process plugin for
'process connect' command.


https://reviews.llvm.org/D91810

Files:
  lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
  lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.h
  lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
  lldb/test/API/commands/process/attach-resume/TestAttachResume.py
  lldb/test/API/functionalities/deleted-executable/TestDeletedExecutable.py
  lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
  lldb/test/Shell/Commands/command-process-connect.test

Index: lldb/test/Shell/Commands/command-process-connect.test
===
--- lldb/test/Shell/Commands/command-process-connect.test
+++ lldb/test/Shell/Commands/command-process-connect.test
@@ -1,5 +1,4 @@
 # UNSUPPORTED: system-windows
-# XFAIL: system-freebsd
 
 # Synchronous
 # RUN: %lldb -o 'platform select remote-gdb-server' -o 'process connect connect://localhost:4321' 2>&1 | FileCheck %s
Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -36,7 +36,6 @@
 self.dbg.GetSelectedPlatform().DisconnectRemote()
 
 @skipIfWindows
-@expectedFailureAll(oslist=["freebsd"])
 def test_process_connect_sync(self):
 """Test the gdb-remote command in synchronous mode"""
 try:
@@ -48,7 +47,6 @@
 self.dbg.GetSelectedPlatform().DisconnectRemote()
 
 @skipIfWindows
-@expectedFailureAll(oslist=["freebsd"])
 @skipIfReproducer # Reproducer don't support async.
 def test_process_connect_async(self):
 """Test the gdb-remote command in asynchronous mode"""
Index: lldb/test/API/functionalities/deleted-executable/TestDeletedExecutable.py
===
--- lldb/test/API/functionalities/deleted-executable/TestDeletedExecutable.py
+++ lldb/test/API/functionalities/deleted-executable/TestDeletedExecutable.py
@@ -16,7 +16,7 @@
 NO_DEBUG_INFO_TESTCASE = True
 
 @skipIfWindows # cannot delete a running executable
-@expectedFailureAll(oslist=["freebsd", "linux"],
+@expectedFailureAll(oslist=["linux"],
 triple=no_match('aarch64-.*-android'))
 # determining the architecture of the process fails
 @skipIfReproducer # File synchronization is not supported during replay.
Index: lldb/test/API/commands/process/attach-resume/TestAttachResume.py
===
--- lldb/test/API/commands/process/attach-resume/TestAttachResume.py
+++ lldb/test/API/commands/process/attach-resume/TestAttachResume.py
@@ -18,7 +18,6 @@
 NO_DEBUG_INFO_TESTCASE = True
 
 @skipIfRemote
-@expectedFailureAll(oslist=['freebsd'], bugnumber='llvm.org/pr19310')
 @expectedFailureNetBSD
 @skipIfWindows # llvm.org/pr24778, llvm.org/pr21753
 @skipIfReproducer # FIXME: Unexpected packet during (active) replay
Index: lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
===
--- lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
+++ lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
@@ -28,6 +28,7 @@
 #include "lldb/Utility/State.h"
 
 #include "FreeBSDThread.h"
+#include "Plugins/Platform/FreeBSD/PlatformFreeBSD.h"
 #include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
 #include "Plugins/Process/Utility/FreeBSDSignals.h"
 #include "Plugins/Process/Utility/InferiorCallPOSIX.h"
@@ -273,6 +274,9 @@
 
 bool ProcessFreeBSD::CanDebug(lldb::TargetSP target_sp,
   bool plugin_specified_by_name) {
+  if (!platform_freebsd::PlatformFreeBSD::UseLegacyPlugin())
+return false;
+
   // For now we are just making sure the file exists for a given module
   ModuleSP exe_module_sp(target_sp->GetExecutableModule());
   if (exe_module_sp.get())
Index: lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.h
===
--- lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.h
+++ lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.h
@@ -42,6 +42,8 @@
 
   bool GetSupportedArchitectureAtIndex(uint32_t idx, ArchSpec ) override;
 
+  static bool UseLegacyPlugin();
+
   bool CanDebugProcess() override;
 
   size_t GetSoftwareBreakpointTrapOpcode(Target ,
Index: lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp