Skip to content

gh-122320: Add _testinternalcapi.is_dict_version_overflowed - #122324

Closed
Eclips4 wants to merge 10 commits into
python:mainfrom
Eclips4:issue-122320
Closed

gh-122320: Add _testinternalcapi.is_dict_version_overflowed#122324
Eclips4 wants to merge 10 commits into
python:mainfrom
Eclips4:issue-122320

Conversation

@Eclips4

@Eclips4 Eclips4 commented Jul 26, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

The problem is that the cache->index field has a type of uint16_t:

typedef struct {
    _Py_BackoffCounter counter;
    uint16_t module_keys_version;
    uint16_t builtin_keys_version;
    uint16_t index;
} _PyLoadGlobalCache;

Therefore, due to overflow, the specialization (_Py_Specialize_LoadGlobal) will fail:

        if (keys_version != (uint16_t)keys_version) { // keys_version is bigger than 2**16
            SPECIALIZATION_FAIL(LOAD_GLOBAL, SPEC_FAIL_OUT_OF_RANGE);
            goto fail;
        }

I think changing the type of the cache->index field to uint32_t isn't worth it. The best thing we can do is reset the version.

@Eclips4
Eclips4 requested a review from brandtbucher July 26, 2024 19:42
@bedevere-app bedevere-app Bot mentioned this pull request Jul 26, 2024
Closed
@Eclips4 Eclips4 added the needs backport to 3.13 only security fixes label Jul 26, 2024
@Eclips4 Eclips4 changed the title gh-122320: Add _PyDictKeys_SetVersionForCurrentState and _testinternalcapi.reset_version gh-122320: Add _testinternalcapi.reset_version Jul 27, 2024
brandtbucher
brandtbucher requested changes Aug 1, 2024
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sort of confused what the purpose of this function is... and I'm not sure I like what it's doing.

We definitely shouldn't be resetting interp->dict_state for any reason, even an internal tests. This breaks uniqueness guarantees for all dictionary key versions, globally. So anything that does that is a non-starter.

I'm also not sure why we don't do anything if the version is nonzero... that seems to go against the function's stated purpose. I'm also not sure why dickeys->dk_version is being set to 1... again, that breaks serious global invariants.

We should probably just start skipping these tests when important versions start overflowing.

@bedevere-app

bedevere-app Bot commented Aug 1, 2024

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@Eclips4 Eclips4 changed the title gh-122320: Add _testinternalcapi.reset_version gh-122320: Add _testinternalcapi.is_version_overflowed Aug 2, 2024
@Eclips4
Eclips4 requested a review from brandtbucher August 3, 2024 18:24
@Eclips4

Eclips4 commented Aug 21, 2024

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

@bedevere-app

bedevere-app Bot commented Aug 21, 2024

Copy link
Copy Markdown

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

vstinner
vstinner reviewed Sep 16, 2024
View reviewed changes
Comment thread Modules/_testinternalcapi.c Outdated
@Eclips4 Eclips4 changed the title gh-122320: Add _testinternalcapi.is_version_overflowed gh-122320: Add _testinternalcapi.is_dict_version_overflowed Sep 18, 2024
@Eclips4

Eclips4 commented Dec 28, 2024

Copy link
Copy Markdown
Member Author

cc @brandtbucher

vstinner
vstinner approved these changes Dec 28, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Maybe the overflow check logic should be moved to a static inline function in pycore_dict.h?

@nascheme

nascheme commented Apr 25, 2025

Copy link
Copy Markdown
Member

I suggest a few changes on how this is implemented:

  • is_dict_version_overflowed is not really the correct name. You are only overflowing the version when it's used in the specialization cache. The version on the dict is 32-bits.
  • checking if the version is exactly overflowed may make the test unreliable. I'd prefer if you check if there is a buffer of 1000 or so versions left before running the test.
  • to do this, _testinternalcapi could have a function get_last_dict_version() and a MAX_SPECIALIZED_DICT_VERSION constant
  • in assert_races_do_not_crash() you don't have to check for each item. At the top of the fucntion, something like this should work:
if _testinternalcapi.MAX_SPECIALIZED_DICT_VERSION - _testinternalcapi.get_last_dict_version() < 1000:
    return unittest.skip("Out of dict versions; no rerun is possible")

@Eclips4

Eclips4 commented Apr 29, 2025

Copy link
Copy Markdown
Member Author

@nascheme, do you think we still need this PR now that #132961 has landed, or can it be closed?

@nascheme

nascheme commented Apr 30, 2025

Copy link
Copy Markdown
Member

I'd say it can be closed. Thanks for working on it though!

@Eclips4 Eclips4 closed this Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@methane methane Awaiting requested review from methane
@markshannon markshannon Awaiting requested review from markshannon
@brandtbucher brandtbucher Awaiting requested review from brandtbucher

Assignees

No one assigned

Labels

awaiting merge needs backport to 3.13 only security fixes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Eclips4 @nascheme @vstinner @brandtbucher