From 3dbf346ef358004c213774c7bb83e47687ced102 Mon Sep 17 00:00:00 2001 From: Sean Callanan Date: Fri, 19 Apr 2013 07:09:15 +0000 Subject: [PATCH] Optimized the way breakpoint conditions are evaluated. Previously, the options for a breakopint or its locations stored only the text of the breakpoint condition (ironically, they used ClangUserExpression as a glorified std::string) and, each time the condition had to be evaluated in the StopInfo code, the expression parser would be invoked via a static method to parse and then execute the expression. I made several changes here: - Each breakpoint location now has its own ClangUserExpressionSP containing a version of the breakpoint expression compiled for that exact location. - Whenever the breakpoint is hit, the breakpoint condition expression is simply re-run to determine whether to stop. - If the process changes (e.g., it's re-run) or the source code of the expression changes (we use a hash so as to avoid doing string comparisons) the ClangUserExpressionSP is re-generated. This should improve performance of breakpoint conditions significantly, and takes advantage of the recent expression re-use work. llvm-svn: 179838 --- .../lldb/Breakpoint/BreakpointLocation.h | 8 +- .../lldb/Breakpoint/BreakpointOptions.h | 6 +- .../lldb/Expression/ClangUserExpression.h | 28 ++--- lldb/source/Breakpoint/BreakpointLocation.cpp | 104 +++++++++++++++++- lldb/source/Breakpoint/BreakpointOptions.cpp | 43 ++++---- .../source/Expression/ClangUserExpression.cpp | 50 ++++++++- lldb/source/Target/StopInfo.cpp | 65 ++--------- 7 files changed, 201 insertions(+), 103 deletions(-) diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h index 014a9a39d9a4..efdc13851986 100644 --- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h +++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h @@ -24,6 +24,7 @@ #include "lldb/Core/Address.h" #include "lldb/Target/Process.h" #include "lldb/Core/StringList.h" +#include "lldb/Expression/ClangUserExpression.h" namespace lldb_private { @@ -175,7 +176,10 @@ public: // condition has been set. //------------------------------------------------------------------ const char * - GetConditionText () const; + GetConditionText (size_t *hash = NULL) const; + + bool + ConditionSaysStop (ExecutionContext &exe_ctx, Error &error); //------------------------------------------------------------------ @@ -381,6 +385,8 @@ private: Breakpoint &m_owner; ///< The breakpoint that produced this object. std::unique_ptr m_options_ap; ///< Breakpoint options pointer, NULL if we're using our breakpoint's options. lldb::BreakpointSiteSP m_bp_site_sp; ///< Our breakpoint site (it may be shared by more than one location.) + ClangUserExpression::ClangUserExpressionSP m_user_expression_sp; ///< The compiled expression to use in testing our condition. + size_t m_condition_hash; ///< For testing whether the condition source code changed. void SendBreakpointLocationChangedEvent (lldb::BreakpointEventType eventKind); diff --git a/lldb/include/lldb/Breakpoint/BreakpointOptions.h b/lldb/include/lldb/Breakpoint/BreakpointOptions.h index 8d26b448e2e5..728f5932fa06 100644 --- a/lldb/include/lldb/Breakpoint/BreakpointOptions.h +++ b/lldb/include/lldb/Breakpoint/BreakpointOptions.h @@ -183,7 +183,7 @@ public: /// A pointer to the condition expression text, or NULL if no // condition has been set. //------------------------------------------------------------------ - const char *GetConditionText () const; + const char *GetConditionText (size_t *hash = NULL) const; //------------------------------------------------------------------ // Enabled/Ignore Count @@ -349,8 +349,8 @@ private: bool m_one_shot; uint32_t m_ignore_count; // Number of times to ignore this breakpoint std::unique_ptr m_thread_spec_ap; // Thread for which this breakpoint will take - std::unique_ptr m_condition_ap; // The condition to test. - + std::string m_condition_text; // The condition to test. + size_t m_condition_text_hash; // Its hash, so that locations know when the condition is updated. }; } // namespace lldb_private diff --git a/lldb/include/lldb/Expression/ClangUserExpression.h b/lldb/include/lldb/Expression/ClangUserExpression.h index 0caa9dcaf7f2..c3c8178564eb 100644 --- a/lldb/include/lldb/Expression/ClangUserExpression.h +++ b/lldb/include/lldb/Expression/ClangUserExpression.h @@ -21,6 +21,7 @@ #include "lldb/lldb-forward.h" #include "lldb/lldb-private.h" +#include "lldb/Core/Address.h" #include "lldb/Core/ClangForward.h" #include "lldb/Expression/ClangExpression.h" #include "lldb/Expression/ClangExpressionVariable.h" @@ -112,6 +113,9 @@ public: return m_can_interpret; } + bool + MatchesContext (ExecutionContext &exe_ctx); + //------------------------------------------------------------------ /// Execute the parsed expression /// @@ -383,34 +387,16 @@ private: lldb::addr_t &cmd_ptr); void - InstallContext (ExecutionContext &exe_ctx) - { - m_process_wp = exe_ctx.GetProcessSP(); - m_target_wp = exe_ctx.GetTargetSP(); - m_frame_wp = exe_ctx.GetFrameSP(); - } + InstallContext (ExecutionContext &exe_ctx); bool LockAndCheckContext (ExecutionContext &exe_ctx, lldb::TargetSP &target_sp, lldb::ProcessSP &process_sp, - lldb::StackFrameSP &frame_sp) - { - target_sp = m_target_wp.lock(); - process_sp = m_process_wp.lock(); - frame_sp = m_frame_wp.lock(); - - if ((target_sp && target_sp.get() != exe_ctx.GetTargetPtr()) || - (process_sp && process_sp.get() != exe_ctx.GetProcessPtr()) || - (frame_sp && frame_sp.get() != exe_ctx.GetFramePtr())) - return false; - - return true; - } + lldb::StackFrameSP &frame_sp); - lldb::TargetWP m_target_wp; ///< The target used as the context for the expression. lldb::ProcessWP m_process_wp; ///< The process used as the context for the expression. - lldb::StackFrameWP m_frame_wp; ///< The stack frame used as context for the expression. + Address m_address; ///< The address the process is stopped in. std::string m_expr_text; ///< The text of the expression, as typed by the user std::string m_expr_prefix; ///< The text of the translation-level definitions, as provided by the user diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp index 71e8eb7c8687..04142b268e48 100644 --- a/lldb/source/Breakpoint/BreakpointLocation.cpp +++ b/lldb/source/Breakpoint/BreakpointLocation.cpp @@ -240,9 +240,109 @@ BreakpointLocation::SetCondition (const char *condition) } const char * -BreakpointLocation::GetConditionText () const +BreakpointLocation::GetConditionText (size_t *hash) const { - return GetOptionsNoCreate()->GetConditionText(); + return GetOptionsNoCreate()->GetConditionText(hash); +} + +bool +BreakpointLocation::ConditionSaysStop (ExecutionContext &exe_ctx, Error &error) +{ + Log *log = lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_BREAKPOINTS); + + size_t condition_hash; + const char *condition_text = GetConditionText(&condition_hash); + + if (!condition_text) + return false; + + if (condition_hash != m_condition_hash || + !m_user_expression_sp || + !m_user_expression_sp->MatchesContext(exe_ctx)) + { + m_user_expression_sp.reset(new ClangUserExpression(condition_text, + NULL, + lldb::eLanguageTypeUnknown, + ClangUserExpression::eResultTypeAny)); + + StreamString errors; + + if (!m_user_expression_sp->Parse(errors, + exe_ctx, + eExecutionPolicyOnlyWhenNeeded, + true)) + { + error.SetErrorStringWithFormat("Couldn't parse conditional expression:\n%s", + errors.GetData()); + m_user_expression_sp.reset(); + return false; + } + + m_condition_hash = condition_hash; + } + + // We need to make sure the user sees any parse errors in their condition, so we'll hook the + // constructor errors up to the debugger's Async I/O. + + ValueObjectSP result_value_sp; + const bool unwind_on_error = true; + const bool ignore_breakpoints = true; + const bool try_all_threads = true; + + Error expr_error; + + StreamString execution_errors; + + ClangExpressionVariableSP result_variable_sp; + + ExecutionResults result_code = + m_user_expression_sp->Execute(execution_errors, + exe_ctx, + unwind_on_error, + ignore_breakpoints, + m_user_expression_sp, + result_variable_sp, + try_all_threads, + ClangUserExpression::kDefaultTimeout); + + bool ret; + + if (result_code == eExecutionCompleted) + { + result_value_sp = result_variable_sp->GetValueObject(); + + if (result_value_sp) + { + Scalar scalar_value; + if (result_value_sp->ResolveValue (scalar_value)) + { + if (scalar_value.ULongLong(1) == 0) + ret = false; + else + ret = true; + if (log) + log->Printf("Condition successfully evaluated, result is %s.\n", + ret ? "true" : "false"); + } + else + { + ret = false; + error.SetErrorString("Failed to get an integer result from the expression"); + } + } + else + { + ret = false; + error.SetErrorString("Failed to get any result from the expression"); + } + } + else + { + ret = false; + error.SetErrorStringWithFormat("Couldn't execute expression:\n%s", execution_errors.GetData()); + } + + return ret; } uint32_t diff --git a/lldb/source/Breakpoint/BreakpointOptions.cpp b/lldb/source/Breakpoint/BreakpointOptions.cpp index 11335fb1db9d..d0aaf47d8a63 100644 --- a/lldb/source/Breakpoint/BreakpointOptions.cpp +++ b/lldb/source/Breakpoint/BreakpointOptions.cpp @@ -42,7 +42,8 @@ BreakpointOptions::BreakpointOptions() : m_one_shot (false), m_ignore_count (0), m_thread_spec_ap (), - m_condition_ap() + m_condition_text (), + m_condition_text_hash (0) { } @@ -56,13 +57,12 @@ BreakpointOptions::BreakpointOptions(const BreakpointOptions& rhs) : m_enabled (rhs.m_enabled), m_one_shot (rhs.m_one_shot), m_ignore_count (rhs.m_ignore_count), - m_thread_spec_ap (), - m_condition_ap () + m_thread_spec_ap () { if (rhs.m_thread_spec_ap.get() != NULL) m_thread_spec_ap.reset (new ThreadSpec(*rhs.m_thread_spec_ap.get())); - if (rhs.m_condition_ap.get()) - m_condition_ap.reset (new ClangUserExpression (rhs.m_condition_ap->GetUserText(), NULL, lldb::eLanguageTypeUnknown, ClangUserExpression::eResultTypeAny)); + m_condition_text = rhs.m_condition_text; + m_condition_text_hash = rhs.m_condition_text_hash; } //---------------------------------------------------------------------- @@ -79,8 +79,8 @@ BreakpointOptions::operator=(const BreakpointOptions& rhs) m_ignore_count = rhs.m_ignore_count; if (rhs.m_thread_spec_ap.get() != NULL) m_thread_spec_ap.reset(new ThreadSpec(*rhs.m_thread_spec_ap.get())); - if (rhs.m_condition_ap.get()) - m_condition_ap.reset (new ClangUserExpression (rhs.m_condition_ap->GetUserText(), NULL, lldb::eLanguageTypeUnknown, ClangUserExpression::eResultTypeAny)); + m_condition_text = rhs.m_condition_text; + m_condition_text_hash = rhs.m_condition_text_hash; return *this; } @@ -162,24 +162,25 @@ BreakpointOptions::HasCallback () void BreakpointOptions::SetCondition (const char *condition) { - if (condition == NULL || condition[0] == '\0') - { - if (m_condition_ap.get()) - m_condition_ap.reset(); - } - else - { - m_condition_ap.reset(new ClangUserExpression (condition, NULL, lldb::eLanguageTypeUnknown, ClangUserExpression::eResultTypeAny)); - } + m_condition_text.assign(condition); + std::hash hasher; + m_condition_text_hash = hasher(m_condition_text); } const char * -BreakpointOptions::GetConditionText () const +BreakpointOptions::GetConditionText (size_t *hash) const { - if (m_condition_ap.get()) - return m_condition_ap->GetUserText(); + if (!m_condition_text.empty()) + { + if (hash) + *hash = m_condition_text_hash; + + return m_condition_text.c_str(); + } else + { return NULL; + } } const ThreadSpec * @@ -250,12 +251,12 @@ BreakpointOptions::GetDescription (Stream *s, lldb::DescriptionLevel level) cons m_callback_baton_sp->GetDescription (s, level); } } - if (m_condition_ap.get()) + if (!m_condition_text.empty()) { if (level != eDescriptionLevelBrief) { s->EOL(); - s->Printf("Condition: %s\n", m_condition_ap->GetUserText()); + s->Printf("Condition: %s\n", m_condition_text.c_str()); } } } diff --git a/lldb/source/Expression/ClangUserExpression.cpp b/lldb/source/Expression/ClangUserExpression.cpp index da1996a93396..6928d164a5ed 100644 --- a/lldb/source/Expression/ClangUserExpression.cpp +++ b/lldb/source/Expression/ClangUserExpression.cpp @@ -329,6 +329,54 @@ ClangUserExpression::ScanContext(ExecutionContext &exe_ctx, Error &err) } } +void +ClangUserExpression::InstallContext (ExecutionContext &exe_ctx) +{ + m_process_wp = exe_ctx.GetProcessSP(); + + lldb::StackFrameSP frame_sp = exe_ctx.GetFrameSP(); + + if (frame_sp) + m_address = frame_sp->GetFrameCodeAddress(); +} + +bool +ClangUserExpression::LockAndCheckContext (ExecutionContext &exe_ctx, + lldb::TargetSP &target_sp, + lldb::ProcessSP &process_sp, + lldb::StackFrameSP &frame_sp) +{ + lldb::ProcessSP expected_process_sp = m_process_wp.lock(); + process_sp = exe_ctx.GetProcessSP(); + + if (process_sp != expected_process_sp) + return false; + + process_sp = exe_ctx.GetProcessSP(); + target_sp = exe_ctx.GetTargetSP(); + frame_sp = exe_ctx.GetFrameSP(); + + if (m_address.IsValid()) + { + if (!frame_sp) + return false; + else + return (0 == Address::CompareLoadAddress(m_address, frame_sp->GetFrameCodeAddress(), target_sp.get())); + } + + return true; +} + +bool +ClangUserExpression::MatchesContext (ExecutionContext &exe_ctx) +{ + lldb::TargetSP target_sp; + lldb::ProcessSP process_sp; + lldb::StackFrameSP frame_sp; + + return LockAndCheckContext(exe_ctx, target_sp, process_sp, frame_sp); +} + // This is a really nasty hack, meant to fix Objective-C expressions of the form // (int)[myArray count]. Right now, because the type information for count is // not available, [myArray count] returns id, which can't be directly cast to @@ -545,7 +593,7 @@ ClangUserExpression::PrepareToExecuteJITExpression (Stream &error_stream, process, frame)) { - error_stream.Printf("The context has changed before we could JIT the expression!"); + error_stream.Printf("The context has changed before we could JIT the expression!\n"); return false; } diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 299c303588aa..c9df372492b5 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -387,79 +387,36 @@ protected: // First run the condition for the breakpoint. If that says we should stop, then we'll run // the callback for the breakpoint. If the callback says we shouldn't stop that will win. - bool condition_says_stop = true; if (bp_loc_sp->GetConditionText() != NULL) { - // We need to make sure the user sees any parse errors in their condition, so we'll hook the - // constructor errors up to the debugger's Async I/O. + Error condition_error; + bool condition_says_stop = bp_loc_sp->ConditionSaysStop(exe_ctx, condition_error); - ValueObjectSP result_valobj_sp; - - ExecutionResults result_code; - ValueObjectSP result_value_sp; - const bool unwind_on_error = true; - const bool ignore_breakpoints = true; - Error error; - result_code = ClangUserExpression::EvaluateWithError (exe_ctx, - eExecutionPolicyOnlyWhenNeeded, - lldb::eLanguageTypeUnknown, - ClangUserExpression::eResultTypeAny, - unwind_on_error, - ignore_breakpoints, - bp_loc_sp->GetConditionText(), - NULL, - result_value_sp, - error, - true, - ClangUserExpression::kDefaultTimeout); - if (result_code == eExecutionCompleted) - { - if (result_value_sp) - { - Scalar scalar_value; - if (result_value_sp->ResolveValue (scalar_value)) - { - if (scalar_value.ULongLong(1) == 0) - condition_says_stop = false; - else - condition_says_stop = true; - if (log) - log->Printf("Condition successfully evaluated, result is %s.\n", - m_should_stop ? "true" : "false"); - } - else - { - condition_says_stop = true; - if (log) - log->Printf("Failed to get an integer result from the expression."); - } - } - } - else + if (!condition_error.Success()) { Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger(); StreamSP error_sp = debugger.GetAsyncErrorStream (); error_sp->Printf ("Stopped due to an error evaluating condition of breakpoint "); bp_loc_sp->GetDescription (error_sp.get(), eDescriptionLevelBrief); - error_sp->Printf (": \"%s\"", + error_sp->Printf (": \"%s\"", bp_loc_sp->GetConditionText()); error_sp->EOL(); - const char *err_str = error.AsCString(""); + const char *err_str = condition_error.AsCString(""); if (log) log->Printf("Error evaluating condition: \"%s\"\n", err_str); error_sp->PutCString (err_str); - error_sp->EOL(); + error_sp->EOL(); error_sp->Flush(); // If the condition fails to be parsed or run, we should stop. condition_says_stop = true; } + else + { + if (!condition_says_stop) + continue; + } } - - // If this location's condition says we should aren't going to stop, - // then don't run the callback for this location. - if (!condition_says_stop) - continue; bool callback_says_stop;