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

435 ↗

(On Diff #161829)

@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

4692–4693 ↗

(On Diff #161829)

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 ↗

(On Diff #162227)

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 ↗

(On Diff #162227)

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 ↗

(On Diff #162227)

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.

source/Core/Debugger.cpp

void Debugger::PushIOHandler(const IOHandlerSP &reader_sp) {
   // 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();
+    // For the ProcessIO handler we don't cancel the current IOHandler. The
+    // current IOHandler shouldn't be influenced by the fact that the temporary
+    // ProcessIO handler has hijacked the IO.
+    if (reader_sp->GetType() != IOHandler::Type::ProcessIO)
       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());