[Lldb-commits] [PATCH] D137684: Make sure we are stopped before we try to install functions into the target
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb565e7f0c4cb: Don't try to create Expressions when the
process is running. (authored by jingham).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137684/new/
https://reviews.llvm.org/D137684
Files:
lldb/source/Expression/FunctionCaller.cpp
lldb/source/Expression/UserExpression.cpp
lldb/source/Expression/UtilityFunction.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
lldb/source/Target/Process.cpp
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -1293,7 +1293,10 @@
}
StateType Process::GetState() {
- return m_public_state.GetValue();
+ if (CurrentThreadIsPrivateStateThread())
+return m_private_state.GetValue();
+ else
+return m_public_state.GetValue();
}
void Process::SetPublicState(StateType new_state, bool restarted) {
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
@@ -99,6 +99,12 @@
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
//
Index: lldb/source/Expression/UtilityFunction.cpp
===
--- lldb/source/Expression/UtilityFunction.cpp
+++ lldb/source/Expression/UtilityFunction.cpp
@@ -64,6 +64,13 @@
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());
Index: lldb/source/Expression/UserExpression.cpp
===
--- lldb/source/Expression/UserExpression.cpp
+++ lldb/source/Expression/UserExpression.cpp
@@ -194,16 +194,22 @@
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;
-}
+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
Index: lldb/source/Expression/FunctionCaller.cpp
===
--- lldb/source/Expression/FunctionCaller.cpp
+++ lldb/source/Expression/FunctionCaller.cpp
@@ -66,17 +66,31 @@
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_manage
[Lldb-commits] [PATCH] D137684: Make sure we are stopped before we try to install functions into the target
jasonmolenda accepted this revision. jasonmolenda added a comment. This revision is now accepted and ready to land. This looks good to me, I like all of the added error messages in `FunctionCaller::WriteFunctionWrapper` in particular. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137684/new/ https://reviews.llvm.org/D137684 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D137684: Make sure we are stopped before we try to install functions into the target
jingham created this revision.
jingham added reviewers: JDevlieghere, jasonmolenda, labath, clayborg.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D137684
Files:
lldb/source/Expression/FunctionCaller.cpp
lldb/source/Expression/UserExpression.cpp
lldb/source/Expression/UtilityFunction.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
lldb/source/Target/Process.cpp
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -1293,7 +1293,10 @@
}
StateType Process::GetState() {
- return m_public_state.GetValue();
+ if (CurrentThreadIsPrivateStateThread())
+return m_private_state.GetValue();
+ else
+return m_public_state.GetValue();
}
void Process::SetPublicState(StateType new_state, bool restarted) {
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
@@ -99,6 +99,12 @@
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
//
Index: lldb/source/Expression/UtilityFunction.cpp
===
--- lldb/source/Expression/UtilityFunction.cpp
+++ lldb/source/Expression/UtilityFunction.cpp
@@ -64,6 +64,13 @@
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());
Index: lldb/source/Expression/UserExpression.cpp
===
--- lldb/source/Expression/UserExpression.cpp
+++ lldb/source/Expression/UserExpression.cpp
@@ -194,16 +194,22 @@
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;
-}
+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 ru
