[lldb] Prevent 'process connect' from using local-only plugins

[mgorny]

Summary

Add a 'can_connect' parameter to Process plugin initialization, and use
it to filter plugins to these capable of remote connections. This is
used to prevent 'process connect' from picking up a plugin that can only
be used locally, e.g. the legacy FreeBSD plugin.

[emaste]

Looks ok to me

[labath]

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.

[mgorny]

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?

[labath]

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.

[mgorny]

Updated to pass can_debug parameter, as suggested by @labath.

[labath]

Seems ok. The code could really use a refactor though...

lldb/test/Shell/Commands/command-process-connect.test

1–2

I'm pretty sure windows suffered from the same problem.

[mgorny]

lldb/test/Shell/Commands/command-process-connect.test

1–2

Ok, I will remove this and see what buildbots say.

lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.h

public:
   const char *GetDescription() override {
     return GetPluginDescriptionStatic(IsHost());
   }
 
   void GetStatus(Stream &strm) override;
 
   bool GetSupportedArchitectureAtIndex(uint32_t idx, ArchSpec &arch) override;
 
+  static bool UseLegacyPlugin();
+
   bool CanDebugProcess() override;
 
   size_t GetSoftwareBreakpointTrapOpcode(Target &target,
                                          BreakpointSite *bp_site) override;
 
   void CalculateTrapHandlerSymbolNames() override;
 
   MmapArgList GetMmapArgumentList(const ArchSpec &arch, lldb::addr_t addr,

lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp

if (bp_site->SetTrapOpcode(g_arm_breakpoint_opcode, trap_opcode_size))
       return trap_opcode_size;
   }
     LLVM_FALLTHROUGH;
   default:
     return Platform::GetSoftwareBreakpointTrapOpcode(target, bp_site);
   }
 }
 
-bool PlatformFreeBSD::CanDebugProcess() {
-  if (IsHost()) {
-    llvm::Triple host_triple{llvm::sys::getProcessTriple()};
-    bool use_legacy_plugin;
+bool PlatformFreeBSD::UseLegacyPlugin() {
 
-    switch (host_triple.getArch()) {
-      case llvm::Triple::x86:
-      case llvm::Triple::x86_64:
-        // FreeBSDRemote plugin supports x86 only at the moment
-        use_legacy_plugin = !!getenv("FREEBSD_LEGACY_PLUGIN");
-        break;
-      default:
-        use_legacy_plugin = true;
-    }
+      return !!getenv("FREEBSD_LEGACY_PLUGIN");
+      return true;
+  }
 
-    return !use_legacy_plugin;
-  } else {
-    // If we're connected, we can debug.
+bool PlatformFreeBSD::CanDebugProcess() {
+  if (IsHost()) // Use DebugProcess() method when using the new plugin
+    return !UseLegacyPlugin();
+  else // If we're connected, we can debug.
     return IsConnected();
-  }
-}
 
 void PlatformFreeBSD::CalculateTrapHandlerSymbolNames() {
   m_trap_handlers.push_back(ConstString("_sigtramp"));
 }
 
 MmapArgList PlatformFreeBSD::GetMmapArgumentList(const ArchSpec &arch,
                                                  addr_t addr, addr_t length,
                                                  unsigned prot, unsigned flags,

lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp

 #include "lldb/Host/Host.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/RegisterValue.h"
 #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"
 #include "ProcessFreeBSD.h"
 #include "ProcessMonitor.h"
 
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/Watchpoint.h"
void ProcessFreeBSD::Finalize() {
   Process::Finalize();
 
   if (m_monitor)
     m_monitor->StopMonitor();
 }
 
 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())
     return FileSystem::Instance().Exists(exe_module_sp->GetFileSpec());
   // If there is no executable module, we return true since we might be
   // preparing to attach.
   return true;
 }

lldb/test/API/commands/process/attach-resume/TestAttachResume.py

 
 
 class AttachResumeTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
     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
     def test_attach_continue_interrupt_detach(self):
         """Test attach/continue/interrupt/detach"""
         self.build()
         self.process_attach_continue_interrupt_detach()
 

lldb/test/API/functionalities/deleted-executable/TestDeletedExecutable.py

 from lldbsuite.test import lldbutil
 
 class TestDeletedExecutable(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
     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.
     def test(self):
         self.build()
         exe = self.getBuildArtifact("a.out")
 
         # Use a file as a synchronization point between test and inferior.

lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py

def test_gdb_remote_async(self):
                         matching=False,
                         substrs=['Process', 'stopped'])
             lldbutil.expect_state_changes(self, self.dbg.GetListener(),
                                           self.process(), [lldb.eStateStopped])
         finally:
             self.dbg.GetSelectedPlatform().DisconnectRemote()
 
     @skipIfWindows
-    @expectedFailureAll(oslist=["freebsd"])
     def test_process_connect_sync(self):
         """Test the gdb-remote command in synchronous mode"""
         try:
             self.dbg.SetAsync(False)
             self.expect("process connect connect://" +
                         self.server.get_connect_address(),
                         substrs=['Process', 'stopped'])
         finally:
             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"""
         try:
             self.dbg.SetAsync(True)
             self.expect("process connect connect://" +
                         self.server.get_connect_address(),
                         matching=False,
                         substrs=['Process', 'stopped'])
             lldbutil.expect_state_changes(self, self.dbg.GetListener(),
                                           self.process(), [lldb.eStateStopped])
         finally:
             self.dbg.GetSelectedPlatform().DisconnectRemote()

lldb/test/Shell/Commands/command-process-connect.test

 # 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
 
 # Asynchronous
 # RUN: echo -e 'platform select remote-gdb-server\nprocess connect connect://localhost:4321' | %lldb 2>&1 | FileCheck %s
 
 # CHECK: Platform: remote-gdb-server
 # CHECK: Connected: no
 # CHECK: error: Failed to connect port