[dexter] Remove unnecessary double check on conditional breakpoints

[Orlando]

Summary

Remove the ConditionalController._conditional_met method. This was missed in the recent ConditionalController refactor (D98699). We don't need to check that the conditions for a conditional breakpoint have been met because DebuggerBase.get_triggered_breakpoint_ids returns the set of ids for breakpoints which have been triggered.

To get the "triggered breakpoints" from lldb we use GetStopReasonDataCount and GetStopReasonDataAtIndex. It seems that these functions count all breakpoints associated with the location which lldb has stopped at, regardless of their condition. i.e. Even if we have two breakpoints at the same source location that have mutually exclusive conditions, both will be found this way when either condition is true. To get around this, we store a map of breakpoint {id: condition} _breakpoint_conditions and evaluate the conditions of the triggered breakpoints to filter the set down to those which are unconditional or have a condition which evaluates to true.

Essentially we are just moving the condition double check from a general debugger controller into the lldb specific wrapper. This tidy up will help make upcoming patches simpler.

All the lit tests pass on linux (with lldb), and I've tested this on windows (with vs2017) manually as the lit tests for DexLimitSteps are currently unsupported on windows.

[chrisjackson]

One small query, otherwise lgtm.

debuginfo-tests/dexter/dex/debugger/lldb/LLDB.py

160

Are 'id' and 'bp.GetID()' the same thing?

[Orlando]

debuginfo-tests/dexter/dex/debugger/lldb/LLDB.py

160

Yes that's right.

[chrisjackson]

debuginfo-tests/dexter/dex/debugger/lldb/LLDB.py

160

In that case, would it make more sense to stick to 'id' rather than call GetID()? Apologies, I should have made it clear this is what I desired in the first comment.

[Orlando]

+ Replace two GetID() calls with id in delete_breakpoint.

[Orlando]

Thanks @chrisjackson.

debuginfo-tests/dexter/dex/debugger/DebuggerControllers/ConditionalController.py

def _set_conditional_bps(self):
         # map of {id: ConditionalBpRange}.
         for cbp in self._conditional_bp_ranges:
             for cond_expr in cbp.get_conditional_expression_list():
                 id = self.debugger.add_conditional_breakpoint(cbp.path,
                                                               cbp.range_from,
                                                               cond_expr)
                 self._conditional_bp_handles[id] = cbp
 
-    def _conditional_met(self, cbp):
-        for cond_expr in cbp.get_conditional_expression_list():
-            valueIR = self.debugger.evaluate_expression(cond_expr)
-            if valueIR.type_name == 'bool' and valueIR.value == 'true':
-                return True
-        return False
-
     def _run_debugger_custom(self):
         # TODO: Add conditional and unconditional breakpoint support to dbgeng.
         if self.debugger.get_name() == 'dbgeng':
             raise DebuggerException('DexLimitSteps commands are not supported by dbgeng')
 
         self.step_collection.clear_steps()
         self._set_conditional_bps()
 
def _run_debugger_custom(self):
             for bp_id in self.debugger.get_triggered_breakpoint_ids():
                 try:
                     # See if this is one of our conditional breakpoints.
                     cbp = self._conditional_bp_handles[bp_id]
                 except KeyError:
                     # This is an unconditional bp. Mark it for removal.
                     bp_to_delete.append(bp_id)
                     continue
-                # We have triggered a breakpoint with a condition. Check that
-                # the condition has been met.
-                if self._conditional_met(cbp):
-                    # Add a range of unconditional breakpoints covering the
-                    # lines requested in the DexLimitSteps command. Ignore
-                    # first line as that's the conditional bp we just hit and
-                    # include the final line.
-                    for line in range(cbp.range_from + 1, cbp.range_to + 1):
-                        self.debugger.add_breakpoint(cbp.path, line)
+                # Add a range of unconditional breakpoints covering the lines
+                # requested in the DexLimitSteps command. Ignore first line as
+                # that's the conditional bp we just hit and include the final
+                # line.
 
             # Remove any unconditional breakpoints we just hit.
             for bp_id in bp_to_delete:
                 self.debugger.delete_breakpoint(bp_id)
 
             self.debugger.go()
             time.sleep(self._pause_between_steps)

debuginfo-tests/dexter/dex/debugger/lldb/LLDB.py

 
 class LLDB(DebuggerBase):
     def __init__(self, context, *args):
         self.lldb_executable = context.options.lldb_executable
         self._debugger = None
         self._target = None
         self._process = None
         self._thread = None
+        # Map {id (int): condition (str)} for breakpoints which have a
+        # condition. See get_triggered_breakpoint_ids usage for more info.
+        self._breakpoint_conditions = {}
         super(LLDB, self).__init__(context, *args)
 
     def _custom_init(self):
         self._debugger = self._interface.SBDebugger.Create()
         self._debugger.SetAsync(False)
         self._target = self._debugger.CreateTargetWithFileAndArch(
             self.context.options.executable, self.context.options.arch)
         if not self._target:
def version(self):
             return self._interface.SBDebugger_GetVersionString()
         except AttributeError:
             return None
 
     def clear_breakpoints(self):
         self._target.DeleteAllBreakpoints()
 
     def _add_breakpoint(self, file_, line):
-        bp = self._target.BreakpointCreateByLocation(file_, line)
-        if not bp:
-            raise DebuggerException(
-                'could not add breakpoint [{}:{}]'.format(file_, line))
-        return bp.GetID()
+        return self._add_conditional_breakpoint(file_, line, None)
 
     def _add_conditional_breakpoint(self, file_, line, condition):
         bp = self._target.BreakpointCreateByLocation(file_, line)
-        if bp:
-            bp.SetCondition(condition)
-        else:
+        if not bp:
             raise DebuggerException(
                   'could not add breakpoint [{}:{}]'.format(file_, line))
-        return bp.GetID()
+        id = bp.GetID()
+        if condition:
+            bp.SetCondition(condition)
+            assert id not in self._breakpoint_conditions
+            self._breakpoint_conditions[id] = condition
+        return id
+
+    def _evaulate_breakpoint_condition(self, id):
+        """Evaluate the breakpoint condition and return the result.
+
+        Returns True if a conditional breakpoint with the specified id cannot
+        be found (i.e. assume it is an unconditional breakpoint).
+        """
+        try:
+            condition = self._breakpoint_conditions[id]
+        except KeyError:
+            # This must be an unconditional breakpoint.
+            return True
+        valueIR = self.evaluate_expression(condition)
+        return valueIR.type_name == 'bool' and valueIR.value == 'true'
 
     def get_triggered_breakpoint_ids(self):
         # Breakpoints can only have been triggered if we've hit one.
         stop_reason = self._translate_stop_reason(self._thread.GetStopReason())
         if stop_reason != StopReason.BREAKPOINT:
             return []
+        breakpoint_ids = set()
+        # When the stop reason is eStopReasonBreakpoint, GetStopReasonDataCount
+        # counts all breakpoints associated with the location that lldb has
+        # stopped at, regardless of their condition. I.e. Even if we have two
+        # breakpoints at the same source location that have mutually exclusive
+        # conditions, both will be counted by GetStopReasonDataCount when
+        # either condition is true. Check each breakpoint condition manually to
+        # filter the list down to breakpoints that have caused this stop.
+        #
         # Breakpoints have two data parts: Breakpoint ID, Location ID. We're
         # only interested in the Breakpoint ID so we skip every other item.
-        return set([self._thread.GetStopReasonDataAtIndex(i)
-                    for i in range(0, self._thread.GetStopReasonDataCount(), 2)])
+        for i in range(0, self._thread.GetStopReasonDataCount(), 2):
+            id = self._thread.GetStopReasonDataAtIndex(i)
+            if self._evaulate_breakpoint_condition(id):
+                breakpoint_ids.add(id)
+        return breakpoint_ids
 
     def delete_breakpoint(self, id):
         bp = self._target.FindBreakpointByID(id)
         if not bp:
             # The ID is not valid.
             raise KeyError
-        self._target.BreakpointDelete(bp.GetID())
+        try:
+            del self._breakpoint_conditions[id]
+        except KeyError:
+            # This must be an unconditional breakpoint.
+            pass
+        self._target.BreakpointDelete(id)
 
     def launch(self):
         self._process = self._target.LaunchSimple(None, None, os.getcwd())
         if not self._process or self._process.GetNumThreads() == 0:
             raise DebuggerException('could not launch process')
         if self._process.GetNumThreads() != 1:
             raise DebuggerException('multiple threads not supported')
         self._thread = self._process.GetThreadAtIndex(0)