[lldb/Plugins] Fix ScriptedThread IndexID reporting

[mib]

Summary

When listing all the Scripted Threads of a ScriptedProcess, we can see that all
have the thread index set to 1. This is caused by the lldb_private::Thread
constructor, which sets the m_index_id member using the provided thread id tid.

Because the call to the super constructor is done before instantiating
the ScriptedThreadInterface, lldb can't fetch the thread id from the
script instance, so it uses LLDB_INVALID_THREAD_ID instead.

To mitigate this, this patch takes advantage of the ScriptedThread::Create
fallible constructor idiom to defer calling the ScriptedThread constructor
(and the Thread super constructor with it), until we can fetch a valid
thread id tid from the ScriptedThreadInterface.

rdar://87432065

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

[labath]

It seems to me like the ScriptedThread class would benefit from using the fallible constructor idiom. If you move all the potentially failing operations (initialization of the ScriptedThreadInterface instance?) into a (public) static Expected<ScriptedThreadInterfaceSP> Create(...) method, then the constructor will not need the funky by-ref error argument, and it will be able to access the thread id sufficiently early.

[mib]

Changed the implementation to defer constructing the ScriptedThread until we have a valid tid

[labath]

That is a step in the right direction, but ideally we shouldn't by introducing new functions with double return values (value in the "real" result + error through a by-ref argument, or vice versa). We have llvm::Expected for that, and (thread id issue aside) this is actually one of the biggest reasons for using the fallible constructor idiom (real constructors can't return Expecteds, but static factories can).

In that light, I am very saddened by the existence of the ErrorWithMessage function as it encourages one to use the incorrect/old/etc. paradigm. Like, I get that it makes it handy to work with the functions which still use the old interface, but if you really want to have it, then I'd suggest to also create one which interoperates nicely with Expected-returning functions, so that you're not tempted to create new functions with by-ref error results.

Personally, I'd say this wrapper is unnecessary for the new functions, as one of the nice (opinions can vary) things about the llvm error types is that they force you to do something with those errors. Usually, when we don't have anything better to do with these errors we at least log them. So, I'd probably skip logging the errors upon creating, and log them in the caller (as it will have to handle them somehow anyway).

lldb/source/Plugins/Process/scripted/ScriptedThread.cpp

73

this is not needed now

[mib]

Rebase and address @labath comments.

[labath]

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

340

Although there's no way to exclude this via the type system (well... we could have a nonnull_shared_ptr<T>...), this really shouldn't ever happen. All of our Expected<unique/shared_ptr>-returning APIs assume that in the non-error case, the pointer object will be valid. At best, this deserves an assertion.

lldb/source/Plugins/Process/scripted/ScriptedThread.cpp

59

btw, there's a llvm::createStringError, which is (slightly) shorter.

lldb/include/lldb/Target/Thread.h

protected:
   uint32_t m_stop_info_stop_id; // This is the stop id for which the StopInfo is
                                 // valid.  Can use this so you know that
   // the thread's m_stop_info_sp is current and you don't have to fetch it
   // again
   uint32_t m_stop_info_override_stop_id; // The stop ID containing the last time
                                          // the stop info was checked against
                                          // the stop info override
   const uint32_t m_index_id; ///< A unique 1 based index assigned to each thread
-                             ///for easy UI/command line access.
+                             /// for easy UI/command line access.
   lldb::RegisterContextSP m_reg_context_sp; ///< The register context for this
                                             ///thread's current register state.
   lldb::StateType m_state;                  ///< The state of our process.
   mutable std::recursive_mutex
       m_state_mutex;       ///< Multithreaded protection for m_state.
   mutable std::recursive_mutex
       m_frame_mutex; ///< Multithreaded protection for m_state.
   lldb::StackFrameListSP m_curr_frames_sp; ///< The stack frames that get lazily

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

auto create_scripted_thread =
     if (ThreadSP thread_sp =
             old_thread_list.FindThreadByID(tid, false /*=can_update*/)) {
       // If the thread was already in the old_thread_list,
       // just add it back to the new_thread_list.
       new_thread_list.AddThread(thread_sp);
       return true;
     }
 
-    lldb::ThreadSP thread_sp =
-        std::make_shared<ScriptedThread>(*this, error, val->GetAsGeneric());
+    auto thread_or_error = ScriptedThread::Create(*this, val->GetAsGeneric());
 
-    if (!thread_sp || error.Fail())
-      return GetInterface().ErrorWithMessage<bool>(LLVM_PRETTY_FUNCTION,
-                                                   error.AsCString(), error);
+    if (!thread_or_error)
+      return GetInterface().ErrorWithMessage<bool>(
+          LLVM_PRETTY_FUNCTION, toString(thread_or_error.takeError()), error);
+
+    ThreadSP thread_sp = thread_or_error.get();
+    lldbassert(thread_sp && "Couldn't initialize thread.");
 
     RegisterContextSP reg_ctx_sp = thread_sp->GetRegisterContext();
     if (!reg_ctx_sp)
       return GetInterface().ErrorWithMessage<bool>(
           LLVM_PRETTY_FUNCTION,
           llvm::Twine("Invalid Register Context for thread " +
                       llvm::Twine(key.AsCString()))
               .str(),
           error);
 

lldb/source/Plugins/Process/scripted/ScriptedThread.h

 
 namespace lldb_private {
 class ScriptedProcess;
 }
 
 namespace lldb_private {
 
 class ScriptedThread : public lldb_private::Thread {
+
 public:
-  ScriptedThread(ScriptedProcess &process, Status &error,
-                 StructuredData::Generic *script_object = nullptr);
+  ScriptedThread(ScriptedProcess &process,
+                 lldb::ScriptedThreadInterfaceSP interface_sp, lldb::tid_t tid,
+                 StructuredData::GenericSP script_object_sp = nullptr);
 
   ~ScriptedThread() override;
 
+  static llvm::Expected<std::shared_ptr<ScriptedThread>>
+  Create(ScriptedProcess &process,
+         StructuredData::Generic *script_object = nullptr);
+
   lldb::RegisterContextSP GetRegisterContext() override;
 
   lldb::RegisterContextSP
   CreateRegisterContextForFrame(lldb_private::StackFrame *frame) override;
 
   bool CalculateStopInfo() override;
 
   const char *GetInfo() override { return nullptr; }
private:
 
   ScriptedThread(const ScriptedThread &) = delete;
   const ScriptedThread &operator=(const ScriptedThread &) = delete;
 
   std::shared_ptr<DynamicRegisterInfo> GetDynamicRegisterInfo();
 
   const ScriptedProcess &m_scripted_process;
   lldb::ScriptedThreadInterfaceSP m_scripted_thread_interface_sp = nullptr;
+  lldb_private::StructuredData::GenericSP m_script_object_sp = nullptr;
   std::shared_ptr<DynamicRegisterInfo> m_register_info_sp = nullptr;
-  lldb_private::StructuredData::ObjectSP m_script_object_sp = nullptr;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_SCRIPTED_THREAD_H

lldb/source/Plugins/Process/scripted/ScriptedThread.cpp

 using namespace lldb;
 using namespace lldb_private;
 
 void ScriptedThread::CheckInterpreterAndScriptObject() const {
   lldbassert(m_script_object_sp && "Invalid Script Object.");
   lldbassert(GetInterface() && "Invalid Scripted Thread Interface.");
 }
 
-ScriptedThread::ScriptedThread(ScriptedProcess &process, Status &error,
-                               StructuredData::Generic *script_object)
-    : Thread(process, LLDB_INVALID_THREAD_ID), m_scripted_process(process),
-      m_scripted_thread_interface_sp(
-          m_scripted_process.GetInterface().CreateScriptedThreadInterface()) {
-  if (!process.IsValid()) {
-    error.SetErrorString("Invalid scripted process");
-    return;
-  }
+llvm::Expected<std::shared_ptr<ScriptedThread>>
+ScriptedThread::Create(ScriptedProcess &process,
+                       StructuredData::Generic *script_object) {
+  if (!process.IsValid())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Invalid scripted process.");
   
   process.CheckInterpreterAndScriptObject();
 
-  auto scripted_thread_interface = GetInterface();
-  if (!scripted_thread_interface) {
-    error.SetErrorString("Failed to get scripted thread interface.");
-    return;
-  }
+  auto scripted_thread_interface =
+      process.GetInterface().CreateScriptedThreadInterface();
+  if (!scripted_thread_interface)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Failed to create scripted thread interface.");
 
   llvm::Optional<std::string> class_name =
       process.GetInterface().GetScriptedThreadPluginName();
-  if (!class_name || class_name->empty()) {
-    error.SetErrorString("Failed to get scripted thread class name.");
-    return;
-  }
+  if (!class_name || class_name->empty())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Failed to get scripted thread class name.");
 
   ExecutionContext exe_ctx(process);
+  StructuredData::GenericSP owned_script_object_sp =
+      scripted_thread_interface->CreatePluginObject(
+          class_name->c_str(), exe_ctx,
+          process.m_scripted_process_info.GetArgsSP(), script_object);
+
+  if (!owned_script_object_sp)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Failed to create script object.");
+  if (!owned_script_object_sp->IsValid())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Created script object is invalid.");
 
-  m_script_object_sp = scripted_thread_interface->CreatePluginObject(
-      class_name->c_str(), exe_ctx, process.m_scripted_process_info.GetArgsSP(),
-      script_object);
-
-  if (!m_script_object_sp) {
-    error.SetErrorString("Failed to create script object");
-    return;
-  }
+  lldb::tid_t tid = scripted_thread_interface->GetThreadID();
 
-  if (!m_script_object_sp->IsValid()) {
-    m_script_object_sp = nullptr;
-    error.SetErrorString("Created script object is invalid");
-    return;
-  }
+  return std::make_shared<ScriptedThread>(process, scripted_thread_interface,
+                                          tid, owned_script_object_sp);
 
-  lldb::tid_t tid = scripted_thread_interface->GetThreadID();
-  SetID(tid);
-}
+ScriptedThread::ScriptedThread(ScriptedProcess &process,
+                               ScriptedThreadInterfaceSP interface_sp,
+                               lldb::tid_t tid,
+                               StructuredData::GenericSP script_object_sp)
+    : Thread(process, tid), m_scripted_process(process),
+      m_scripted_thread_interface_sp(interface_sp),
+      m_script_object_sp(script_object_sp) {}
 
 ScriptedThread::~ScriptedThread() { DestroyThread(); }
 
 const char *ScriptedThread::GetName() {
   CheckInterpreterAndScriptObject();
   llvm::Optional<std::string> thread_name = GetInterface()->GetName();
   if (!thread_name)
     return nullptr;