Skip to content

[C-API] Convert obvious unsafe macros to static inline functions #87668

Closed
Closed
[C-API] Convert obvious unsafe macros to static inline functions#87668
Labels
3.10 (EOL)end of lifetopic-C-API

Description

@erlend-aasland
erlend-aasland
opened on Mar 15, 2021
BPO 43502
Nosy @rhettinger, @vstinner, @corona10, @pablogsal, @erlend-aasland
PRs
  • bpo-43502: Convert PyExceptionClass_Check from macro to static inline function #24875
    		•
    Files
  • macros-that-reuse-args.txt
    		•

    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 2021-03-20.10:32:47.343>
created_at = <Date 2021-03-15.13:54:44.103>
labels = ['expert-C-API', '3.10']
title = '[C-API] Convert obvious unsafe macros to static inline functions'
updated_at = <Date 2021-03-20.10:32:47.342>
user = 'https://github.com/erlend-aasland'

    bugs.python.org fields:

    activity = <Date 2021-03-20.10:32:47.342>
actor = 'erlendaasland'
assignee = 'none'
closed = True
closed_date = <Date 2021-03-20.10:32:47.343>
closer = 'erlendaasland'
components = ['C API']
creation = <Date 2021-03-15.13:54:44.103>
creator = 'erlendaasland'
dependencies = []
files = ['49877']
hgrepos = []
issue_num = 43502
keywords = ['patch']
message_count = 11.0
messages = ['388739', '388740', '388741', '388742', '388839', '388870', '388893', '388899', '388910', '389078', '389153']
nosy_count = 5.0
nosy_names = ['rhettinger', 'vstinner', 'corona10', 'pablogsal', 'erlendaasland']
pr_nums = ['24875']
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue43502'
versions = ['Python 3.10']

    Activity

    1. erlend-aasland commented on Mar 15, 2021

      @erlend-aasland
      erlend-aasland
      on Mar 15, 2021
      ContributorAuthor

      Convert macros to static inline functions if...:

      • the macro contains a clear pitfall (for example "duplication of side effects")
      • the fix is trivial
      • the macro is not used as an l-value (for example Py_TYPE())

      See also:

    2. erlend-aasland
      added
      3.10 (EOL)end of life
      topic-C-API
      on Mar 15, 2021

    3. vstinner commented on Mar 15, 2021

      @vstinner
      vstinner
      on Mar 15, 2021
      Member

      For me, one of the my worst issue is when a macro evalutes an argument twice.

      Example:
      ---

      #include <stdio.h>

#define DOUBLE(value) ((value) + (value))

int main()
{
    int x = 1;
    // expanded to: ((++x) + (++x))
    int y = DOUBLE(++x);
    printf("x=%i y=%i\n", x, y);
    return 0;
}

      Surprising output:
      ---

      $ gcc x.c -o x; ./x
x=3 y=6

      Expected output:
      ---

      x=2 y=4

      Fixed code using a static inline function:
      ---

      #include <stdio.h>

static inline int DOUBLE(int value) {
    return value + value;
}

int main()
{
    int x = 1;
    // expanded to: DOUBLE(++x)
    int y = DOUBLE(++x);
    printf("x=%i y=%i\n", x, y);
    return 0;
}

      Output:
      ---

      $ gcc x.c -o x; ./x
x=2 y=4

    4. vstinner commented on Mar 15, 2021

      @vstinner
      vstinner
      on Mar 15, 2021
      Member

      I would add that we should pay attention to:

      • not introducing a backward incompatible C API change by mistake
      • not introducing new compiler warnings.

      A common source of warning is that macros have no type for their arguments or return value, whereas static inline functions are strictly types. A common pattern in the Python header file is to use _PyObject_CAST() or _PyObject_CONST_CAST(). Example:

      #define _PyObject_CAST(op) ((PyObject*)(op))
#define _PyObject_CAST_CONST(op) ((const PyObject*)(op))

static inline void _Py_INCREF(PyObject *op) { (...) }
#define Py_INCREF(op) _Py_INCREF(_PyObject_CAST(op))

      For macros of the *internal* C API, it's ok to becom more strict about types. For example, in the *public* C API, I replaced:

      #define PyObject_INIT(op, typeobj) \
    ( Py_TYPE(op) = (typeobj), _Py_NewReference((PyObject *)(op)), (op) )

      with:

      PyAPI_FUNC(PyObject *) PyObject_Init(PyObject *, PyTypeObject *);
#define PyObject_INIT(op, typeobj) PyObject_Init(_PyObject_CAST(op), (typeobj))

      But for the internal C API, I added:

      static inline void _PyObject_Init(PyObject *op, PyTypeObject *typeobj) { ... }

      which doesn't cast its arguments.

    5. erlend-aasland commented on Mar 15, 2021

      @erlend-aasland
      erlend-aasland
      on Mar 15, 2021
      ContributorAuthor

      Should we try to split all macros like that, if possible?

    6. erlend-aasland commented on Mar 16, 2021

      @erlend-aasland
      erlend-aasland
      on Mar 16, 2021
      ContributorAuthor

      Under Include/, including subdirectories, there are 88 macros that reuse arguments.

    7. erlend-aasland commented on Mar 16, 2021

      @erlend-aasland
      erlend-aasland
      on Mar 16, 2021
      ContributorAuthor

      PyUnicode_WRITE, PyUnicode_READ, and PyUnicode_READ_CHAR all reuse their arguments, and there's a lot of occurrences (58 AFAICS) where they are called with the increment operator applied to the index argument. For example:

      Objects/unicodeobject.c: PyUnicode_WRITE(kind, data, pos++, ch);
      Modules/_io/textio.c: c = PyUnicode_READ(kind, in_str, i++);
      Objects/stringlib/unicode_format.h: c = PyUnicode_READ_CHAR(self->str.str, self->index++);

      Would a single PR for all three of them be acceptable, Victor?

    8. rhettinger commented on Mar 16, 2021

      @rhettinger
      rhettinger
      on Mar 16, 2021
      Contributor

      Be careful about performance. We know for sure that macros will inline. In contrast, inline functions might or might not inline (there are rules for when it can be done). When applied across source files, inlining often only occurs with an LTO build — any other build can't inline across object files. Historically, we've never had to depend on LTO, so that would be a regression for some users. We've already have a performance bug that needed to be reverted because of too aggressive conversion of macros to inline functions.

      A problem with these blanket PRs is that they second guess the original developer who may have made a conscious and informed decision to use a macro. If you really think that developer was wrong, they should be included in the conversation and given the chance to opt in or out. Otherwise, we're mechanically undoing previous intelligent efforts.

      Ideally, there needs to be a more nuanced view than "macros are bad, inline functions are good".

    9. pablogsal commented on Mar 17, 2021

      @pablogsal
      pablogsal
      on Mar 17, 2021
      Member

      I agree we should be careful here. There are several things to consider:

      • Macros transformed into function may or may not get inlined so there is a risk of affecting performance if we just transform them in bulk.

      • Because the macro is defined as 'static inline' in the header file, then it *can* be inlined everywhere if the compiled decides to do it. It will be one copy symbol in the text segment on every compilation unit. Notice that 'inline' has a special meaning in C99 and it doesn't only mean "inline this function". It has visibility implications (see section 6.7.4 of the C99 standard).

      • There is risk to introducing a backwards-incompatible ABI change by mistake as Victor mentions.

      • This can affect also the link patterns of extension modules because these symbols may be visible in the resulting binaries when they were macro-level-inlined before. I cannot see any problem that can happen because of this but we should carefully consider it.

      In short, there is the value of converting these macros for extra type safety, but we need to at the very least be very careful and if we decide to go forward this needs as many reviews as possible so we don't miss anything. I would recommend maybe start a thread about this in python-dev.

    10. erlend-aasland commented on Mar 17, 2021

      @erlend-aasland
      erlend-aasland
      on Mar 17, 2021
      ContributorAuthor

      Pablo:

      I agree we should be careful here. There are several things to consider:

      Yes, and thanks for your thorough reply and insights. I do not suggest converting macros blindly, and I have no intention of doing so. Apart from the three PyUnicode_*() cases mentioned in msg388870, I've not found any misuse of macros that reuse their arguments in the CPython code base.

      Perhaps using a meta-issue is the wrong approach. I guess a PEP is too much? I'll start a thread on dpo (or on the list).

      Thanks.

    11. erlend-aasland commented on Mar 19, 2021

      @erlend-aasland
      erlend-aasland
      on Mar 19, 2021
      ContributorAuthor

      FYI, thread started on https://discuss.python.org/t/what-to-do-with-unsafe-macros/7771?u=erlendaasland

    12. erlend-aasland commented on Mar 20, 2021

      @erlend-aasland
      erlend-aasland
      on Mar 20, 2021
      ContributorAuthor

      Based on the feedback, I'm closing this. The PyUnicode_* macros, or the callers, can be fixed in a separate issue.

    13. erlend-aasland
      closed this as completedon Mar 20, 2021
    14. erlend-aasland
      closed this as completedon Mar 20, 2021
    15. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.10 (EOL)end of lifetopic-C-API

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions