Skip to content

gh-122320: Add _testinternalcapi.is_dict_version_overflowed #122324

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.

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

Closed
wants to merge 10 commits into from

Conversation

Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Jul 26, 2024

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 added the needs backport to 3.13 bugs and 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
Copy link
Member

@brandtbucher brandtbucher left a comment

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
Copy link

bedevere-app bot commented Aug 1, 2024

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
Copy link
Member Author

Eclips4 commented Aug 21, 2024

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Aug 21, 2024

Thanks for making the requested changes!

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

@Eclips4 Eclips4 changed the title gh-122320: Add _testinternalcapi.is_version_overflowed gh-122320: Add _testinternalcapi.is_dict_version_overflowed Sep 18, 2024
@Eclips4
Copy link
Member Author

Eclips4 commented Dec 28, 2024

cc @brandtbucher

Copy link
Member

@vstinner vstinner left a comment

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
Copy link
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
Copy link
Member Author

Eclips4 commented Apr 29, 2025

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

@nascheme
Copy link
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants