[lldb] Fix bug to update process public run lock with process state

[mib]

Summary

This patch should address an issue that caused the process public run
lock to not be updated during a process launch/attach when the process
stops.

That caused the public run lock to report its state as running while the
process state is stopped. This prevents the users to interact with the
process (through the command line or via the SBAPI) since it's
considered still running.

To address that, this patch refactors the name of the internal hijack
listeners to a specific pattern lldb.internal.<action>.hijack that
are used to ensure that we've attached to or launched a process successfully.

Then, when updating the process public state, after updating the state
value, if the process is not hijacked externally, meaning if the process
doens't have a hijack listener that matches the internal hijack
listeners pattern, we can update the public run lock accordingly.

rdar://108283017

Signed-off-by: Med Ismail Bennani <[e-mail]>

[bulbazord]

Idea is good, few concerns about the implementation.

lldb/source/Target/Process.cpp

404–419

Do these need to be ConstStrings? They will live in the string pool forever, and it looks like in the code below you're just manipulating const char * anyway.

Could be something like:

llvm::StringRef Process::GetAttachSynchronousHijackListenerName() {
  return "lldb.internal.Process.AttachSynchronous.hijack";
}

1394–1395

Instead of using the strstr function directly, I would at least do strnstr to ensure that we're not touching memory we don't have. Especially if we could get hijacking_name to be shorter than "lldb.internal" somehow...

We could also change this to use StringRefs and use the starts_with/startswith methods instead.

1403–1405

First, I would probably avoid strcmp directly and use strncmp instead. We know the length of the HijackListener name, that would be a good choice for n here.

But you could probably simplify this even further. You can compare GetResumeSynchronousHijackListenerName to hijacking_name with StringRef's operator==.

[mib]

Address @bulbazord comments

[bulbazord]

LGTM

[JDevlieghere]

lldb/source/Target/Process.cpp

404–414

Do these actually need to be methods? Can't these be public static StringRefs instead?

[mib]

Turning hijack listener name getters into public static constexpr llvmStringRef as @JDevlieghere suggested.

[JDevlieghere]

Ship it

[mib]

Reverting this because it introduced some failures on the linux bot: https://lab.llvm.org/buildbot/#/builders/68/builds/51397/steps/6/logs/stdio

[mib]

lldb/source/Target/Process.cpp

1395

I think I forgot to negate this: if hijacking_name DOES NOT start with "lldb.internal", then it mean that the process state change is externally hijacked.

[mib]

Fix condition typo

lldb/include/lldb/Target/Process.h

public:
   // or just try to lock it to see if they can immediately access the stopped
   // process. If the try read lock fails, then the process is running.
   typedef ProcessRunLock::ProcessRunLocker StopLocker;
 
   // These two functions fill out the Broadcaster interface:
 
   static ConstString &GetStaticBroadcasterClass();
 
+  static constexpr llvm::StringRef AttachSynchronousHijackListenerName =
+      "lldb.internal.Process.AttachSynchronous.hijack";
+  static constexpr llvm::StringRef LaunchSynchronousHijackListenerName =
+      "lldb.internal.Process.LaunchSynchronous.hijack";
+  static constexpr llvm::StringRef ResumeSynchronousHijackListenerName =
+      "lldb.internal.Process.ResumeSynchronous.hijack";
+
   ConstString &GetBroadcasterClass() const override {
     return GetStaticBroadcasterClass();
   }
 
 /// A notification structure that can be used by clients to listen
 /// for changes in a process's lifetime.
 ///
 /// \see RegisterNotificationCallbacks (const Notifications&) @see

lldb/source/Target/Process.cpp

ProcessSP Process::FindPlugin(lldb::TargetSP target_sp,
   return process_sp;
 }
 
 ConstString &Process::GetStaticBroadcasterClass() {
   static ConstString class_name("lldb.process");
   return class_name;
 }
 
 Process::Process(lldb::TargetSP target_sp, ListenerSP listener_sp)
     : Process(target_sp, listener_sp, UnixSignals::CreateForHost()) {
   // This constructor just delegates to the full Process constructor,
   // defaulting to using the Host's UnixSignals.
 }
 
 Process::Process(lldb::TargetSP target_sp, ListenerSP listener_sp,
                  const UnixSignalsSP &unix_signals_sp)
     : ProcessProperties(this),
       Broadcaster((target_sp->GetDebugger().GetBroadcasterManager()),
                   Process::GetStaticBroadcasterClass().AsCString()),
       m_target_wp(target_sp), m_public_state(eStateUnloaded),
       m_private_state(eStateUnloaded),
       m_private_state_broadcaster(nullptr,
                                   "lldb.process.internal_state_broadcaster"),
       m_private_state_control_broadcaster(
           nullptr, "lldb.process.internal_state_control_broadcaster"),
       m_private_state_listener_sp(
           Listener::MakeListener("lldb.process.internal_state_listener")),
       m_mod_id(), m_process_unique_id(0), m_thread_index_id(0),
       m_thread_id_to_index_id_map(), m_exit_status(-1), m_exit_string(),
       m_exit_status_mutex(), m_thread_mutex(), m_thread_list_real(this),
       m_thread_list(this), m_thread_plans(*this), m_extended_thread_list(this),
       m_extended_thread_stop_id(0), m_queue_list(this), m_queue_list_stop_id(0),
Status Process::Resume() {
   Status error = PrivateResume();
   if (!error.Success()) {
     // Undo running state change
     m_public_run_lock.SetStopped();
   }
   return error;
 }
 
-static const char *g_resume_sync_name = "lldb.Process.ResumeSynchronous.hijack";
-
 Status Process::ResumeSynchronous(Stream *stream) {
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
   LLDB_LOGF(log, "Process::ResumeSynchronous -- locking run lock");
   if (!m_public_run_lock.TrySetRunning()) {
     Status error("Resume request failed - process still running.");
     LLDB_LOGF(log, "Process::Resume: -- TrySetRunning failed, not resuming.");
     return error;
   }
 
   ListenerSP listener_sp(
-      Listener::MakeListener(g_resume_sync_name));
+      Listener::MakeListener(ResumeSynchronousHijackListenerName.data()));
   HijackProcessEvents(listener_sp);
 
   Status error = PrivateResume();
   if (error.Success()) {
     StateType state =
         WaitForProcessToStop(std::nullopt, nullptr, true, listener_sp, stream);
     const bool must_be_alive =
         false; // eStateExited is ok, so this must be false
Status Process::ResumeSynchronous(Stream *stream) {
   // Undo the hijacking of process events...
   RestoreProcessEvents();
 
   return error;
 }
 
 bool Process::StateChangedIsExternallyHijacked() {
   if (IsHijackedForEvent(eBroadcastBitStateChanged)) {
-    const char *hijacking_name = GetHijackingListenerName();
-    if (hijacking_name &&
-        strcmp(hijacking_name, g_resume_sync_name))
+    llvm::StringRef hijacking_name = GetHijackingListenerName();
+    if (!hijacking_name.starts_with("lldb.internal"))
       return true;
   }
   return false;
 }
 
 bool Process::StateChangedIsHijackedForSynchronousResume() {
   if (IsHijackedForEvent(eBroadcastBitStateChanged)) {
-    const char *hijacking_name = GetHijackingListenerName();
-    if (hijacking_name &&
-        strcmp(hijacking_name, g_resume_sync_name) == 0)
+    llvm::StringRef hijacking_name = GetHijackingListenerName();
+    if (hijacking_name == ResumeSynchronousHijackListenerName)
       return true;
   }
   return false;
 }
 
 StateType Process::GetPrivateState() { return m_private_state.GetValue(); }
 
 void Process::SetPrivateState(StateType new_state) {
   if (m_finalizing)

lldb/source/Target/Target.cpp

Status Target::Launch(ProcessLaunchInfo &launch_info, Stream *stream) {
   if (!launch_info.GetArchitecture().IsValid())
     launch_info.GetArchitecture() = GetArchitecture();
 
   // Hijacking events of the process to be created to be sure that all events
   // until the first stop are intercepted (in case if platform doesn't define
   // its own hijacking listener or if the process is created by the target
   // manually, without the platform).
   if (!launch_info.GetHijackListener())
-    launch_info.SetHijackListener(
-        Listener::MakeListener("lldb.Target.Launch.hijack"));
+    launch_info.SetHijackListener(Listener::MakeListener(
+        Process::LaunchSynchronousHijackListenerName.data()));
 
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
       platform_sp->CanDebugProcess() && !launch_info.IsScriptedProcess()) {
     LLDB_LOGF(log, "Target::%s asking the platform to debug the process",
               __FUNCTION__);
 
if (!attach_info.ProcessInfoSpecified()) {
     }
   }
 
   const auto platform_sp =
       GetDebugger().GetPlatformList().GetSelectedPlatform();
   ListenerSP hijack_listener_sp;
   const bool async = attach_info.GetAsync();
   if (!async) {
-    hijack_listener_sp =
-        Listener::MakeListener("lldb.Target.Attach.attach.hijack");
+    hijack_listener_sp = Listener::MakeListener(
+        Process::AttachSynchronousHijackListenerName.data());
     attach_info.SetHijackListener(hijack_listener_sp);
   }
 
   Status error;
   if (state != eStateConnected && platform_sp != nullptr &&
       platform_sp->CanDebugProcess() && !attach_info.IsScriptedProcess()) {
     SetPlatform(platform_sp);
     process_sp = platform_sp->Attach(attach_info, GetDebugger(), this, error);