8000 `f_lasti` points to `CACHE` opcode during inlined `CALL` · Issue #96970 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

f_lasti points to CACHE opcode during inlined CALL #96970

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.

Sign up for GitHub

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

Jump to bottom
Closed
15r10nk opened this issue Sep 20, 2022 · 12 comments
Closed

f_lasti points to CACHE opcode during inlined CALL #96970

15r10nk opened this issue Sep 20, 2022 · 12 comments
Assignees
@brandtbucher
Labels
3.11 only security fixes 3.12 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@15r10nk
Copy link
Contributor
15r10nk commented Sep 20, 2022 •
edited
Loading

The instruction pointer f_lasti points under some conditions to a CACHE opcode.
This is not completely wrong, because this cache opcode comes after the expected CALL opcode, but the documetation says that CACHE opcodes should be skipped. Which it is not in this case.

script:

import inspect
import dis

frame = inspect.currentframe()


class Tester:
    def __call__(self, f):
        deco(f)


tester = Tester()


def deco(f):
    assert f.__name__ == "foo"
    instructions = list(dis.get_instructions(frame.f_code, show_caches=True))
    opname = instructions[frame.f_lasti // 2].opname
    print(f"{frame.f_lasti = }\n{opname = }")
    assert opname == "CALL", opname


print("correct")


@tester
def foo():
    pass


print()
print("incorrect")


@deco
def foo():
    pass

output (Python 3.11.0rc2+):

correct
frame.f_lasti = 132
opname = 'CALL'

incorrect
frame.f_lasti = 204
opname = 'CACHE'
Traceback (most recent call last):
  File "/home/frank/projects/executing/example.py", line 35, in <module>
    @deco
     ^^^^
  File "/home/frank/projects/executing/example.py", line 20, in deco
    assert opname == "CALL", opname
AssertionError: CACHE
@15r10nk 15r10nk added the type-bug An unexpected behavior, bug, or error label Sep 20, 2022
@15r10nk 15r10nk mentioned this issue Sep 20, 2022
Merged
@brandtbucher
Copy link
Member

The issue is that when "inlining" a pure-Python call like deco, we must save the frame's instruction pointer to point to the next instruction, which means that f_lasti will be the code unit prior to that: the CALL's last CACHE entry. No other opcode (including non-inlined calls) has this requirement, so f_lasti should always(?) point to a "real" instruction within their bodies.

Personally, I think the docs you linked to cover this:

Logically, this space is part of the preceding instruction.

So, in my opinion, the correct thing to do here is for tools to scan backwards over any caches when trying to find the last "real" instruction.

I can see why we might want to make this more consistent, though. If others agree, we can look into ways of making that happen, but I'm worried that they'll either come with a performance cost for many types of calls, or be too invasive to justify in a patch release.

@brandtbucher
Copy link
Member

I see you updated your comment. I read the original wording as "this is completely wrong".

@sweeneyde
Copy link
Member

Not for 3.11, but maybe we could change the INSTRUCTION_START() macro in ceval.c to something like:

- #define INSTRUCTION_START(op) (frame->prev_instr = next_instr++)
+ #define INSTRUCTION_START(op) do {
+     frame->prev_instr = next_instr;
+     next_instr += (1 + INLINE_CACHE_ENTRIES_ ## op); // fixme for adaptive instructions
+ while (0)

Then we could remove most JUMPBY(INLINE_CACHE_ENTRIES_...) in ceval.c and reconfigure the compiler to take this into account.

I'm not sure whether the resulting code would look better or worse, but I believe it would make f_lasti consistently meaningful.

@15r10nk
Copy link
Contributor Author
15r10nk commented Sep 20, 2022

We can work with the current implementation. Our work around already is that we scan backwards to the last instruction.

and will instruct the interpreter to skip over them at runtime.

This confused me and I got the impression that this might be a bug.
Maybe the docs could say what should be done if f_lasti points to a CACHE.

@sweeneyde
Copy link
Member

Actually, I think I misunderstood, what I wrote would make prev_instr always point to the next instruction.

@brandtbucher brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.11 only security fixes 3.12 only security fixes labels Sep 20, 2022
@brandtbucher brandtbucher changed the title f_lasti points to CACHE opcode (python 3.11.0rc2) f_lasti points to CACHE opcode during inlined CALL Sep 20, 2022
@brandtbucher
Copy link
Member
brandtbucher commented Sep 21, 2022 •
edited
Loading

@carljm had an interesting idea: leave prev_instr alone when inlining calls, and jump over the caches when resuming with something like:

next_instr += _PyOpcode_Caches[_PyOpcode_Deopt[_Py_OPCODE(next_instr[-1])]];

This should "fix" the issue, but we should definitely measure its performance impact, since it adds quite a bit of logic to every return from an inlined call. I'll experiment with this approach over the next couple of days.

@brandtbucher brandtbucher self-assigned this Oct 13, 2022
@iritkatriel
Copy link
Member

The situation is probably very different following #109095.
Could you check whether this issue is still relevant?

@brandtbucher
Copy link
Member

Indeed, a slightly tweaked example (since dis doesn't yield CACHE instructions anymore) correctly shows the CALL instruction for both examples.

It does still reproduce on 3.12, but I'm not sure that's really worth changing at this point?

@picnixz
Copy link
Member
picnixz commented Feb 15, 2025

On 3.14, we have different outputs and instructions[frame.f_lasti // 2] now raises an IndexError. I don't know when it has been changed, but is this issue still relevant?

@15r10nk
Copy link
Contributor Author
15r10nk commented Feb 15, 2025

The current behavior is ok for me. My initial assumption was that it could be a bug based on what I read in the docs.

@picnixz
Copy link
Member
picnixz commented Feb 15, 2025

Is it ok to close the issue then?

@15r10nk
Copy link
Contributor Author
15r10nk commented Feb 15, 2025

Yes it is ok for me if you close this issue

@picnixz picnixz closed this as completed Feb 15, 2025
@picnixz picnixz closed this as not planned Won't fix, can't repro, duplicate, stale Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brandtbucher brandtbucher

Labels
3.11 only security fixes 3.12 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@iritkatriel @picnixz @sweeneyde @brandtbucher @15r10nk
0