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.

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

Support Py_tss_NEEDS_INIT outside of static initialisation #76009

Open
scoder opened this issue Oct 20, 2017 · 4 comments
Open

Support Py_tss_NEEDS_INIT outside of static initialisation #76009

scoder opened this issue Oct 20, 2017 · 4 comments
Labels
3.7 (EOL) end of life extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@scoder
Copy link
Contributor

scoder commented Oct 20, 2017

BPO 31828
Nosy @ncoghlan, @scoder, @embray, @ma8ma
PRs
  • bpo-31828: make Py_tss_NEEDS_INIT usable in non-static initialisers #4060
  • No issue #: Fix wording about Py_tss_NEEDS_INIT in docs #4096
  • 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 = None
    created_at = <Date 2017-10-20.16:07:56.415>
    labels = ['extension-modules', 'interpreter-core', 'type-feature', '3.7']
    title = 'Support Py_tss_NEEDS_INIT outside of static initialisation'
    updated_at = <Date 2017-10-26.15:52:49.298>
    user = 'https://github.com/scoder'

    bugs.python.org fields:

    activity = <Date 2017-10-26.15:52:49.298>
    actor = 'erik.bray'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Extension Modules', 'Interpreter Core']
    creation = <Date 2017-10-20.16:07:56.415>
    creator = 'scoder'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31828
    keywords = ['patch']
    message_count = 4.0
    messages = ['304661', '304697', '304757', '304778']
    nosy_count = 4.0
    nosy_names = ['ncoghlan', 'scoder', 'erik.bray', 'masamoto']
    pr_nums = ['4060', '4096']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31828'
    versions = ['Python 3.7']

    @scoder
    Copy link
    Contributor Author

    scoder commented Oct 20, 2017

    Following up on bpo-25658, it was found that the current definition of Py_tss_NEEDS_INIT restricts its use to initialisers in C and cannot be used for arbitrary assignments. It is currently declared as follows:

        #define Py_tss_NEEDS_INIT   {0}

    which results in a C compiler error for assignments like "x = Py_tss_NEEDS_INIT".

    I proposed to change this to

        #define Py_tss_NEEDS_INIT   ((Py_tss_t) {0})

    in compliance with GCC and C99, but that fails to compile in MSVC and probably other old C++-ish compilers.

    I'm not sure how to improve this declaration, but given that it's a public header file, restricting its applicability seems really unfortunate.

    @scoder scoder added 3.7 (EOL) end of life extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Oct 20, 2017
    @ncoghlan
    Copy link
    Contributor

    Right, the cases we were aiming to cover were:

    • C variable declarations ("static Py_tss_t tss_key = Py_tss_NEEDS_INIT;")
    • dynamic allocation with PyThread_tss_alloc
    • resetting a key back to the uninitialised state with PyThread_tss_delete

    The problem we have is that the second field in Py_tss_t is platform dependent, and not all platforms define a safe "unused" value for their NATIVE_TSS_KEY_T, which means Py_tss_NEEDS_INIT ends up being only a partial initialiser (earlier versions of the PEP used a field initialiser, but we were asked to switch it to a partial initialiser in order to support more compilers).

    We could offer a PyThread_tss_reset (to reset a key back to Py_tss_NEEDS_INIT), but that's confusingly similar to PyThread_tss_delete.

    Another option would be to check for typed partial initialiser support in the configure script, and declare Py_tss_NEEDS_INIT accordingly. However, that wouldn't solve the problem for any clients that are themselves also attempting to write cross-platform code.

    @ma8ma
    Copy link
    Mannequin

    ma8ma mannequin commented Oct 22, 2017

    Or Py_tss_NEEDS_INIT is removed from the C API document to become a private feature; in other words, the Python interpreter will recommend that the static allocation for the TSS key shouldn't be used in the API client codes except for the interpreter core and built-in modules. (okay, I know it's an unfair suggestion)

    BTW, it's my mistake which the word "default value" for Py_tss_NEEDS_INIT, actually it makes more sense "initializer". I'd like to fix the document and the PEP, would I open the PR with this issue?

    @ncoghlan
    Copy link
    Contributor

    For the wording fix, since it's just a trivial docs update, we can go ahead and fix it without an issue reference from the PRs.

    As far as the API goes, I do want to support "static Py_tss_t my_tss_key = Py_tss_NEEDS_INIT;" in third party extension modules, which is what the current API gives us.

    The question is whether there's a reasonable compiler independent way to also support struct *assignment*, in addition to initialisation.

    @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
    Labels
    3.7 (EOL) end of life extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants