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

Disable runtime checks in release mode #81587

Closed
vstinner opened this issue Jun 25, 2019 · 7 comments
Closed

Disable runtime checks in release mode #81587

vstinner opened this issue Jun 25, 2019 · 7 comments
Labels
3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@vstinner
Copy link
Member

BPO 37406
Nosy @malemburg, @vstinner, @serhiy-storchaka, @jdemeyer
PRs
  • bpo-37406: Remove debug checks in unicodeobject.c #14384
  • bpo-37406: sqlite3 raises TypeError for wrong operation type #14386
  • 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 2019-07-16.10:13:32.293>
    created_at = <Date 2019-06-25.23:16:31.528>
    labels = ['interpreter-core', '3.9', 'performance']
    title = 'Disable runtime checks in release mode'
    updated_at = <Date 2019-07-16.10:13:32.288>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2019-07-16.10:13:32.288>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-07-16.10:13:32.293>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2019-06-25.23:16:31.528>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37406
    keywords = ['patch']
    message_count = 7.0
    messages = ['346570', '346577', '346588', '346643', '346983', '346995', '348012']
    nosy_count = 4.0
    nosy_names = ['lemburg', 'vstinner', 'serhiy.storchaka', 'jdemeyer']
    pr_nums = ['14384', '14386']
    priority = 'normal'
    resolution = 'out of date'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue37406'
    versions = ['Python 3.9']

    @vstinner
    Copy link
    Member Author

    A Python debug build is ABI compatible with a Python release build since Python 3.8 on most platforms (except Windows, Cygwin and Android):
    https://docs.python.org/3.8/whatsnew/3.8.html#debug-build-uses-the-same-abi-as-release-build

    It should now be way easier to debug an application with a debug build.

    I propose to remove runtime debug checks in release mode. assert() is already widely used in the C code base, but there are many runtime checks using PyErr_BadInternalCall() or PyErr_BadArgument. Example:

    Py_ssize_t
    PyUnicode_GetLength(PyObject *unicode)
    {
        if (!PyUnicode_Check(unicode)) {
            PyErr_BadArgument();
            return -1;
        }
        if (PyUnicode_READY(unicode) == -1)
            return -1;
        return PyUnicode_GET_LENGTH(unicode);
    }

    Attached PR removes these checks when Python is compiled in release mode: when Py_DEBUG is not defined.

    Even if I marked this issue as "performance", I don't expect a significant speedup.

    @vstinner vstinner added 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Jun 25, 2019
    @vstinner vstinner changed the title Disable runtime checks in release mode Disable debug runtime checks in release mode Jun 25, 2019
    @vstinner
    Copy link
    Member Author

    New changeset c6a2320 by Victor Stinner in branch 'master':
    bpo-37406: sqlite3 raises TypeError for wrong operation type (GH-14386)
    c6a2320

    @malemburg
    Copy link
    Member

    Given that extensions call these APIs, I find it highly risky to
    disable these checks in any version of the Python runtime and
    am -1 on such a change.

    Using assert() in C is a pretty bad alternative, since this crashes
    the whole process. It should really only be used where no other
    means of error handling are possible. Python's exception mechanism
    is a much better way to signal and handle such errors at the
    application level.

    --
    Marc-Andre Lemburg
    eGenix.com

    Professional Python Services directly from the Experts (#1, Jun 26 2019)
    >>> Python Projects, Coaching and Consulting ...  http://www.egenix.com/
    >>> Python Database Interfaces ...           http://products.egenix.com/
    >>> Plone/Zope Database Interfaces ...           http://zope.egenix.com/
    ________________________________________________________________________

    ::: We implement business ideas - efficiently in both time and costs :::

    eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48
    D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
    Registered at Amtsgericht Duesseldorf: HRB 46611
    http://www.egenix.com/company/contact/
    http://www.malemburg.com/

    @malemburg malemburg changed the title Disable debug runtime checks in release mode Disable runtime checks in release mode Jun 26, 2019
    @serhiy-storchaka
    Copy link
    Member

    I think that the public C API should have runtime checks for its arguments, except performance sensitive functions and macros (like PyTuple_GET_ITEM). The internal C API can use just asserts.

    I have a work-in-progress patch which adds tests for the public C API, including calls with invalid arguments.

    @jdemeyer
    Copy link
    Contributor

    jdemeyer commented Jul 1, 2019

    Python's exception mechanism is a much better way to signal and handle such errors at the application level.

    I disagree. There is a difference between exceptions which are possible in normal usage of the code and real bugs, which shouldn't ever happen. A NULL argument to a C API function is not an ordinary exception, it's a real bug. Even if we raise a Python exception for that, it should never be handled in a try/exception block like an ordinary exception.

    Using assert() in C is a pretty bad alternative, since this crashes the whole process.

    I *prefer* debugging an assert() failure (which is pretty easy with a decent debugger) rather than debugging a Python exception raised from C (which typically does NOT give a useful traceback).

    @vstinner
    Copy link
    Member Author

    vstinner commented Jul 1, 2019

    FYI I'm finishing an article to explain the problem that I am trying to solve here. Sorry for the delay, I will reply later :-)

    @vstinner
    Copy link
    Member Author

    I didn't find time to write a proper a rationale for this change, and I will not be available next weeks. So I prefer to close this issue. I may reopen it later once I have a strong rationale :-) In the meanwhile, have a look at the new PyHandle API idea which will start without sanity check in release mode since day 1 :-)
    https://github.com/pyhandle/pyhandle/

    @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.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants