From 5dc1df3c0d442dd294ec203a212d97f07a9fb175 Mon Sep 17 00:00:00 2001 From: Elizaveta Shashkova Date: Tue, 27 Feb 2018 19:48:19 +0300 Subject: [PATCH] Fixes after review (PY-26101) Fix stepping after error jump, fix tests, move code modification to an appropriate function and make it more general --- .../pydev/_pydevd_bundle/pydevd_frame.py | 2 +- .../pydevd_frame_evaluator.pyx | 7 +-- .../pydevd_frame_tracing.py | 27 +------- .../pydevd_modify_bytecode.py | 39 ++++++++---- python/helpers/pydev/pydevd.py | 5 +- .../test_bytecode_modification.py | 62 +++++++++++++++++-- .../debugger/PyCythonExtensionWarning.java | 8 +-- .../env/python/PythonDebuggerTest.java | 9 ++- 8 files changed, 102 insertions(+), 57 deletions(-) diff --git a/python/helpers/pydev/_pydevd_bundle/pydevd_frame.py b/python/helpers/pydev/_pydevd_bundle/pydevd_frame.py index 76e3d8f52082..5df7448637d6 100644 --- a/python/helpers/pydev/_pydevd_bundle/pydevd_frame.py +++ b/python/helpers/pydev/_pydevd_bundle/pydevd_frame.py @@ -560,7 +560,7 @@ class PyDBFrame: breakpoint = breakpoints_for_file[line] new_frame = frame stop = True - if (step_cmd == CMD_STEP_OVER or step_cmd == CMD_SET_NEXT_STATEMENT) and stop_frame is frame and (is_line or is_return): + if step_cmd == CMD_STEP_OVER and stop_frame is frame and (is_line or is_return): stop = False #we don't stop on breakpoint if we have to stop by step-over (it will be processed later) elif plugin_manager is not None and main_debugger.has_plugin_line_breaks: result = plugin_manager.get_breakpoint(main_debugger, self, frame, event, self._args) diff --git a/python/helpers/pydev/_pydevd_frame_eval/pydevd_frame_evaluator.pyx b/python/helpers/pydev/_pydevd_frame_eval/pydevd_frame_evaluator.pyx index 7894d855ec67..2bc0dcf01627 100644 --- a/python/helpers/pydev/_pydevd_frame_eval/pydevd_frame_evaluator.pyx +++ b/python/helpers/pydev/_pydevd_frame_eval/pydevd_frame_evaluator.pyx @@ -3,7 +3,7 @@ from _pydev_imps._pydev_saved_modules import threading from _pydevd_bundle.pydevd_additional_thread_info import PyDBAdditionalThreadInfo from _pydevd_bundle.pydevd_comm import get_global_debugger from _pydevd_bundle.pydevd_dont_trace_files import DONT_TRACE -from _pydevd_frame_eval.pydevd_frame_tracing import create_code_wrapper, update_globals_dict, dummy_tracing_holder +from _pydevd_frame_eval.pydevd_frame_tracing import pydev_trace_code_wrapper, update_globals_dict, dummy_tracing_holder from _pydevd_frame_eval.pydevd_modify_bytecode import insert_code from pydevd_file_utils import get_abs_path_real_path_and_base_from_frame, NORM_PATHS_AND_BASE_CONTAINER @@ -107,16 +107,13 @@ cdef PyObject* get_bytecode_while_frame_eval(PyFrameObject *frame_obj, int exc): code_object = frame.f_code if breakpoints: breakpoints_to_update = [] - injected_code_size = 0 for offset, line in dis.findlinestarts(code_object): if line in breakpoints: breakpoint = breakpoints[line] if code_object not in breakpoint.code_objects: # This check is needed for generator functions, because after each yield the new frame is created # but the former code object is used - injected_code = create_code_wrapper(offset + injected_code_size) - success, new_code = insert_code(frame.f_code, injected_code , line) - injected_code_size += len(injected_code.co_code) + success, new_code = insert_code(frame.f_code, pydev_trace_code_wrapper.__code__, line) if success: breakpoints_to_update.append(breakpoint) Py_INCREF(new_code) diff --git a/python/helpers/pydev/_pydevd_frame_eval/pydevd_frame_tracing.py b/python/helpers/pydev/_pydevd_frame_eval/pydevd_frame_tracing.py index 40980f1c8819..7b228a38d152 100644 --- a/python/helpers/pydev/_pydevd_frame_eval/pydevd_frame_tracing.py +++ b/python/helpers/pydev/_pydevd_frame_eval/pydevd_frame_tracing.py @@ -1,5 +1,3 @@ -from types import CodeType - import sys from _pydev_bundle import pydev_log @@ -8,7 +6,7 @@ from _pydevd_bundle.pydevd_comm import get_global_debugger, CMD_SET_BREAK, CMD_S from pydevd_file_utils import get_abs_path_real_path_and_base_from_frame, NORM_PATHS_AND_BASE_CONTAINER from _pydevd_bundle.pydevd_frame import handle_breakpoint_condition, handle_breakpoint_expression -import opcode + class DummyTracingHolder: dummy_trace_func = None @@ -90,26 +88,3 @@ def pydev_trace_code_wrapper(): # import this module again, because it's inserted inside user's code global _pydev_stop_at_break return _pydev_stop_at_break() - - - -def create_code_wrapper(offset): - co = pydev_trace_code_wrapper.__code__ - # 0 + offset LOAD_GLOBAL 0 (_pydev_stop_at_break) - # 2 + offset CALL_FUNCTION 0 - # 4 + offset POP_JUMP_IF_TRUE offset - byte_code = [116, 0, 131, 0] - if offset > 0xFF: - byte_code += [opcode.EXTENDED_ARG, offset >> 8] - byte_code += [115, offset & 0xFF] - - #below code is just function trailer and gets removed - byte_code += [100, 0, 83, 0] - return CodeType( - co.co_argcount, co.co_kwonlyargcount, co.co_nlocals, - co.co_stacksize, - co.co_flags, - bytes(byte_code), - co.co_consts, co.co_names, co.co_varnames, co.co_filename, - co.co_name, co.co_firstlineno, co.co_lnotab, co.co_freevars, - co.co_cellvars) \ No newline at end of file diff --git a/python/helpers/pydev/_pydevd_frame_eval/pydevd_modify_bytecode.py b/python/helpers/pydev/_pydevd_frame_eval/pydevd_modify_bytecode.py index 14f3a38aa4bc..86c7b8c171df 100644 --- a/python/helpers/pydev/_pydevd_frame_eval/pydevd_modify_bytecode.py +++ b/python/helpers/pydev/_pydevd_frame_eval/pydevd_modify_bytecode.py @@ -4,9 +4,10 @@ from opcode import opmap, EXTENDED_ARG, HAVE_ARGUMENT from types import CodeType MAX_BYTE = 255 +RETURN_VALUE_SIZE = 2 -def _add_attr_values_from_insert_to_original(original_code, insert_code, insert_code_obj, attribute_name, op_list): +def _add_attr_values_from_insert_to_original(original_code, insert_code, insert_code_list, attribute_name, op_list): """ This function appends values of the attribute `attribute_name` of the inserted code to the original values, and changes indexes inside inserted code. If some bytecode instruction in the inserted code used to call argument @@ -24,7 +25,7 @@ def _add_attr_values_from_insert_to_original(original_code, insert_code, insert_ orig_value = getattr(original_code, attribute_name) insert_value = getattr(insert_code, attribute_name) orig_names_len = len(orig_value) - code_with_new_values = list(insert_code_obj) + code_with_new_values = list(insert_code_list) offset = 0 while offset < len(code_with_new_values): op = code_with_new_values[offset] @@ -155,6 +156,23 @@ def _return_none_fun(): return None +def add_jump_instruction(jump_arg, code_to_insert): + """ + Add additional instruction POP_JUMP_IF_TRUE to implement a proper jump for "set next statement" action + Jump should be done to the beginning of the inserted fragment + :param jump_arg: argument for jump instruction + :param code_to_insert: code to insert + :return: a code to insert with properly added jump instruction + """ + extended_arg_list = [] + if jump_arg > MAX_BYTE: + extended_arg_list += [EXTENDED_ARG, jump_arg >> 8] + jump_arg = jump_arg & MAX_BYTE + + # remove 'RETURN_VALUE' instruction and add 'POP_JUMP_IF_TRUE' with (if needed) 'EXTENDED_ARG' + return list(code_to_insert.co_code[:-RETURN_VALUE_SIZE]) + extended_arg_list + [opmap['POP_JUMP_IF_TRUE'], jump_arg] + + def insert_code(code_to_modify, code_to_insert, before_line): """ Insert piece of code `code_to_insert` to `code_to_modify` right inside the line `before_line` before the @@ -173,19 +191,18 @@ def insert_code(code_to_modify, code_to_insert, before_line): if line_no == before_line: offset = off - return_none_size = len(_return_none_fun.__code__.co_code) - code_to_insert_obj = code_to_insert.co_code[:-return_none_size] + code_to_insert_list = add_jump_instruction(offset, code_to_insert) try: - code_to_insert_obj, new_names = \ - _add_attr_values_from_insert_to_original(code_to_modify, code_to_insert, code_to_insert_obj, 'co_names', + code_to_insert_list, new_names = \ + _add_attr_values_from_insert_to_original(code_to_modify, code_to_insert, code_to_insert_list, 'co_names', dis.hasname) - code_to_insert_obj, new_consts = \ - _add_attr_values_from_insert_to_original(code_to_modify, code_to_insert, code_to_insert_obj, 'co_consts', + code_to_insert_list, new_consts = \ + _add_attr_values_from_insert_to_original(code_to_modify, code_to_insert, code_to_insert_list, 'co_consts', [opmap['LOAD_CONST']]) - code_to_insert_obj, new_vars = \ - _add_attr_values_from_insert_to_original(code_to_modify, code_to_insert, code_to_insert_obj, 'co_varnames', + code_to_insert_list, new_vars = \ + _add_attr_values_from_insert_to_original(code_to_modify, code_to_insert, code_to_insert_list, 'co_varnames', dis.haslocal) - new_bytes, all_inserted_code = _update_label_offsets(code_to_modify.co_code, offset, list(code_to_insert_obj)) + new_bytes, all_inserted_code = _update_label_offsets(code_to_modify.co_code, offset, list(code_to_insert_list)) new_lnotab = _modify_new_lines(code_to_modify, all_inserted_code) except ValueError: diff --git a/python/helpers/pydev/pydevd.py b/python/helpers/pydev/pydevd.py index 838e834220da..9b9d5c9ed733 100644 --- a/python/helpers/pydev/pydevd.py +++ b/python/helpers/pydev/pydevd.py @@ -750,8 +750,7 @@ class PyDB: if curr_func_name == func_name: line = next_line - if frame.f_trace is None: - frame.f_trace = self.trace_dispatch + frame.f_trace = self.trace_dispatch frame.f_lineno = line stop = True else: @@ -866,7 +865,7 @@ class PyDB: info.pydev_state = STATE_SUSPEND thread.stop_reason = CMD_THREAD_SUSPEND # return to the suspend state and wait for other command - self.do_wait_suspend(thread, frame, event, arg, "trace", send_suspend_message=False) + self.do_wait_suspend(thread, frame, event, arg, suspend_type, send_suspend_message=False) return elif info.pydev_step_cmd == CMD_STEP_RETURN: diff --git a/python/helpers/pydev/tests_pydevd_python/test_bytecode_modification.py b/python/helpers/pydev/tests_pydevd_python/test_bytecode_modification.py index 19e9eda6e03e..4773884357d8 100644 --- a/python/helpers/pydev/tests_pydevd_python/test_bytecode_modification.py +++ b/python/helpers/pydev/tests_pydevd_python/test_bytecode_modification.py @@ -4,6 +4,7 @@ import unittest from io import StringIO from _pydevd_frame_eval.pydevd_modify_bytecode import insert_code +from opcode import EXTENDED_ARG TRACE_MESSAGE = "Trace called" @@ -12,7 +13,7 @@ def tracing(): def call_tracing(): - tracing() + return tracing() def bar(a, b): @@ -44,15 +45,36 @@ class TestInsertCode(unittest.TestCase): code_orig = func_to_modify.__code__ code_to_insert = func_to_insert.__code__ success, result = insert_code(code_orig, code_to_insert, line_number) - self.compare_bytes_sequence(list(result.co_code), list(code_for_check.co_code)) + self.compare_bytes_sequence(list(result.co_code), list(code_for_check.co_code), len(code_to_insert.co_code)) - def compare_bytes_sequence(self, code1, code2): + def compare_bytes_sequence(self, code1, code2, inserted_code_size): + """ + Compare code after modification and the real code + Since we add POP_JUMP_IF_TRUE instruction, we can't compare modified code and the real code. That's why we + allow some inaccuracies while code comparison + :param code1: result code after modification + :param code2: a real code for checking + :param inserted_code_size: size of inserted code + """ seq1 = [(offset, op, arg) for offset, op, arg in dis._unpack_opargs(code1)] seq2 = [(offset, op, arg) for offset, op, arg in dis._unpack_opargs(code2)] self.assertTrue(len(seq1) == len(seq2), "Bytes sequences have different lengths") for i in range(len(seq1)): of, op1, arg1 = seq1[i] _, op2, arg2 = seq2[i] + if op1 != op2: + if op1 == 115 and op2 == 1: + # it's ok, because we added POP_JUMP_IF_TRUE manually, but it's POP_TOP in the real code + # inserted code - 2 (removed return instruction) - real code inserted + # Jump should be done to the beginning of inserted fragment + self.assertEqual(arg1, of - (inserted_code_size - 2)) + continue + elif op1 == EXTENDED_ARG and op2 == 12: + # we added a real UNARY_NOT to balance EXTENDED_ARG added by new jump instruction + # i.e. inserted code size was increased as well + inserted_code_size += 2 + continue + self.assertEqual(op1, op2, "Different operators at offset {}".format(of)) if arg1 != arg2: if op1 in (100, 101, 106, 116): @@ -488,12 +510,44 @@ class TestInsertCode(unittest.TestCase): a = a + 1 return a + def foo_check_2(): + a = 1 + b = 2 + if b > 0: + d = a + b + d += 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + not tracing() # add 'not' to balance EXTENDED_ARG when jumping + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + b = b - 1 if a > 0 else b + 1 + a = a + 1 + return a + self.check_insert_to_line_with_exec(foo, tracing, foo.__code__.co_firstlineno + 2) sys.stdout = self.original_stdout self.check_insert_to_line_by_symbols(foo, call_tracing, foo.__code__.co_firstlineno + 3, foo_check.__code__) + self.check_insert_to_line_by_symbols(foo, call_tracing, foo.__code__.co_firstlineno + 21, + foo_check_2.__code__) + finally: - sys.stdout = self.original_stdout \ No newline at end of file + sys.stdout = self.original_stdout diff --git a/python/src/com/jetbrains/python/debugger/PyCythonExtensionWarning.java b/python/src/com/jetbrains/python/debugger/PyCythonExtensionWarning.java index 168b6caa7c06..14e45c4833e9 100644 --- a/python/src/com/jetbrains/python/debugger/PyCythonExtensionWarning.java +++ b/python/src/com/jetbrains/python/debugger/PyCythonExtensionWarning.java @@ -121,7 +121,7 @@ public class PyCythonExtensionWarning { throw new ExecutionException("Python Run Configuration should be selected"); } AbstractPythonRunConfiguration runConfiguration = (AbstractPythonRunConfiguration)configuration; - final String sdkPath = runConfiguration.getSdkHome(); + final String interpreterPath = runConfiguration.getInterpreterPath(); final String helpersPath = PythonHelpersLocator.getHelpersRoot().getPath(); final String cythonExtensionsDir = PyDebugRunner.CYTHON_EXTENSIONS_DIR; @@ -129,7 +129,7 @@ public class PyCythonExtensionWarning { {"build_ext", "--build-lib", cythonExtensionsDir, "--build-temp", String.format("%s%sbuild", cythonExtensionsDir, File.separator)}; final List cmdline = new ArrayList<>(); - cmdline.add(sdkPath); + cmdline.add(interpreterPath); cmdline.add(FileUtil.join(helpersPath, FileUtil.toSystemDependentName(SETUP_CYTHON_PATH))); cmdline.addAll(Arrays.asList(cythonArgs)); LOG.info("Compile Cython Extensions " + StringUtil.join(cmdline, " ")); @@ -138,8 +138,8 @@ public class PyCythonExtensionWarning { PythonEnvUtil.addToPythonPath(environment, cythonExtensionsDir); PythonEnvUtil.setPythonUnbuffered(environment); PythonEnvUtil.setPythonDontWriteBytecode(environment); - if (sdkPath != null) { - PythonEnvUtil.resetHomePathChanges(sdkPath, environment); + if (interpreterPath != null) { + PythonEnvUtil.resetHomePathChanges(interpreterPath, environment); } GeneralCommandLine commandLine = new GeneralCommandLine(cmdline).withEnvironment(environment); diff --git a/python/testSrc/com/jetbrains/env/python/PythonDebuggerTest.java b/python/testSrc/com/jetbrains/env/python/PythonDebuggerTest.java index c23ac442fa13..ccf944edfea4 100644 --- a/python/testSrc/com/jetbrains/env/python/PythonDebuggerTest.java +++ b/python/testSrc/com/jetbrains/env/python/PythonDebuggerTest.java @@ -1295,20 +1295,23 @@ public class PythonDebuggerTest extends PyEnvTestCase { Pair pair = setNextStatement(7); waitForPause(); assertTrue(pair.first); - eval("x").hasValue("1"); + eval("x").hasValue("0"); // try to jump into a loop pair = setNextStatement(9); // do not wait for pause here, because we don't refresh suspension for incorrect jumps assertFalse(pair.first); assertTrue(pair.second.startsWith("Error:")); + stepOver(); + waitForPause(); + eval("x").hasValue("2"); resume(); waitForPause(); - eval("a").hasValue("3"); + eval("a").hasValue("2"); // jump inside a function pair = setNextStatement(2); waitForPause(); assertTrue(pair.first); - eval("a").hasValue("6"); + eval("a").hasValue("2"); resume(); } });