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

Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy #46695

Closed
rolland mannequin opened this issue Mar 21, 2008 · 15 comments
Closed

Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy #46695

rolland mannequin opened this issue Mar 21, 2008 · 15 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@rolland
Copy link
Mannequin

rolland mannequin commented Mar 21, 2008

BPO 2443
Nosy @abalkin, @pitrou, @tiran, @benjaminp, @bitdancer
Files
  • issue2443.diff: Patch against revision 61893
  • issue2443a.diff: Patch against revision 78398
  • issue2443-py3k.diff
  • 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 = 'https://github.com/abalkin'
    closed_at = <Date 2010-08-11.22:12:59.423>
    created_at = <Date 2008-03-21.10:27:37.295>
    labels = ['interpreter-core', 'type-feature']
    title = 'Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy'
    updated_at = <Date 2010-08-11.22:12:59.422>
    user = 'https://bugs.python.org/rolland'

    bugs.python.org fields:

    activity = <Date 2010-08-11.22:12:59.422>
    actor = 'belopolsky'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2010-08-11.22:12:59.423>
    closer = 'belopolsky'
    components = ['Interpreter Core']
    creation = <Date 2008-03-21.10:27:37.295>
    creator = 'rolland'
    dependencies = []
    files = ['9849', '16349', '18477']
    hgrepos = []
    issue_num = 2443
    keywords = ['patch']
    message_count = 15.0
    messages = ['64234', '64250', '64483', '64487', '64500', '64523', '64588', '70468', '84605', '88611', '99987', '99989', '113597', '113608', '113635']
    nosy_count = 7.0
    nosy_names = ['belopolsky', 'pitrou', 'christian.heimes', 'benjamin.peterson', 'stutzbach', 'rolland', 'r.david.murray']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue2443'
    versions = ['Python 3.2']

    @rolland
    Copy link
    Mannequin Author

    rolland mannequin commented Mar 21, 2008

    In many files, the following code is present (with slight variations,
    but the important part is there) :
    static PyObject *
    objargs_mktuple(va_list va)
    {
    	int i, n = 0;
    	va_list countva;
    	PyObject *result, *tmp;
    
    #ifdef VA_LIST_IS_ARRAY
    	memcpy(countva, va, sizeof(va_list));
    #else
    #ifdef __va_copy
    	__va_copy(countva, va);
    #else
    	countva = va;
    #endif
    #endif

    ...

    memcpy() is accessing va_list before it is initialized.

    Before the first access to a va_list type variable, and after the last
    access to that variable, calls to va_start() and va_end() must be made
    to initialize and free the variable.

    Such behaviour should be corrected in the following files :

    @rolland rolland mannequin added build The build process and cross-build labels Mar 21, 2008
    @tiran
    Copy link
    Member

    tiran commented Mar 21, 2008

    Can you provide a patch for 2.6 against the latest svn checkout of the
    trunk please?

    @tiran tiran added interpreter-core (Objects, Python, Grammar, and Parser dirs) and removed build The build process and cross-build labels Mar 21, 2008
    @abalkin
    Copy link
    Member

    abalkin commented Mar 25, 2008

    This is not a bug. All the reported functions expect va_list argument
    to be initialized before being called. AFAICT, they are consistently
    used in this way. For example,

    PyObject *
    PyObject_CallFunctionObjArgs(PyObject *callable, ...)
    {
            PyObject *args, *tmp;
            va_list vargs;
        if (callable == NULL)
                return null_error();
    
            /* count the args */
            va_start(vargs, callable);
            args = objargs_mktuple(vargs);
            va_end(vargs);
            if (args == NULL)
                    return NULL;
            tmp = PyObject_Call(callable, args, NULL);
            Py_DECREF(args);
    
            return tmp;
    }

    This may need to be clarified in the docs. For example, PyString_FromFormatV does not mention that vargs needs to be
    initialized: <http://docs.python.org/dev/c-
    api/string.html#PyString_FromFormatV>. On the other hand, this may be
    obvious to most C programmers.

    I suggest to close this issue as invalid.

    @abalkin
    Copy link
    Member

    abalkin commented Mar 25, 2008

    On the second thought the macro dance highlighted by OP belongs to
    pyport.h. Attached patch defines Py_VA_COPY macro and uses it to simplify
    va_list copying code.

    @tiran
    Copy link
    Member

    tiran commented Mar 25, 2008

    Looks like a good idea to me

    @abalkin abalkin changed the title uninitialized access to va_list Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy Mar 25, 2008
    @abalkin abalkin added type-feature A feature request or enhancement and removed build The build process and cross-build labels Mar 25, 2008
    @rolland
    Copy link
    Mannequin Author

    rolland mannequin commented Mar 25, 2008

    This is what I meant. The initialization should be done by calling
    va_start(count_va); as you described.
    In the files and lines I reported though, this is not called.
    I'll file a patch for it soon.
    --Rolland Dudemaine

    Alexander Belopolsky wrote:

    Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:

    This is not a bug. All the reported functions expect va_list argument
    to be initialized before being called. AFAICT, they are consistently
    used in this way. For example,

    PyObject *
    PyObject_CallFunctionObjArgs(PyObject *callable, ...)
    {
    PyObject *args, *tmp;
    va_list vargs;

        if (callable == NULL)
                return null_error();
    
        /* count the args \*/
        va_start(vargs, callable);
        args = objargs_mktuple(vargs);
        va_end(vargs);
        if (args == NULL)
                return NULL;
        tmp = PyObject_Call(callable, args, NULL);
        Py_DECREF(args);
    
        return tmp;
    

    }

    This may need to be clarified in the docs. For example, PyString_FromFormatV does not mention that vargs needs to be
    initialized: <http://docs.python.org/dev/c-
    api/string.html#PyString_FromFormatV>. On the other hand, this may be
    obvious to most C programmers.

    I suggest to close this issue as invalid.

    ----------
    nosy: +belopolsky


    Tracker <report@bugs.python.org>
    <http://bugs.python.org/issue2443\>


    @rolland rolland mannequin changed the title Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy uninitialized access to va_list Mar 25, 2008
    @rolland
    Copy link
    Mannequin Author

    rolland mannequin commented Mar 27, 2008

    Actually, this thing is more complex to solve than I thought.
    Specifically, as described in
    http://www.opengroup.org/onlinepubs/007908775/xsh/stdarg.h.html stdarg
    requires that variable argument functions have at least one fixed argument.
    This is implied by the declaration of "void va_start(va_list ap, argN);".
    As explained in the original ticket description, and also described
    before in the above link, va_start() must be called before any call to
    va_arg(), and this includes any access to the argument list using
    __va_copy namely.

    The problem is that at least objargs_mktuple(), line 2649 of
    Objects/abstract.c does not have a first fixed argument.

    @benjaminp
    Copy link
    Contributor

    What's the status of this?

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Mar 30, 2009

    Rolland,

    The va_list is initialized by the function that calls objargs_mktuple.
    va_start() and va_end() need to be called in the function that takes
    "..." as a parameter, and it is.

    Not a bug, but +1 on Alexander's patch to consolidate all the #ifdef's
    for cleanliness.

    @bitdancer
    Copy link
    Member

    Reducing the priority and updating the target releases, since from the
    discussion there doesn't appear to be a bug here.

    @AlexanderBelopolsky
    Copy link
    Mannequin

    AlexanderBelopolsky mannequin commented Feb 24, 2010

    Updated the patch. In a year one more copy of va_copy macro dance have cropped in. Any reason this is still languishing?

    @AlexanderBelopolsky
    Copy link
    Mannequin

    AlexanderBelopolsky mannequin commented Feb 24, 2010

    Rolland,

    Please don't revert the title. The consensus is that there is no uninitialized access to va_list.

    @AlexanderBelopolsky AlexanderBelopolsky mannequin changed the title uninitialized access to va_list Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy Feb 24, 2010
    @abalkin abalkin self-assigned this Aug 11, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Aug 11, 2010

    Looks like a good idea. Don't other compilers have __va_copy equivalents?
    Apparently, C99 defines va_copy(), which we could use conditionally.

    @abalkin
    Copy link
    Member

    abalkin commented Aug 11, 2010

    I updated the patch for 3.x. I agree that using va_copy where available makes sense, but let's leave this type of improvements for the future.

    @abalkin
    Copy link
    Member

    abalkin commented Aug 11, 2010

    Committed in r83949.

    @abalkin abalkin closed this as completed Aug 11, 2010
    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants