Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On current main (after applying PEP 669) the tracing behavior changed and can notify about the same line more than once. #103471

Closed
fabioz opened this issue Apr 12, 2023 · 7 comments · Fixed by #104387
Assignees
Labels
type-bug An unexpected behavior, bug, or error

Comments

@fabioz
Copy link
Contributor

fabioz commented Apr 12, 2023

The test case for this is below (it works for Python 3.11 and fails in the current master).

import sys
import unittest


class TestSetLocalTrace(unittest.TestCase):

    def test_with_branches(self):

        def tracefunc(frame, event, arg):
            if frame.f_code.co_name == "func":
                frame.f_trace = None
                frame.f_trace = tracefunc
                line = frame.f_lineno - frame.f_code.co_firstlineno
                events.append((line, event))
            return tracefunc

        def func(arg=1):
            N = 1
            if arg >= 2:
                not_reached = 3
            else:
                reached = 5
            if arg >= 3:
                not_reached = 7
            else:
                reached = 9
            the_end = 10

        EXPECTED_EVENTS = [
            (0, 'call'),
            (1, 'line'),
            (2, 'line'),
            (5, 'line'),
            (6, 'line'),
            (9, 'line'),
            (10, 'line'),
            (10, 'return'),
        ]

        events = []
        sys.settrace(tracefunc)
        sys._getframe().f_trace = tracefunc
        func()
        self.assertEqual(events, EXPECTED_EVENTS)
        sys.settrace(None)
@ronaldoussoren
Copy link
Contributor

@markshannon

@AlexWaygood AlexWaygood changed the title In the current master (after applying PEP 669) the tracing behavior changed and can notify about the same line more than once. On current main (after applying PEP 669) the tracing behavior changed and can notify about the same line more than once. Apr 12, 2023
@gaogaotiantian
Copy link
Member

I don't think the tracing mechanism pre-669 keeps the last line being traced. The only test failed without remembering the last traced line is test_pdb_issue_20766 - and having "last traced line" is the wrong solution for that test.

For that test, the code is

while i <= 2:
    sess = pdb.Pdb()
    sess.set_trace(sys._getframe())
    print('pdb %d: %s' % (i, sess._previous_sigint_handler))
    i += 1

What happened without remembering the last traced line (pre-669 style) is:

  1. pdb is brought up, stopped at print line, remember it
  2. The user inputs continue, which turns off the debugger (sys.settrace(None))
  3. The full loop executes, go back to setting up the debugger.
  4. pdb is brought up again, then starts debugging, but wait - the print line is already traced! So let's skip that line and go to `i += 1

This is obviously wrong - after tracing the print line for the first time, the whole loop executed, remembering the last traced line is incorrect.

But "forgetting the last trace line" is not correct either, because we are considering jump and branch as the LINE event trigger for the target line.

For code like:

if a > 0:
    return 1
else:
    return 2

After the instrumentation, there will be an instrumented jump, to an instrumented line, which would trigger TWO LINE events. However, the debugger was brought up between these two events. If we turn settrace off and on, and it clears last trace line, both events will shoot(and that's what happened in the given example).

I think the pre-669 solution is to check the last executed instruction in the frame and see if that instruction is in the same line(or maybe for jump, the target of the instuction) to avoid duplicated events, not remembering the last traced line.

So, the current solution is not equivalent to what we had before, and we should probably reconsider if that's the way to go. It seems like checking the last executed line is better than last traced line.

@markshannon
Copy link
Member

The original behavior was to see if the offset of previous instruction and the offset of the about-to-be-executed instruction were one of:

  • next < prev: Backwards jump, always emit line event
  • line(next) != line(prev) and line(next) != -1: emit line event

We can mimic this by setting a flag in branch and jump events so that the next INSTRUMENTED_LINE doesn't duplicate the emitted line (or emit one at all if jumping from line N to line N over code with line M.

Unfortunately, it seems that we don't emit BRANCH events when exiting FOR_ITER, so I'll need to fix that first.

@gaogaotiantian
Copy link
Member

Can we ask BRANCH and JUMP events not to emit in settrace mode when the target is an INSTRUMENTED_LINE? That seems a cleaner solution and does not require any states. Basically have a different callback for BRANCH and JUMP in settrace mode than LINE event and somehow checks the target there(if it's possible?).

@fabioz
Copy link
Contributor Author

fabioz commented May 9, 2023

@markshannon any news on this one? I'd really like to be able to use debugpy/pydevd with CPython 3.12 earlier rather than later...

@markshannon
Copy link
Member

We need to make sure that we are emitting line events in the right places first (#104239).
Once that is done, it should be relatively straightforward to mimic the prior behavior.

Should probably be fixed late this week or early next.

OOI, are you planning on porting debugpy/pydevd to PEP 669?

@fabioz
Copy link
Contributor Author

fabioz commented May 9, 2023

OOI, are you planning on porting debugpy/pydevd to PEP 669?

I do have plans to do it, but still no timeframe ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants