Frame.f_trace, paving the way to support `pdb` debugging #1918

alanjds · 2020-05-08T23:14:30Z

f_trace is initialized with PyNone. I do not know if it would be better on memory/cpu to handle Option::None instead.

No tests for now, just wanting a feedback. Specially on the RwLock used.

Is guarded by a RwLock<PyObjectRef>. Is there a better option maybe? Could use an RwLock<Option<PyObjectRef>> and provide a vm.get_none() on each None::<PyObjectRef> found. I do not know what is better on memory & cpu

coolreader18 · 2020-05-08T23:45:14Z

I think it would be a micro-optimization for PyObjectRef vs Option<PyObjectRef>, as they're the same size since Option::<Rc>::None is represented by a null pointer. vm.is_none(&obj) vs opt.is_none() is I think 1 asm instruction different, as it's basically ptr == 0 vs ptr == vm->none_address

alanjds · 2020-05-09T03:19:13Z

I see. The part intriguing me is that Option::None codepath is followed by return vm.get_none()

But storing PyNone lead to return .unwrap().clone() and I could not dodge this refcount cost on .clone()

alanjds · 2020-05-09T03:23:35Z

I may be overthinking this all. Will keep pushing "just work" code until pdb.set_trace() works and then get back to optimize.

What about the rwlock? Is it ok? The rational was that I expect it to be checked on every single line change, so should be fast. But also light on memory as will be on every frame created.

alanjds · 2020-05-09T03:24:31Z

...and thanks for the attention @coolreader18.

coolreader18 · 2020-05-09T05:38:38Z

Yeah, I think the RwLock is good, it makes sense since you might occasionally be reading the f_trace property concurrently.

Is a frame, instead of None, but still not the _right_ frame

alanjds · 2020-05-11T23:48:23Z

pdb.runcall() working, but not quite right. The 1st line stopped is into bdb yet 🤔
(pdb) args works, but also behaves as if I had (pdb) next too 🤔
Changed from RwLock to Mutex. On single-threaded is faster, on multiple it works IIUC.
Optimized a bit the codepath of vm::trace_event for the case of no trace function set, the default.

Implementation of sys.__displayhook__ from CPython 3.8.3 docs. Is the pseudocode inlined on the docs copy-pasted. See: https://docs.python.org/3/library/sys.html#sys.displayhook

This is the CPython behavior expected by bdb.py debugger. Had not understood how does it work on CPython. Here has implemented via frame.__delattr__ specially for f_trace. Other `del frame.*` are actually noops for now

alanjds · 2020-05-16T21:39:15Z

And this is the end of PyCon US 2020 sprint:

(pdb) continue works as expected, not breaking at some del frame.f_trace

Thanks @youknowone for the help on Gitter.
Had not found how do CPython implement del frame.f_trace. I implemented via frame.__delattr__. Yet I do not know how to call super() from Rust side. Will need help on this.

Now may need support for a setter on frame.lineno if wanting to have (pdb) jump to work. Maybe for (pdb) next too, as seems broken.

Good night.

youknowone · 2020-05-17T15:28:51Z

vm/src/obj/objframe.rs

@@ -24,6 +24,15 @@ impl FrameRef {
        "<frame object at .. >".to_owned()
    }

+    #[pymethod(name = "__delattr__")]
+    fn delattr(self, value: PyObjectRef, vm: &VirtualMachine) {
+        println!("Called: FrameRef::f_trace over {:?}", value);


nice workaround for now. putting an incompatibility comment will be helpful for future work.

youknowone · 2020-07-26T07:14:44Z

Do you want to work more for this or want to merge part of the work?

alanjds · 2020-07-26T22:03:54Z

Both 😬.

Better to merge the actual progress for this to not get rotten when I find the time to finish it.

youknowone · 2020-07-26T22:17:55Z

Will you make a new PR with finished commits after rebase?

alanjds · 2020-07-27T15:06:17Z

Will you make a new PR with finished commits after rebase?

I am not used to rebase stuff, but can learn to.

Should I go cherry-picking over master and send a new PR? Is this the way?

youknowone · 2020-07-27T15:07:53Z

Oh, yep. Because this one is very aged, cherry-pick could be easier than rebase. That sounds very reasonable.

Also got its pure-python dependencies bdb.py and cmd.py Also stubbed the C-based gc.py using `pystubber` (https://pypi.org/project/pystubber/)

coolreader18 · 2020-08-11T21:22:05Z

Oh, right -- looks like the errors are from .lock().unwrap(); we switched to parking_lot Mutexs since this PR was opened, so the .unwrap()s shouldn't be necessary.

coolreader18 · 2020-08-11T21:43:55Z

Is the gc.py module necessary if everything raises a NotImplementedError anyway? We've mostly just been commenting out gc imports and function calls in modules that have them, is there a reason that can't work for pdb?

alanjds · 2020-08-11T21:53:45Z

Is the gc.py module necessary if everything raises a NotImplementedError anyway? We've mostly just been commenting out gc imports and function calls in modules that have them, is there a reason that can't work for pdb?

bdb.py imports it to use somewhere, but I had not hit the case when it calls gc for real.

Anyway I do not feel bad by stubbing C-based stuff that we want not to implement right now.

(I need to say that sometimes I am missing the ability of calling rust code directly from python code. Maybe on future a custom importhook can do the trick.)

coolreader18 · 2020-08-11T22:16:07Z

@youknowone what do you think?

alanjds · 2020-08-11T22:30:08Z

We've mostly just been commenting out gc imports and function calls in modules that have them

My approach on this is to try to do the lowest number of changes possible on foreign code, if we want to get back to the original somewhere in future. That is why I am trying to provide a bogus gc instead of messing with bdb.py original code.

Less changes to track upstream on bdb.py and keep patching here until a fully compatible gc is ready.

alanjds · 2020-08-11T22:34:26Z

https://github.com/RustPython/RustPython/pull/1918/checks?check_run_id=973313037#step:10:7685

AttributeError: module 'sys' has no attribute 'stdout'

What? I though that this was been fixed before 🤔. I saw a PR about this!

This reverts commit c0b11de.

alanjds · 2020-08-12T00:45:36Z

Looks like sys.displayhook was natively implemented in meantime. Removing my hack from site.py and waiting for CI completion.

For some reason my local tests passed without changes. I may have a local environment messed up.

alanjds · 2020-08-12T01:34:09Z

Yay! Got green.

Now pdb.set_trace() "works" but stops on the wrong place.
Then works:

a(rgs)
c(ontinue)
h(elp)
l(ist)
w(here)
s(tep) into
local evaluation also works

Yet broken:

n(ext): Is ignored, showing the very same line forever
r(eturn): Is ignored, showing the very same line forever
j(ump): Is ignored, and a follow c(ontinue) raises AttributeError: attribute 'f_lineno' of 'frame' objects is not writable

This PR is good to go. At least locals() and globals() variables inspection are working. Closure variables inspection seemed broken.

Lets get this merged and open another to fix j(ump) and n(ext)

coolreader18 · 2020-08-12T18:54:50Z

I'm merging this manually to fix this weird lf->crlf change in frame.rs

coolreader18 · 2020-08-12T19:05:52Z

Alright, this is merged to master even though github doesn't show it.

alanjds added 4 commits May 5, 2020 21:45

Fix: f.f_lasti is 0-indexed

eb141ac

Frame.f_trace exists but always returns None

63da975

Frame::new() now needs vm

83921fe

Frame.f_trace inits with PyNone and can be changed later

4eac7d3

Is guarded by a RwLock<PyObjectRef>. Is there a better option maybe? Could use an RwLock<Option<PyObjectRef>> and provide a vm.get_none() on each None::<PyObjectRef> found. I do not know what is better on memory & cpu

alanjds added 3 commits May 11, 2020 11:49

No need to spend extra time on trace_event without a trace function

6bef4b2

Not expecting multiple simultaneous reads for a given frame

b67d570

sys.settrace()-ed func actually receive a frame on 1st arg

a158a9d

Is a frame, instead of None, but still not the _right_ frame

alanjds added 2 commits May 16, 2020 14:35

Enable sys.displayhook and sys.__displayhook__

c0b11de

Implementation of sys.__displayhook__ from CPython 3.8.3 docs. Is the pseudocode inlined on the docs copy-pasted. See: https://docs.python.org/3/library/sys.html#sys.displayhook

Working del frame.f_trace as `frame.f_trace = None

b4189ab

This is the CPython behavior expected by bdb.py debugger. Had not understood how does it work on CPython. Here has implemented via frame.__delattr__ specially for f_trace. Other `del frame.*` are actually noops for now

youknowone reviewed May 17, 2020

View reviewed changes

alanjds marked this pull request as ready for review July 26, 2020 22:04

alanjds added 3 commits August 10, 2020 19:38

Faster setattr

3f74daf

Added pdb.py from Python 3.5

445078a

Also got its pure-python dependencies bdb.py and cmd.py Also stubbed the C-based gc.py using `pystubber` (https://pypi.org/project/pystubber/)

Merge branch 'master' into support-pdb

0f8db25

alanjds added 2 commits August 11, 2020 18:29

Comment on incompatibility on Frame.__delattr__

688a3c8

No .unwrap() is needed on PyRef !!

f5d119b

gc.collect() is dumb to always return 0 collected objects

f5bb995

Disable gc on RustPython tests

c542022

Revert python-based sys.displayhook implementation

8b58390

This reverts commit c0b11de.

coolreader18 approved these changes Aug 12, 2020

View reviewed changes

coolreader18 closed this Aug 12, 2020

Frame.f_trace, paving the way to support pdb debugging #1918

Frame.f_trace, paving the way to support pdb debugging #1918

Uh oh!

Conversation

alanjds commented May 8, 2020

Uh oh!

coolreader18 commented May 8, 2020

Uh oh!

alanjds commented May 9, 2020

Uh oh!

alanjds commented May 9, 2020

Uh oh!

alanjds commented May 9, 2020

Uh oh!

coolreader18 commented May 9, 2020

Uh oh!

alanjds commented May 11, 2020

Uh oh!

alanjds commented May 16, 2020

Uh oh!

youknowone May 17, 2020

Choose a reason for hiding this comment

Uh oh!

youknowone commented Jul 26, 2020

Uh oh!

alanjds commented Jul 26, 2020

Uh oh!

youknowone commented Jul 26, 2020

Uh oh!

alanjds commented Jul 27, 2020

Uh oh!

youknowone commented Jul 27, 2020

Uh oh!

coolreader18 commented Aug 11, 2020

Uh oh!

coolreader18 commented Aug 11, 2020

Uh oh!

alanjds commented Aug 11, 2020

Uh oh!

coolreader18 commented Aug 11, 2020

Uh oh!

alanjds commented Aug 11, 2020

Uh oh!

alanjds commented Aug 11, 2020

Uh oh!

alanjds commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanjds commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coolreader18 commented Aug 12, 2020

Uh oh!

coolreader18 commented Aug 12, 2020

Uh oh!

Uh oh!

Frame.f_trace, paving the way to support `pdb` debugging #1918

Frame.f_trace, paving the way to support `pdb` debugging #1918

alanjds commented Aug 12, 2020 •

edited

Loading

alanjds commented Aug 12, 2020 •

edited

Loading