[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Change by Terry J. Reedy : -- versions: +Python 3.10 -Python 2.7, Python 3.5, Python 3.6, Python 3.7 ___ Python tracker ___ ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Change by Ionel Cristian Mărieș : -- nosy: +ionelmc ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Change by Nick Coghlan : -- pull_requests: +17447 stage: -> patch review pull_request: https://github.com/python/cpython/pull/18052 ___ Python tracker ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: Starting to make some progress on an implementation, and it occurs to me that if this approach does work out, it should make Python level trace functions *much* faster. Right now, the call to the Python function in call_trampoline is

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: When I rejected that approach previously, it hadn't occurred to me yet that the write-through proxy could write through to both the frame state *and* the regular dynamic snapshot returned by locals(). The current design came from asking

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: No, as locals() never returns the write-through proxy in the latest version of the PEP - it only ever returns the dynamic snapshot namespace (retrieving it from the proxy if necessary). To get access to the write-through proxy in Python

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Xavier de Gaye added the comment: Nick, in msg304388 you wrote "[allow] immediate write-through from trace functions". The latest iteration of PEP 558 says instead: "tracing mode: the way the interpreter behaves when a trace hook has been registered..." and also says:

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: I rewrote the PEP based on the latest iteration of the design concept: https://github.com/python/peps/commit/9a8e590a523bdeed0598084a0782fb07fc15dfc0 (That initial commit has some minor errors in it that I've already fixed, so check the

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: Just confirming that the locals() vs frame.f_locals distinction also exists in the C API: the former is PyEval_GetLocals() (which implicitly ensures the snapshot is up to date), while the equivalent of the latter is direct access to

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Change by Guido van Rossum : -- nosy: -gvanrossum ___ Python tracker ___ ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Guido van Rossum added the comment: Sounds like a plan, I will ignore the ticket then. -- ___ Python tracker ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: Err, I phrased that last sentence really badly. I meant that if Guido *hasn't* been following this ticket closely, he's more likely to ask for clarification if PEP 558 proposes a semantic change based on the discussion here without

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: Guido: any proposed semantic changes for CPython will eventually make their way through PEP 558 (and potentially a competing PEP if someone else is sufficiently keen on the idea of making non-tracing mode behave the same way as tracing

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Guido van Rossum added the comment: > Moreover, if f_locals can be modified outside a tracing hook, then we have > the same problem in a cross-function way, e.g. if function f1() calls > function f2() which does sys._getframe(1).f_locals['foo'] = 42. That's covered by the

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Guido van Rossum added the comment: @Armin: > Guido: you must be tired and forgot that locals() is a regular function :-) > The compiler cannot recognize it reliably. I know, but we occasionally do make exceptions for builtins. In particular super() is recognized by the

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: The current pdb misbehaviour when navigating the stack is indeed a valid argument in favour of allowing immediate write-through from trace functions for regular local variables as well - I don't recall reading that bug report when Xavier

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: @Nick: > We're OK with the idea that installing a trace function might automatically > turn off various compiler and interpreter managed optimisations We're OK with assigning to locals() doing that too. > What we're aiming to avoid is

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: Yep, my current goal is to see if I can come up with a surgical fix that solves the established problem with the bad interaction between cells and trace functions without any unintended consequences for either CPython or other interpreters.

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Armin Rigo added the comment: Guido: you must be tired and forgot that locals() is a regular function :-) The compiler cannot recognize it reliably. Moreover, if f_locals can be modified outside a tracing hook, then we have the same problem in a cross-function

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Change by STINNER Victor : -- nosy: -haypo ___ Python tracker ___ ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Guido van Rossum added the comment: Hm I may just be completely off here, but I thought that compilers could be allowed to recognize the use of locals() in a particular function and then disable JIT optimizations for that function. (In fact I thought we already had a

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: I'll also note there's a simpler reason the namespace object exposed at the function level can't just be a write-through proxy for the underlying frame: references to frame.f_locals may outlive the frame backing it, at which point we really

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Armin Rigo added the comment: Thanks Nick for the clarification. Yes, that's what I meant: supporting such code in simple JITs is a nightmare. Perhaps more importantly, I am sure that if Python starts supporting random mutation of locals outside tracing hooks,

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: We're OK with the idea that installing a trace function might automatically turn off various compiler and interpreter managed optimisations (it's similar to how requesting or implying reliance on full frame support in other implementations

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: @arigo: But CPython is already committed to supporting writes to locals() at any moment, because at any moment you can set a trace function and in every proposal trace functions can reliably write to locals. So I don't think this is a

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Armin Rigo added the comment: FWIW, a Psyco-level JIT compiler can support reads from locals() or f_locals, but writes are harder. The need to support writes would likely become another hard step on the way towards adding some simple JIT support to CPython in the

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: I guess I should say that I'm still confused about why we're coming up with such elaborate schemes here, instead of declaring that f_locals and locals() shall return a dict proxy so that from the user's point of view, they Always Just Work

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: Aye, what's in PEP 558 is the least invasive implementation change I've been able to come up that fixes the reported bug, but Nathaniel's right that it would break debuggers (and probably some other things) as currently written. So the

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Guido van Rossum added the comment: Wow, nothing here is simple. :-( Even though the examples show that there's obviously a bug, I would caution against fixing it rashly without understanding how the current behavior may be useful in other circumstances. I've lost what I

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Change by Guido van Rossum : -- nosy: +gvanrossum ___ Python tracker ___ ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: I've been thinking further about the write-through proxy idea, and I think I've come up with a design for one that shouldn't be too hard to implement, while also avoiding all of the problems that we want to avoid. The core of the idea is

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: For write-backs: no, since the interpreter will still write those values back into the destination cell For locals display: no, since nothing changes for the handling of fast locals For closure display: yes as, by default, debuggers will now print the closure

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: Doesn't this proposal break every debugger, including pdb? -- ___ Python tracker ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Changes by Nick Coghlan : -- dependencies: +Clarify the required behaviour of locals() ___ Python tracker ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: https://github.com/python/cpython/pull/3640/files includes a more automated-tested-friendly version of Armin's deterministic reproducer (the generator loop uses range() to generator a separate loop counter, and then the test checks that the incremented closure

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Changes by Nick Coghlan : -- keywords: +patch pull_requests: +3634 stage: -> patch review ___ Python tracker ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: The same way the dis module does: by looking at the names listed in the various code object attributes. If it's listed in co_cellvars, then it's a local variable in the current frame that's in a cell because it's part of the closure for a nested function. If

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: How does a trace function or debugger tell the difference between a closure cell and a fast local whose value happens to be a cell object? -- ___ Python tracker

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: After drafting PEP 558, briefly chatting about it to Guido, and sleeping on the topic, I'm wondering if there might be answer that's simpler than any of the alternatives consider so far: what if PyFrame_FastToLocals added the *cell objects* to f_locals for any

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: Err, s/officially part of the status quo/officially part of the language specification/ :) -- ___ Python tracker ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: As per the discussion in issue #17960, I'm going to put together a short PEP about this topic for Python 3.7 that will better specify the expected behaviour of locals() and frame.f_locals. That will be a combination of making the status quo official, proposing

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Xavier de Gaye added the comment: In msg296758 Nathaniel wrote: > It turns out if you simply delete the LocalsToFast and FastToLocals calls in > call_trampoline, then the test suite still passes. I'm pretty sure that pdb > relies on this as a way to set local variables, though, so I think this

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: The problem I see with proxy objects for functions/coroutines/generators is that it *doesn't* match how locals() currently behaves in the absence of a tracing function - that gives you a "single shared snapshot" behaviour, where writes to the result of locals()

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: > I like it because it categorically eliminates the "tracing or not?" global > state dependence when it comes to manipulation of the return value of > locals() - manipulating that will either always affect the original execution > namespace immediately

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: I like it because it categorically eliminates the "tracing or not?" global state dependence when it comes to manipulation of the return value of locals() - manipulating that will either always affect the original execution namespace immediately (modules,

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: > Folks that actually *wanted* the old behaviour would then need to do either > "sys._getframe().f_locals" or "inspect.currentframe().f_locals". So by making locals() and f_locals have different semantics, we'd be adding yet another user-visible

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Changes by STINNER Victor : -- nosy: +haypo ___ Python tracker ___ ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: I updated the old "we should clarify the semantics" issue with a more concrete update proposal: https://bugs.python.org/issue17960#msg296880 Essentially nothing would change for module and class scopes, but the proposal for function scopes is that locals() be

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: Sorry, I wasn't clear: I don't see any problem for the cases that don't optimize local variable access, and don't think any of those should change. Instead, I think we should tighten up the formal specification of locals() to better match how it is actually

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: Interesting idea! I'm not sure I fully understand how it would work though. What would you do for the frames that don't use the fast array, and where locals() currently returns the "real" namespace? How are you imagining that the trace function writeback

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: To make the behaviour more consistent in 3.7, I'd be more inclined to go in the other direction: make locals() return a truly independent snapshot when used in a function, rather than sharing a single snapshot between all locals() calls. Shared snapshots that

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: It isn't obvious to me whether the write-through proxy idea is a good one on net, but here's the rationale for why it might be. Currently, the user-visible semantics of locals() and f_locals are a bit complicated. AFAIK they aren't documented anywhere

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nick Coghlan added the comment: The writeback-only-if-changed approach sounds like a plausible improvement to me. I'd be hesitant to include such a change in 3.5.4 though, since we don't get a second go at that if something breaks unexpectedly. However, I don't think returning a write-through

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Changes by Xavier G. Domingo : -- nosy: +xgdomingo ___ Python tracker ___ ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Nathaniel Smith added the comment: Some thoughts based on discussion with Armin in #pypy: It turns out if you simply delete the LocalsToFast and FastToLocals calls in call_trampoline, then the test suite still passes. I'm pretty sure that pdb relies on this as a way to set local variables,

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Armin Rigo added the comment: (Note: x.py is for Python 2.7; for 3.x, of course, replace ``.next()`` with ``.__next__()``. The result is the same) -- ___ Python tracker

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Armin Rigo added the comment: A version of the same problem without threads, using generators instead to get the bug deterministically. Prints 1, 1, 1, 1 on CPython and 1, 2, 3, 3 on PyPy; in both cases we would rather expect 1, 2, 3, 4. -- Added file:

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Changes by Armin Rigo : -- nosy: +arigo ___ Python tracker ___ ___

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

Changes by Nick Coghlan : -- nosy: +ncoghlan ___ Python tracker ___ ___ Python-bugs-list

[issue30744] Local variable assignment is broken when combined with threads + tracing + closures

New submission from Nathaniel Smith: The attached script looks innocent, but gives wildly incorrect results on all versions of CPython I've tested. It does two things: - spawns a thread which just loops, doing nothing - in the main thread, repeatedly increments a variable 'x' And most of