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

Compiler warnings in _PyObject_CallArg1() #73041

Closed
vstinner opened this issue Dec 2, 2016 · 6 comments
Closed

Compiler warnings in _PyObject_CallArg1() #73041

vstinner opened this issue Dec 2, 2016 · 6 comments
Labels
3.7 (EOL) end of life

Comments

@vstinner
Copy link
Member

vstinner commented Dec 2, 2016

BPO 28855
Nosy @vstinner, @benjaminp, @serhiy-storchaka
Files
  • static_inline.patch
  • 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 2016-12-02.06:01:42.923>
    created_at = <Date 2016-12-02.00:38:43.561>
    labels = ['3.7']
    title = 'Compiler warnings in _PyObject_CallArg1()'
    updated_at = <Date 2016-12-02.11:35:37.589>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2016-12-02.11:35:37.589>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-12-02.06:01:42.923>
    closer = 'python-dev'
    components = []
    creation = <Date 2016-12-02.00:38:43.561>
    creator = 'vstinner'
    dependencies = []
    files = ['45729']
    hgrepos = []
    issue_num = 28855
    keywords = ['patch']
    message_count = 6.0
    messages = ['282212', '282220', '282221', '282222', '282230', '282237']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'benjamin.peterson', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue28855'
    versions = ['Python 3.7']

    @vstinner
    Copy link
    Member Author

    vstinner commented Dec 2, 2016

    _PyObject_CallArg1() is the following macro:
    ---

    #define _PyObject_CallArg1(func, arg) \
        _PyObject_FastCall((func), &(arg), 1)

    It works well in most cases, but my change 8f258245c391 or change b9c9691c72c5 added compiler warnings, because an argument which is not directly a PyObject* type is passed as "arg".

    I tried to cast in the caller: _PyObject_CallArg1(func, (PyObject*)arg), but sadly it doesn't work :-( I get a compiler error.

    Another option is to cast after "&" in the macro:
    ---

     #define _PyObject_CallArg1(func, arg) \
    -    _PyObject_FastCall((func), &(arg), 1)
    +    _PyObject_FastCall((func), (PyObject **)&(arg), 1)

    This option may hide real bugs, so I dislike it.

    A better option is to stop using ugly C macros and use a modern static inline function: see attached static_inline.patch. This patch casts to PyObject* before calling _PyObject_CallArg1() to fix warnings.

    The question is if compilers are able to emit efficient code for static inline functions using "&" on an argument. I wrote the macro to implement the "&" optimization: convert a PyObject* to a stack of arguments: C array of PyObject* objects.

    @vstinner vstinner added the 3.7 (EOL) end of life label Dec 2, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 2, 2016

    New changeset 96245d4af0ca by Benjamin Peterson in branch 'default':
    fix _PyObject_CallArg1 compiler warnings (closes bpo-28855)
    https://hg.python.org/cpython/rev/96245d4af0ca

    @python-dev python-dev mannequin closed this as completed Dec 2, 2016
    @benjaminp
    Copy link
    Contributor

    It doesn't seem like the question is whether to use inline functions but whether to force all callers to cast. Your original code would work if you added all the casts in your static_inline.patch patch.

    @benjaminp
    Copy link
    Contributor

    (Sorry, I noticed and landed a fix before completely reading the issue.)

    @benjaminp
    Copy link
    Contributor

    I also think forcing callers to cast is fine. Most of our APIs require PyObject *.

    @vstinner
    Copy link
    Member Author

    vstinner commented Dec 2, 2016

    No problem, thanks for the fix Benjamin!

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

    No branches or pull requests

    2 participants