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

Author: scallanan

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 @@ class BreakpointLocation :
     //     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 @@ class BreakpointLocation :
     Breakpoint &m_owner; ///< The breakpoint that produced this object.
     std::unique_ptr<BreakpointOptions> 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);

lldb/include/lldb/Breakpoint/BreakpointOptions.h

@@ -183,7 +183,7 @@ class BreakpointOptions
     ///    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 @@ class BreakpointOptions
     bool m_one_shot;
     uint32_t m_ignore_count; // Number of times to ignore this breakpoint
     std::unique_ptr<ThreadSpec> m_thread_spec_ap; // Thread for which this breakpoint will take
-    std::unique_ptr<ClangUserExpression> 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

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 @@ class ClangUserExpression : public ClangExpression
         return m_can_interpret;
     }
 
+    bool
+    MatchesContext (ExecutionContext &exe_ctx);
+    
     //------------------------------------------------------------------
     /// Execute the parsed expression
     ///
@@ -383,34 +387,16 @@ class ClangUserExpression : public ClangExpression
                                    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

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

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<std::string> 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());
         }
     }    
 }

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;
     }