Don't cancel the current IOHandler when we push a handler for an utility function run.

[teemperor]

Summary

D48465 is currently blocked by the fact that tab-completing the first expression is deadlocking LLDB.

The reason for this deadlock is that when we push the ProcessIO handler for reading the Objective-C runtime
information from the executable (which is triggered when we parse the an expression for the first time),
the IOHandler can't be pushed as the Editline::Cancel method is deadlocking.

The deadlock in Editline is coming from the m_output_mutex, which is locked before we go into tab completion.
Even without this lock, calling Cancel on Editline will mean that Editline cleans up behind itself and deletes the
current user-input, which is screws up the console when we are tab-completing at the same time.

I think for now the most reasonable way of fixing this is to just not call Cancel on the current IOHandler when we push
the IOHandler for running an internal utility function.

As we can't really write unit tests for IOHandler itself (due to the hard dependency on an initialized Debugger including
all its global state) and Editline completion is currently also not really testable in an automatic fashion, the test for this has
to be that the expression command completion in D48465 doesn't fail when requesting completion the first time.

A more precise test plan for this is:

1. Apply D48465.
2. Start lldb and break in some function.
3. Type expr foo and press tab to request completion.
4. Without this patch, we deadlock and LLDB stops responding.

I'll provide an actual unit test for this once I got around and made the IOHandler code testable,
but for now unblocking D48465 is more critical.

Thanks to Jim for helping me debugging this.

[jingham]

It isn't true that the ProcessIO handler will have a short duration, in general. It will live on the stack as long as process is running, which is of arbitrary duration. I actually don't think that much matters to this discussion, however.

The most common IOHandler that's going to be on the stack when the Process IO handler is called is the EditLine handler. That handler's Cancel does two things, one is to lock the output mutex - which is what you are trying to avoid, and one is to fix up the cursor location so that process IO won't mix with command IO, and tell the IO handler it has been interrupted. When you are interrupting the IOHandler for a hand-called function, especially in the middle of a command, which is what will happen when you are in the middle of completion you actually don't want the cursor to get reset either. But for a regular continue you probably do want to do all this work. lldb knows the difference between a "real" continue - when the user expects to give control over to the ProcessIO handler, and a hand-called function (m_running_user_expression gets set in the ProcessModID). I wonder if there's some way to use that fact to discriminate between the times it's good to avoid cancelling the underlying IO handler and when its not.

[teemperor]

include/lldb/Target/Process.h

436

@jingham That might be wrong, but I'm not sure what exactly each member variable is tracking.

For reference, those are the values in the two different situations:
When running from the utility function:

(lldb_private::ProcessModID) $0 = {
m_stop_id = 15
m_last_natural_stop_id = 15
m_resume_id = 15
m_memory_id = 12
m_last_user_expression_resume = 15
m_running_user_expression = 1
m_last_natural_stop_event = std::__1::shared_ptr<lldb_private::Event>::element_type @ 0x00007fc9afd5c790 strong=1 weak=1 {                                                                              
  __ptr_ = 0x00007fc9afd5c790
}

When running an user expression:

(lldb_private::ProcessModID) $0 = {
m_stop_id = 16
m_last_natural_stop_id = 15
m_resume_id = 16
m_memory_id = 31
m_last_user_expression_resume = 16
m_running_user_expression = 1
m_last_natural_stop_event = std::__1::shared_ptr<lldb_private::Event>::element_type @ 0x00007ff93576a850 strong=1 weak=1 {                                                                              
  __ptr_ = 0x00007ff93576a850
}
}

[jingham]

m_last_natural_stop_id is the stop ID for the last time we stopped when the user was just running the process (run, continue, next, etc...)

m_last_user_expression_resume is the resume ID for the last user expression resume.

The stops and resumes always occur in pairs in lldb at present. That won't be true when we get to no-stop debugging, but we'll have to adjust a bunch of things when we get to handling that...

Anyway, so if m_last_user_expression_resume >= m_last_natural_stop_id you are running some kind of expression.

[clayborg]

source/Target/Process.cpp

4696–4697

Another possible fix is to avoid pushing the IOHandler all together? Something like:

if (!m_mod_id.IsRunningUtilityFunction())
  GetTarget().GetDebugger().PushIOHandler(io_handler_sp);

This would avoid an needed changes to the IOHandler stuff?

[jingham]

What would happen to any output that the process produced while running the utility function if we did this.

[clayborg]

In D50912#1209994, @jingham wrote:

What would happen to any output that the process produced while running the utility function if we did this.

I believe it would still be fetched on next stop. Just not real time. Do you think anyone running a utility function would need to ever fetch data real time? "utility" in my mind means not started by a interactive user. Am I wrong?

[jingham]

It's not a huge deal, but it would be disconcerting to have a bunch of text dumped to your console the next time you continue. I think the ideal solution would be for utility functions to push a "capture & report" IO handler, so the Utility function could retrieve what went to stdout when it was running, and choose whether to display it or not (for instance, display it only if the function errored out...)

[teemperor]

I added an flag for this to the evaluation options. I also set this flag for all utility calls I found.

[clayborg]

include/lldb/Target/Process.h

479

Might want this to be:

else if (m_running_utility_function > 0)
  --m_running_utility_function;

Just to make sure we don't ever underflow

source/Target/Process.cpp

4696–4697

Do we still need this extra bool to PushIOHandler? Can't we do the code snippet I suggested above?

[teemperor]

• Add an assert against underflow checking.
• Fixed a (serious) typo that broke the test.

source/Target/Process.cpp

4696–4697

Do we know for sure that omitting this IOHandler push doesn't break something? From the comments above it seems that this is actually changing LLDB's behavior. And we can't easily revert this commit once I stack the expression completion + follow-ups on top of it.

I'm fine with doing this change as a direct follow-up commit that won't have any dependencies attached.

[clayborg]

Sounds good. I am ok with this as long as Jim Ingham is.

[jingham]

This is fine by me.

include/lldb/Core/Debugger.h

public:
   //------------------------------------------------------------------
   // If any of the streams are not set, set them to the in/out/err stream of
   // the top most input reader to ensure they at least have something
   //------------------------------------------------------------------
   void AdoptTopIOHandlerFilesIfInvalid(lldb::StreamFileSP &in,
                                        lldb::StreamFileSP &out,
                                        lldb::StreamFileSP &err);
 
-  void PushIOHandler(const lldb::IOHandlerSP &reader_sp);
+  void PushIOHandler(const lldb::IOHandlerSP &reader_sp,
+                     bool cancel_top_handler = true);
 
   bool PopIOHandler(const lldb::IOHandlerSP &reader_sp);
 
   // Synchronously run an input reader until it is done
   void RunIOHandler(const lldb::IOHandlerSP &reader_sp);
 
   bool IsTopIOHandler(const lldb::IOHandlerSP &reader_sp);
 

include/lldb/Target/Process.h

 // FIXME: Should we also include modification of register states?
 
 class ProcessModID {
   friend bool operator==(const ProcessModID &lhs, const ProcessModID &rhs);
 
 public:
   ProcessModID()
       : m_stop_id(0), m_last_natural_stop_id(0), m_resume_id(0), m_memory_id(0),
-        m_last_user_expression_resume(0), m_running_user_expression(false) {}
+        m_last_user_expression_resume(0), m_running_user_expression(false),
+        m_running_utility_function(0) {}
 
   ProcessModID(const ProcessModID &rhs)
       : m_stop_id(rhs.m_stop_id), m_memory_id(rhs.m_memory_id) {}
 
   const ProcessModID &operator=(const ProcessModID &rhs) {
     if (this != &rhs) {
       m_stop_id = rhs.m_stop_id;
       m_memory_id = rhs.m_memory_id;
public:
   void BumpMemoryID() { m_memory_id++; }
 
   void BumpResumeID() {
     m_resume_id++;
     if (m_running_user_expression > 0)
       m_last_user_expression_resume = m_resume_id;
   }
 
+  bool IsRunningUtilityFunction() const {
+    return m_running_utility_function > 0;
+  }
+
   uint32_t GetStopID() const { return m_stop_id; }
   uint32_t GetLastNaturalStopID() const { return m_last_natural_stop_id; }
   uint32_t GetMemoryID() const { return m_memory_id; }
   uint32_t GetResumeID() const { return m_resume_id; }
   uint32_t GetLastUserExpressionResumeID() const {
     return m_last_user_expression_resume;
   }
 
public:
 
   void SetRunningUserExpression(bool on) {
     if (on)
       m_running_user_expression++;
     else
       m_running_user_expression--;
   }
 
+  void SetRunningUtilityFunction(bool on) {
+    if (on)
+      m_running_utility_function++;
+    else {
+      assert(m_running_utility_function > 0 &&
+             "Called SetRunningUtilityFunction(false) without calling "
+             "SetRunningUtilityFunction(true) before?");
+      m_running_utility_function--;
+    }
+  }
+
   void SetStopEventForLastNaturalStopID(lldb::EventSP event_sp) {
     m_last_natural_stop_event = event_sp;
   }
 
   lldb::EventSP GetStopEventForStopID(uint32_t stop_id) const {
     if (stop_id == m_last_natural_stop_id)
       return m_last_natural_stop_event;
     return lldb::EventSP();
   }
 
 private:
   uint32_t m_stop_id;
   uint32_t m_last_natural_stop_id;
   uint32_t m_resume_id;
   uint32_t m_memory_id;
   uint32_t m_last_user_expression_resume;
   uint32_t m_running_user_expression;
+  uint32_t m_running_utility_function;
   lldb::EventSP m_last_natural_stop_event;
 };
 
 inline bool operator==(const ProcessModID &lhs, const ProcessModID &rhs) {
   if (lhs.StopIDEqual(rhs) && lhs.MemoryIDEqual(rhs))
     return true;
   else
     return false;
public:
   ///
   /// @return
   ///     Returns \b true if we were able to start up the notification
   ///     \b false otherwise.
   //------------------------------------------------------------------
   virtual bool StopNoticingNewThreads() { return true; }
 
   void SetRunningUserExpression(bool on);
+  void SetRunningUtilityFunction(bool on);
 
   //------------------------------------------------------------------
   // lldb::ExecutionContextScope pure virtual functions
   //------------------------------------------------------------------
   lldb::TargetSP CalculateTarget() override;
 
   lldb::ProcessSP CalculateProcess() override { return shared_from_this(); }
 
private:
   //------------------------------------------------------------------
   bool ShouldBroadcastEvent(Event *event_ptr);
 
   void ControlPrivateStateThread(uint32_t signal);
 
   DISALLOW_COPY_AND_ASSIGN(Process);
 };
 
+//------------------------------------------------------------------
+/// RAII guard that should be aquired when an utility function is called within
+/// a given process.
+//------------------------------------------------------------------
+class UtilityFunctionScope {
+  Process *m_process;
+
+public:
+  UtilityFunctionScope(Process *p) : m_process(p) {
+    if (m_process)
+      m_process->SetRunningUtilityFunction(true);
+  }
+  ~UtilityFunctionScope() {
+    if (m_process)
+      m_process->SetRunningUtilityFunction(false);
+  }
+};
+
 } // namespace lldb_private
 
 #endif // liblldb_Process_h_

include/lldb/Target/Target.h

#endif
   void SetResultIsInternal(bool b) { m_result_is_internal = b; }
 
   bool GetResultIsInternal() const { return m_result_is_internal; }
 
   void SetAutoApplyFixIts(bool b) { m_auto_apply_fixits = b; }
 
   bool GetAutoApplyFixIts() const { return m_auto_apply_fixits; }
 
+  bool IsForUtilityExpr() const { return m_running_utility_expression; }
+
+  void SetIsForUtilityExpr(bool b) { m_running_utility_expression = b; }
+
 private:
   ExecutionPolicy m_execution_policy = default_execution_policy;
   lldb::LanguageType m_language = lldb::eLanguageTypeUnknown;
   std::string m_prefix;
   bool m_coerce_to_id = false;
   bool m_unwind_on_error = true;
   bool m_ignore_breakpoints = false;
   bool m_keep_in_memory = false;
   bool m_try_others = true;
   bool m_stop_others = true;
   bool m_debug = false;
   bool m_trap_exceptions = true;
   bool m_repl = false;
   bool m_generate_debug_info = false;
   bool m_ansi_color_errors = false;
   bool m_result_is_internal = false;
   bool m_auto_apply_fixits = true;
+  /// True if the executed code should be treated as utility code that is only
+  /// used by LLDB internally.
+  bool m_running_utility_expression = false;
+
   lldb::DynamicValueType m_use_dynamic = lldb::eNoDynamicValues;
   Timeout<std::micro> m_timeout = default_timeout;
   Timeout<std::micro> m_one_thread_timeout = llvm::None;
   lldb::ExpressionCancelCallback m_cancel_callback = nullptr;
   void *m_cancel_callback_baton = nullptr;
   // If m_pound_line_file is not empty and m_pound_line_line is non-zero, use
   // #line %u "%s" before the expression content to remap where the source
   // originates

source/Core/Debugger.cpp

else
       err = GetErrorFile();
 
     // If there is nothing, use stderr
     if (!err)
       err = std::make_shared<StreamFile>(stdout, false);
   }
 }
 
-void Debugger::PushIOHandler(const IOHandlerSP &reader_sp) {
+void Debugger::PushIOHandler(const IOHandlerSP &reader_sp,
+                             bool cancel_top_handler) {
   if (!reader_sp)
     return;
 
   std::lock_guard<std::recursive_mutex> guard(m_input_reader_stack.GetMutex());
 
   // Get the current top input reader...
   IOHandlerSP top_reader_sp(m_input_reader_stack.Top());
 
   // Don't push the same IO handler twice...
   if (reader_sp == top_reader_sp)
     return;
 
   // Push our new input reader
   m_input_reader_stack.Push(reader_sp);
   reader_sp->Activate();
 
   // Interrupt the top input reader to it will exit its Run() function and let
   // this new input reader take over
   if (top_reader_sp) {
     top_reader_sp->Deactivate();
+    if (cancel_top_handler)
       top_reader_sp->Cancel();
   }
 }
 
 bool Debugger::PopIOHandler(const IOHandlerSP &pop_reader_sp) {
   if (!pop_reader_sp)
     return false;
 
   std::lock_guard<std::recursive_mutex> guard(m_input_reader_stack.GetMutex());

source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp

bool AppleObjCRuntime::GetObjectDescription(Stream &strm, Value &value,
   }
 
   EvaluateExpressionOptions options;
   options.SetUnwindOnError(true);
   options.SetTryAllThreads(true);
   options.SetStopOthers(true);
   options.SetIgnoreBreakpoints(true);
   options.SetTimeout(g_po_function_timeout);
+  options.SetIsForUtilityExpr(true);
 
   ExpressionResults results = m_print_object_caller_up->ExecuteFunction(
       exe_ctx, &wrapper_struct_addr, options, diagnostics, ret);
   if (results != eExpressionCompleted) {
     strm.Printf("Error evaluating Print Object function: %d.\n", results);
     return false;
   }
 

source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp

AppleObjCRuntimeV2::UpdateISAToDescriptorMapDynamic(
   if (get_class_info_function->WriteFunctionArguments(
           exe_ctx, m_get_class_info_args, arguments, diagnostics)) {
     EvaluateExpressionOptions options;
     options.SetUnwindOnError(true);
     options.SetTryAllThreads(false);
     options.SetStopOthers(true);
     options.SetIgnoreBreakpoints(true);
     options.SetTimeout(g_utility_function_timeout);
+    options.SetIsForUtilityExpr(true);
 
     Value return_value;
     return_value.SetValueType(Value::eValueTypeScalar);
     // return_value.SetContext (Value::eContextTypeClangType,
     // clang_uint32_t_type);
     return_value.SetCompilerType(clang_uint32_t_type);
     return_value.GetScalar() = 0;
 
if (get_shared_cache_class_info_function->WriteFunctionArguments(
           exe_ctx, m_get_shared_cache_class_info_args, arguments,
           diagnostics)) {
     EvaluateExpressionOptions options;
     options.SetUnwindOnError(true);
     options.SetTryAllThreads(false);
     options.SetStopOthers(true);
     options.SetIgnoreBreakpoints(true);
     options.SetTimeout(g_utility_function_timeout);
+    options.SetIsForUtilityExpr(true);
 
     Value return_value;
     return_value.SetValueType(Value::eValueTypeScalar);
     // return_value.SetContext (Value::eContextTypeClangType,
     // clang_uint32_t_type);
     return_value.SetCompilerType(clang_uint32_t_type);
     return_value.GetScalar() = 0;
 

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process,
   EvaluateExpressionOptions options;
   options.SetExecutionPolicy(eExecutionPolicyAlways);
   options.SetLanguage(eLanguageTypeC_plus_plus);
   options.SetIgnoreBreakpoints(true);
   options.SetUnwindOnError(true);
   options.SetTrapExceptions(false); // dlopen can't throw exceptions, so
                                     // don't do the work to trap them.
   options.SetTimeout(std::chrono::seconds(2));
-  
+  options.SetIsForUtilityExpr(true);
   Value return_value;
   // Fetch the clang types we will need:
   ClangASTContext *ast = process->GetTarget().GetScratchClangASTContext();
 
   CompilerType clang_void_pointer_type
       = ast->GetBasicType(eBasicTypeVoid).GetPointerType();
 
   return_value.SetCompilerType(clang_void_pointer_type);

source/Plugins/SystemRuntime/MacOSX/AppleGetItemInfoHandler.cpp

AppleGetItemInfoHandler::GetItemInfo(Thread &thread, uint64_t item,
   DiagnosticManager diagnostics;
   ExecutionContext exe_ctx;
   EvaluateExpressionOptions options;
   options.SetUnwindOnError(true);
   options.SetIgnoreBreakpoints(true);
   options.SetStopOthers(true);
   options.SetTimeout(std::chrono::milliseconds(500));
   options.SetTryAllThreads(false);
+  options.SetIsForUtilityExpr(true);
   thread.CalculateExecutionContext(exe_ctx);
 
   if (!m_get_item_info_impl_code) {
     error.SetErrorString("Unable to compile function to call "
                          "__introspection_dispatch_queue_item_get_info");
     return return_value;
   }
 

source/Plugins/SystemRuntime/MacOSX/AppleGetPendingItemsHandler.cpp

FunctionCaller *get_pending_items_caller =
       m_get_pending_items_impl_code->GetFunctionCaller();
 
   EvaluateExpressionOptions options;
   options.SetUnwindOnError(true);
   options.SetIgnoreBreakpoints(true);
   options.SetStopOthers(true);
   options.SetTimeout(std::chrono::milliseconds(500));
   options.SetTryAllThreads(false);
+  options.SetIsForUtilityExpr(true);
   thread.CalculateExecutionContext(exe_ctx);
 
   if (get_pending_items_caller == NULL) {
     error.SetErrorString("Unable to compile function to call "
                          "__introspection_dispatch_queue_get_pending_items");
     return return_value;
   }
 

source/Plugins/SystemRuntime/MacOSX/AppleGetQueuesHandler.cpp

AppleGetQueuesHandler::GetCurrentQueues(Thread &thread, addr_t page_to_free,
   DiagnosticManager diagnostics;
   ExecutionContext exe_ctx;
   EvaluateExpressionOptions options;
   options.SetUnwindOnError(true);
   options.SetIgnoreBreakpoints(true);
   options.SetStopOthers(true);
   options.SetTimeout(std::chrono::milliseconds(500));
   options.SetTryAllThreads(false);
+  options.SetIsForUtilityExpr(true);
   thread.CalculateExecutionContext(exe_ctx);
 
   ExpressionResults func_call_ret;
   Value results;
   func_call_ret = get_queues_caller->ExecuteFunction(
       exe_ctx, &args_addr, options, diagnostics, results);
   if (func_call_ret != eExpressionCompleted || !error.Success()) {
     if (log)

source/Plugins/SystemRuntime/MacOSX/AppleGetThreadItemInfoHandler.cpp

AppleGetThreadItemInfoHandler::GetThreadItemInfo(Thread &thread,
   EvaluateExpressionOptions options;
   FunctionCaller *get_thread_item_info_caller = nullptr;
 
   options.SetUnwindOnError(true);
   options.SetIgnoreBreakpoints(true);
   options.SetStopOthers(true);
   options.SetTimeout(std::chrono::milliseconds(500));
   options.SetTryAllThreads(false);
+  options.SetIsForUtilityExpr(true);
   thread.CalculateExecutionContext(exe_ctx);
 
   if (!m_get_thread_item_info_impl_code) {
     error.SetErrorString("Unable to compile function to call "
                          "__introspection_dispatch_thread_get_item_info");
     return return_value;
   }
 

source/Target/Process.cpp

if (log)
           StateAsCString(new_state));
   }
 }
 
 void Process::SetRunningUserExpression(bool on) {
   m_mod_id.SetRunningUserExpression(on);
 }
 
+void Process::SetRunningUtilityFunction(bool on) {
+  m_mod_id.SetRunningUtilityFunction(on);
+}
+
 addr_t Process::GetImageInfoAddress() { return LLDB_INVALID_ADDRESS; }
 
 const lldb::ABISP &Process::GetABI() {
   if (!m_abi_sp)
     m_abi_sp = ABI::FindPlugin(shared_from_this(), GetTarget().GetArchitecture());
   return m_abi_sp;
 }
 

 bool Process::PushProcessIOHandler() {
   IOHandlerSP io_handler_sp(m_process_input_reader);
   if (io_handler_sp) {
     Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
     if (log)
       log->Printf("Process::%s pushing IO handler", __FUNCTION__);
 
     io_handler_sp->SetIsDone(false);
-    GetTarget().GetDebugger().PushIOHandler(io_handler_sp);
+    // If we evaluate an utility function, then we don't cancel the current
+    // IOHandler. Our IOHandler is non-interactive and shouldn't disturb the
+    // existing IOHandler that potentially provides the user interface (e.g.
+    // the IOHandler for Editline).
+    bool cancel_top_handler = !m_mod_id.IsRunningUtilityFunction();
+    GetTarget().GetDebugger().PushIOHandler(io_handler_sp, cancel_top_handler);
     return true;
   }
   return false;
 }
 
 bool Process::PopProcessIOHandler() {
   IOHandlerSP io_handler_sp(m_process_input_reader);
   if (io_handler_sp)
Process::RunThreadPlan(ExecutionContext &exe_ctx,
 
   // The plans run with RunThreadPlan also need to be terminal master plans or
   // when they are done we will end up asking the plan above us whether we
   // should stop, which may give the wrong answer.
 
   thread_plan_sp->SetIsMasterPlan(true);
   thread_plan_sp->SetOkayToDiscard(false);
 
+  // If we are running some utility expression for LLDB, we now have to mark
+  // this in the ProcesModID of this process. This RAII takes care of marking
+  // and reverting the mark it once we are done running the expression.
+  UtilityFunctionScope util_scope(options.IsForUtilityExpr() ? this : nullptr);
+
   if (m_private_state.GetValue() != eStateStopped) {
     diagnostic_manager.PutString(
         eDiagnosticSeverityError,
         "RunThreadPlan called while the private state was not stopped.");
     return eExpressionSetupError;
   }
 
   // Save the thread & frame from the exe_ctx for restoration after we run