Make sure we are stopped before we try to install functions into the target

[jingham]

Summary

It's actually fairly hard to get lldb to do this, since most ways you can access the expression evaluator already assert that you have to be stopped. But if you are unlucky you might end up doing that, for instance SBTarget::FindFirstType when called the first time for something that requires types from the ObjC runtime metadata can end up installing and calling the "metadata introspection" UtilityFunction. This fails, mostly in harmless ways, but if you happen to hit a breakpoint and stop while in the middle of making up the expression, you can mess up internal state. I really don't think it's worth the effort to make this work. It's easier to reason about if we just assert that you have to be stopped if you want to cons up a UtilityExpression.

The only subtle bit of this patch is that if code is running on the Internal State Thread, it is governed by the m_private_state, not the m_public_state. So I made Process::GetState behave the same way the target API lock does, with a separate entity for the private & public running code.

[jasonmolenda]

This looks good to me, I like all of the added error messages in FunctionCaller::WriteFunctionWrapper in particular.

lldb/source/Expression/FunctionCaller.cpp

if (jit_module_sp)
       process_sp->GetTarget().GetImages().Remove(jit_module_sp);
   }
 }
 
 bool FunctionCaller::WriteFunctionWrapper(
     ExecutionContext &exe_ctx, DiagnosticManager &diagnostic_manager) {
   Process *process = exe_ctx.GetProcessPtr();
 
-  if (!process)
+  if (!process) {
+    diagnostic_manager.Printf(eDiagnosticSeverityError, "no process.");
     return false;
+  }
   
   lldb::ProcessSP jit_process_sp(m_jit_process_wp.lock());
 
-  if (process != jit_process_sp.get())
+  if (process != jit_process_sp.get()) {
+    diagnostic_manager.Printf(eDiagnosticSeverityError,
+                             "process does not match the stored process.");
     return false;
+  }
     
-  if (!m_compiled)
+  if (process->GetState() != lldb::eStateStopped) {
+    diagnostic_manager.Printf(eDiagnosticSeverityError, 
+                              "process is not stopped");
     return false;
+  }
+
+  if (!m_compiled) {
+    diagnostic_manager.Printf(eDiagnosticSeverityError, 
+                              "function not compiled");
+    return false;
+  }
   
   if (m_JITted)
     return true;
 
   bool can_interpret = false; // should stay that way
 
   Status jit_error(m_parser->PrepareForExecution(
       m_jit_start_addr, m_jit_end_addr, m_execution_unit_sp, exe_ctx,
       can_interpret, eExecutionPolicyAlways));
bool FunctionCaller::WriteFunctionArguments(
   }
 
   return true;
 }
 
 bool FunctionCaller::InsertFunction(ExecutionContext &exe_ctx,
                                     lldb::addr_t &args_addr_ref,
                                     DiagnosticManager &diagnostic_manager) {
+  // Since we might need to call allocate memory and maybe call code to make
+  // the caller, we need to be stopped.
+  Process *process = exe_ctx.GetProcessPtr();
+  if (!process) {
+    diagnostic_manager.PutString(eDiagnosticSeverityError, "no process");
+    return false;
+  }
+  if (process->GetState() != lldb::eStateStopped) {
+    diagnostic_manager.PutString(eDiagnosticSeverityError, "process running");
+    return false;
+  }
   if (CompileFunction(exe_ctx.GetThreadSP(), diagnostic_manager) != 0)
     return false;
   if (!WriteFunctionWrapper(exe_ctx, diagnostic_manager))
     return false;
   if (!WriteFunctionArguments(exe_ctx, args_addr_ref, diagnostic_manager))
     return false;
 
   Log *log = GetLog(LLDBLog::Step);

lldb/source/Expression/UserExpression.cpp

if (!target) {
     LLDB_LOG(log, "== [UserExpression::Evaluate] Passed a NULL target, can't "
                   "run expressions.");
     error.SetErrorString("expression passed a null target");
     return lldb::eExpressionSetupError;
   }
 
   Process *process = exe_ctx.GetProcessPtr();
 
-  if (process == nullptr || process->GetState() != lldb::eStateStopped) {
-    if (execution_policy == eExecutionPolicyAlways) {
-      LLDB_LOG(log, "== [UserExpression::Evaluate] Expression may not run, but "
-                    "is not constant ==");
+  if (process == nullptr && execution_policy == eExecutionPolicyAlways) {
+    LLDB_LOG(log, "== [UserExpression::Evaluate] No process, but the policy is "
+                  "eExecutionPolicyAlways");
 
-      error.SetErrorString("expression needed to run but couldn't");
+    error.SetErrorString("expression needed to run but couldn't: no process");
 
-      return execution_results;
-    }
+  // Since we might need to call allocate memory and maybe call code to make
+  // the caller, we need to be stopped.
+  if (process != nullptr && process->GetState() != lldb::eStateStopped) {
+    error.SetErrorString("Can't make a function caller while the process is " 
+                          "running");
+    return execution_results;
   }
 
+
   // Explicitly force the IR interpreter to evaluate the expression when the
   // there is no process that supports running the expression for us. Don't
   // change the execution policy if we have the special top-level policy that
   // doesn't contain any expression and there is nothing to interpret.
   if (execution_policy != eExecutionPolicyTopLevel &&
       (process == nullptr || !process->CanJIT()))
     execution_policy = eExecutionPolicyNever;
 

lldb/source/Expression/UtilityFunction.cpp

FunctionCaller *UtilityFunction::MakeFunctionCaller(
   if (m_caller_up)
     return m_caller_up.get();
 
   ProcessSP process_sp = m_jit_process_wp.lock();
   if (!process_sp) {
     error.SetErrorString("Can't make a function caller without a process.");
     return nullptr;
   }
+  // Since we might need to call allocate memory and maybe call code to make
+  // the caller, we need to be stopped.
+  if (process_sp->GetState() != lldb::eStateStopped) {
+    error.SetErrorString("Can't make a function caller while the process is " 
+                         "running");
+    return nullptr;
+  }
 
   Address impl_code_address;
   impl_code_address.SetOffset(StartAddress());
   std::string name(m_function_name);
   name.append("-caller");
 
   m_caller_up.reset(process_sp->GetTarget().GetFunctionCallerForLanguage(
       Language(), return_type, impl_code_address, arg_value_list, name.c_str(),

lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp

bool ClangUtilityFunction::Install(DiagnosticManager &diagnostic_manager,
 
   Process *process = exe_ctx.GetProcessPtr();
 
   if (!process) {
     diagnostic_manager.PutString(eDiagnosticSeverityError, "invalid process");
     return false;
   }
 
+  // Since we might need to call allocate memory and maybe call code to make
+  // the caller, we need to be stopped.
+  if (process->GetState() != lldb::eStateStopped) {
+    diagnostic_manager.PutString(eDiagnosticSeverityError, "process running");
+    return false;
+  }
   //////////////////////////
   // Parse the expression
   //
 
   bool keep_result_in_memory = false;
 
   ResetDeclMap(exe_ctx, keep_result_in_memory);
 

lldb/source/Target/Process.cpp

uint32_t Process::AssignIndexIDToThread(uint64_t thread_id) {
   } else {
     result = iterator->second;
   }
 
   return result;
 }
 
 StateType Process::GetState() {
+  if (CurrentThreadIsPrivateStateThread())
+    return m_private_state.GetValue();
+  else
     return m_public_state.GetValue();
 }
 
 void Process::SetPublicState(StateType new_state, bool restarted) {
   const bool new_state_is_stopped = StateIsStoppedState(new_state, false);
   if (new_state_is_stopped) {
     // This will only set the time if the public stop time has no value, so
     // it is ok to call this multiple times. With a public stop we can't look
     // at the stop ID because many private stops might have happened, so we