fix: handle negative line number deltas (#73)

In Python 3.6-3.9, unlike in Python 2, co_lnotab's line number offsets can be negative and are represented by a signed integer. The Debugger agent had been using an unsigned integer which prevented it from being able to set breakpoints on lines of code in a code object after the line table had a negative offset.

This PR also enables most of the previously disabled tests now that they work on all supported versions of Python. A couple still remain disabled and will be fixed later on.

Author: mctavish

src/googleclouddebugger/module_explorer.py

@@ -62,7 +62,7 @@ def GetCodeObjectAtLine(module, line):
         prev_line = max(prev_line, co_line_number)
       elif co_line_number > line:
         next_line = min(next_line, co_line_number)
-        break
+        # Continue because line numbers may not be sequential.
 
   prev_line = None if prev_line == 0 else prev_line
   next_line = None if next_line == sys.maxsize else next_line
@@ -87,6 +87,9 @@ def _GetLineNumbers(code_object):
     line_incrs = code_object.co_lnotab[1::2]
     current_line = code_object.co_firstlineno
     for line_incr in line_incrs:
+      if line_incr >= 0x80:
+        # line_incrs is an array of 8-bit signed integers
+        line_incr -= 0x100
       current_line += line_incr
       yield current_line
   else:

src/googleclouddebugger/python_util.cc

@@ -79,7 +79,7 @@ bool CodeObjectLinesEnumerator::Next() {
 
   while (true) {
     offset_ += next_entry_[0];
-    line_number_ += next_entry_[1];
+    line_number_ += static_cast<int8_t>(next_entry_[1]);
 
     bool stop = ((next_entry_[0] != 0xFF) || (next_entry_[1] != 0)) &&
                 ((next_entry_[0] != 0) || (next_entry_[1] != 0xFF));

tests/py/integration_test_disabled.py tests/py/integration_test.pytests/py/integration_test_disabled.py renamed to tests/py/integration_test.py

@@ -1,7 +1,5 @@
 """Complete tests of the debugger mocking the backend."""
 
-# TODO: Get this test to work well all supported versions of python.
-
 from datetime import datetime
 from datetime import timedelta
 import functools
@@ -20,7 +18,7 @@
 import google.auth
 from absl.testing import absltest
 
-from googleclouddebugger import capture_collector
+from googleclouddebugger import collector
 from googleclouddebugger import labels
 import python_test_util
 
@@ -98,7 +96,7 @@ def GetVal():
       cdbg.enable()
 
       # Increase the polling rate to speed up the test.
-      cdbg._hub_client.min_interval_sec = 0.001  # Poll every 1 ms
+      cdbg.gcp_hub_client.min_interval_sec = 0.001  # Poll every 1 ms
 
     def SetBreakpoint(self, tag, template=None):
       """Sets a new breakpoint in this source file.
@@ -214,7 +212,6 @@ def execute(self):  # pylint: disable=invalid-name
 
       return FakeBreakpointUpdateCommand(self._incoming_breakpoint_updates)
 
-
 # We only need to attach the debugger exactly once. The IntegrationTest class
 # is created for each test case, so we need to keep this state global.
 
@@ -226,7 +223,7 @@ def _FakeLog(self, message, extra=None):
 
   def setUp(self):
     self._info_log = []
-    capture_collector.log_info_message = self._FakeLog
+    collector.log_info_message = self._FakeLog
 
   def tearDown(self):
     IntegrationTest._hub.SetActiveBreakpoints([])
@@ -281,8 +278,8 @@ def testExistingLabelsPriority(self):
     def Trigger():
       print('Breakpoint trigger with labels')  # BPTAG: EXISTING_LABELS_PRIORITY
 
-    current_labels_collector = capture_collector.breakpoint_labels_collector
-    capture_collector.breakpoint_labels_collector = \
+    current_labels_collector = collector.breakpoint_labels_collector
+    collector.breakpoint_labels_collector = \
         lambda: {'label_1': 'value_1', 'label_2': 'value_2'}
 
     IntegrationTest._hub.SetBreakpoint(
@@ -294,7 +291,7 @@ def Trigger():
 
     Trigger()
 
-    capture_collector.breakpoint_labels_collector = current_labels_collector
+    collector.breakpoint_labels_collector = current_labels_collector
 
     # In this case, label_1 was in both the agent and the pre existing labels,
     # the pre existing value of value_foobar should be preserved.
@@ -313,14 +310,14 @@ def Trigger():
       print('Breakpoint trigger req id label')  # BPTAG: REQUEST_LOG_ID_LABEL
 
     current_request_log_id_collector = \
-      capture_collector.request_log_id_collector
-    capture_collector.request_log_id_collector = lambda: 'foo_bar_id'
+      collector.request_log_id_collector
+    collector.request_log_id_collector = lambda: 'foo_bar_id'
 
     IntegrationTest._hub.SetBreakpoint('REQUEST_LOG_ID_LABEL')
 
     Trigger()
 
-    capture_collector.request_log_id_collector = \
+    collector.request_log_id_collector = \
         current_request_log_id_collector
 
     result = IntegrationTest._hub.GetNextResult()
@@ -559,23 +556,25 @@ def Method(a):
         }, python_test_util.PackFrameVariable(result, 'x', frame=1))
     return x
 
-  def testRecursion(self):
-
-    def RecursiveMethod(i):
-      if i == 0:
-        return 0  # BPTAG: RECURSION
-      return RecursiveMethod(i - 1)
-
-    IntegrationTest._hub.SetBreakpoint('RECURSION')
-    RecursiveMethod(5)
-    result = IntegrationTest._hub.GetNextResult()
 
-    for frame in range(5):
-      self.assertEqual({
-          'name': 'i',
-          'value': str(frame),
-          'type': 'int'
-      }, python_test_util.PackFrameVariable(result, 'i', frame, 'arguments'))
+# FIXME: Broken in Python 3.10
+#  def testRecursion(self):
+#
+#    def RecursiveMethod(i):
+#      if i == 0:
+#        return 0  # BPTAG: RECURSION
+#      return RecursiveMethod(i - 1)
+#
+#    IntegrationTest._hub.SetBreakpoint('RECURSION')
+#    RecursiveMethod(5)
+#    result = IntegrationTest._hub.GetNextResult()
+#
+#    for frame in range(5):
+#      self.assertEqual({
+#          'name': 'i',
+#          'value': str(frame),
+#          'type': 'int'
+#      }, python_test_util.PackFrameVariable(result, 'i', frame, 'arguments'))
 
   def testWatchedExpressions(self):
 

tests/py/module_explorer_test_disabled.py tests/py/module_explorer_test.pytests/py/module_explorer_test_disabled.py renamed to tests/py/module_explorer_test.py

@@ -1,7 +1,5 @@
 """Unit test for module_explorer module."""
 
-# TODO: Get this test to run properly on all supported versions of Python
-
 import dis
 import inspect
 import os

…ts/py/python_breakpoint_test_disabled.py tests/py/python_breakpoint_test.pytests/py/python_breakpoint_test_disabled.py renamed to tests/py/python_breakpoint_test.py tests/py/python_breakpoint_test_disabled.py renamed to tests/py/python_breakpoint_test.py

@@ -1,7 +1,5 @@
 """Unit test for python_breakpoint module."""
 
-# TODO: Get this test to work with all supported versions of Python.
-
 from datetime import datetime
 from datetime import timedelta
 import inspect
@@ -12,7 +10,7 @@
 from absl.testing import absltest
 
 from googleclouddebugger import cdbg_native as native
-from googleclouddebugger import imphook2
+from googleclouddebugger import imphook
 from googleclouddebugger import python_breakpoint
 import python_test_util
 
@@ -102,7 +100,7 @@ def testDeferredBreakpoint(self):
                      self._update_queue[0]['stackFrames'][0]['function'])
     self.assertTrue(self._update_queue[0]['isFinalState'])
 
-    self.assertEmpty(imphook2._import_callbacks)
+    self.assertEmpty(imphook._import_callbacks)
 
   # Old module search algorithm rejects multiple matches. This test verifies
   # that the new module search algorithm searches sys.path sequentially, and
@@ -142,7 +140,7 @@ def testSearchUsingSysPathOrder(self):
     self.assertEqual(
         '2', self._update_queue[0]['stackFrames'][0]['locals'][0]['value'])
 
-    self.assertEmpty(imphook2._import_callbacks)
+    self.assertEmpty(imphook._import_callbacks)
 
   # Old module search algorithm rejects multiple matches. This test verifies
   # that when the new module search cannot find any match in sys.path, it
@@ -183,7 +181,7 @@ def testMultipleDeferredMatches(self):
     self.assertEqual(
         '1', self._update_queue[0]['stackFrames'][0]['locals'][0]['value'])
 
-    self.assertEmpty(imphook2._import_callbacks)
+    self.assertEmpty(imphook._import_callbacks)
 
   def testNeverLoadedBreakpoint(self):
     open(os.path.join(self._test_package_dir, 'never_print.py'), 'w').close()
@@ -223,7 +221,7 @@ def testDeferredNoCodeAtLine(self):
     params = desc['parameters']
     self.assertIn('defer_empty.py', params[1])
     self.assertEqual(params[0], '10')
-    self.assertEmpty(imphook2._import_callbacks)
+    self.assertEmpty(imphook._import_callbacks)
 
   def testDeferredBreakpointCancelled(self):
     open(os.path.join(self._test_package_dir, 'defer_cancel.py'), 'w').close()
@@ -236,7 +234,7 @@ def testDeferredBreakpointCancelled(self):
     breakpoint.Clear()
 
     self.assertFalse(self._completed)
-    self.assertEmpty(imphook2._import_callbacks)
+    self.assertEmpty(imphook._import_callbacks)
     unused_no_code_line_above = 0  # BPTAG: NO_CODE_LINE_ABOVE
 
   # BPTAG: NO_CODE_LINE
@@ -345,7 +343,7 @@ def testNonRootInitFile(self):
       self.assertEqual('DoPrint',
                        self._update_queue[0]['stackFrames'][0]['function'])
 
-      self.assertEmpty(imphook2._import_callbacks)
+      self.assertEmpty(imphook._import_callbacks)
       self._update_queue = []
 
   def testBreakpointInLoadedPackageFile(self):
@@ -414,15 +412,26 @@ def testInvalidCondition(self):
     self.assertEqual(set(['BP_ID']), self._completed)
     self.assertLen(self._update_queue, 1)
     self.assertTrue(self._update_queue[0]['isFinalState'])
-    self.assertEqual(
-        {
-            'isError': True,
-            'refersTo': 'BREAKPOINT_CONDITION',
-            'description': {
-                'format': 'Expression could not be compiled: $0',
-                'parameters': ['unexpected EOF while parsing']
-            }
-        }, self._update_queue[0]['status'])
+    if sys.version_info.minor < 10:
+      self.assertEqual(
+          {
+              'isError': True,
+              'refersTo': 'BREAKPOINT_CONDITION',
+              'description': {
+                  'format': 'Expression could not be compiled: $0',
+                  'parameters': ['unexpected EOF while parsing']
+              }
+          }, self._update_queue[0]['status'])
+    else:
+      self.assertEqual(
+          {
+              'isError': True,
+              'refersTo': 'BREAKPOINT_CONDITION',
+              'description': {
+                  'format': 'Expression could not be compiled: $0',
+                  'parameters': ['invalid syntax']
+              }
+          }, self._update_queue[0]['status'])
 
   def testHit(self):
     breakpoint = python_breakpoint.PythonBreakpoint(self._template, self, self,