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.

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

LOAD_ATTR cache does not fully replicate PyObject_GetAttr behavior #86432

Closed
KevinModzelewski mannequin opened this issue Nov 4, 2020 · 7 comments
Closed

LOAD_ATTR cache does not fully replicate PyObject_GetAttr behavior #86432

KevinModzelewski mannequin opened this issue Nov 4, 2020 · 7 comments
Labels
3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@KevinModzelewski
Copy link
Mannequin

KevinModzelewski mannequin commented Nov 4, 2020

BPO 42266
Nosy @pmp-p, @1st1, @pablogsal, @Jongy
PRs
  • bpo-42266: Handle monkey-patching descriptors in LOAD_ATTR cache #23157
    		•
    Files
  • test.py: Test case with unexpected opcache behavior
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2020-11-05.09:23:52.835>
created_at = <Date 2020-11-04.21:56:10.288>
labels = ['interpreter-core', 'type-bug', '3.10']
title = 'LOAD_ATTR cache does not fully replicate PyObject_GetAttr behavior'
updated_at = <Date 2020-11-05.09:23:52.835>
user = 'https://bugs.python.org/KevinModzelewski'

    bugs.python.org fields:

    activity = <Date 2020-11-05.09:23:52.835>
actor = 'pablogsal'
assignee = 'none'
closed = True
closed_date = <Date 2020-11-05.09:23:52.835>
closer = 'pablogsal'
components = ['Interpreter Core']
creation = <Date 2020-11-04.21:56:10.288>
creator = 'Kevin Modzelewski'
dependencies = []
files = ['49568']
hgrepos = []
issue_num = 42266
keywords = ['patch']
message_count = 7.0
messages = ['380370', '380374', '380375', '380376', '380378', '380379', '380399']
nosy_count = 5.0
nosy_names = ['pmpp', 'yselivanov', 'Kevin Modzelewski', 'pablogsal', 'Yonatan Goldschmidt']
pr_nums = ['23157']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue42266'
versions = ['Python 3.10']
    @KevinModzelewski
    Copy link
    Mannequin Author

    KevinModzelewski mannequin commented Nov 4, 2020

    The problem is that the descriptor-ness of a type-level attribute is only checked at opcache-set time, not at opcache-hit time.

    $ python3.8 test.py
2

$ ./python --version
Python 3.10.0a2+
$ git rev-parse --short HEAD
789359f47c
$ ./python test.py
1

    @KevinModzelewski KevinModzelewski mannequin added 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Nov 4, 2020
    @pablogsal
    Copy link
    Member

    Good catch, Kevin! Would you like to submit a PR for fixing this?

    @pablogsal
    Copy link
    Member

    Given that having attributes that are classes is quite uncommon, I think we can not optimize of the attribute itself is a class instead of checking for descriptors on every hit, hurting the performance gains

    @pablogsal
    Copy link
    Member

    Yury, any preference here?

    @pablogsal
    Copy link
    Member

    We could also store the tag of the type object if is a descriptor and compare against that on the cache hit to check that our assumptions are valid. The price here would be an extra pointer on the cache per opcode that may not even be used most of the time.

    @pablogsal
    Copy link
    Member

    s/the attribute itself is a class/the attribute itself is in the class/

    @pablogsal
    Copy link
    Member

    New changeset 80449f2 by Pablo Galindo in branch 'master':
    bpo-42266: Handle monkey-patching descriptors in LOAD_ATTR cache (GH-23157)
    80449f2

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.10 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
    1 participant
    @pablogsal