Skip to content

bpo-37406: Remove debug checks in unicodeobject.c#14384

Closed
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:unicode_pydebug
Closed

bpo-37406: Remove debug checks in unicodeobject.c#14384
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:unicode_pydebug

Conversation

@vstinner

@vstinner vstinner commented Jun 25, 2019

Copy link
Copy Markdown
Member

When Python is built in release mode (when Py_DEBUG is not defined),
remove debug/sanity runtime checks in unicodeobject.c (checks using
PyErr_BadInternalCall() or PyErr_BadArgument()).

https://bugs.python.org/issue37406

@vstinner

Copy link
Copy Markdown
Member Author

There are still a few PyErr_BadInternalCall() and PyErr_BadArgument() in:

  • PyUnicode_RichCompare (invalid op)
  • PyUnicode_Append()
  • _PyUnicode_FormatLong(): Py_REFCNT(result) == 1 sanity check

@vstinner

Copy link
Copy Markdown
Member Author

Oh, this change breaks the sqlite module which expect that PyUnicode_AsUTF8AndSize() raises an excpetion if its argument is not a Unicode object:

    sql_cstr = PyUnicode_AsUTF8AndSize(sql, &sql_cstr_len);
    if (sql_cstr == NULL) {
        rc = PYSQLITE_SQL_WRONG_TYPE;
        return rc;
    }

@vstinner

Copy link
Copy Markdown
Member Author

I wrote PR #14386 to change the sqlite3 module.

When Python is built in release mode (when Py_DEBUG is not defined),
remove debug/sanity runtime checks in unicodeobject.c (checks using
PyErr_BadInternalCall() or PyErr_BadArgument()).
@jdemeyer

Copy link
Copy Markdown
Contributor

Wouldn't it be simpler to replace those checks by assertions then? For example

    assert(PyUnicode_Check(from));
    assert(PyUnicode_Check(to));

instead of

#ifdef Py_DEBUG
    if (!PyUnicode_Check(from) || !PyUnicode_Check(to)) {
        PyErr_BadInternalCall();
        return -1;
    }
#endif

@vstinner

Copy link
Copy Markdown
Member Author

Wouldn't it be simpler to replace those checks by assertions then?

That's a big behavior difference: currrently, it's possible to catch and handle the exception. If we start using assert(), the process is killed immediately by SIGABRT using abort(). I prefer to not change too many things at once :-)

@jdemeyer

Copy link
Copy Markdown
Contributor

That's a big behavior difference

Given that the default is to build without debug info, this PR is a big behavior difference: explicit checks are removed.

If we start using assert(), the process is killed immediately by SIGABRT using abort().

I actually consider that a good thing. It's easier to debug a SIGABRT than it is to debug a Python exception raised in C code.

But I'm not going to argue this further, it was just a suggestion.

@vstinner

Copy link
Copy Markdown
Member Author

Given that the default is to build without debug info, this PR is a big behavior difference: explicit checks are removed.

Almost everybody use release builds of Python. So yes, this PR changes the default behavior. It's a deliberate choice: https://bugs.python.org/issue37406

I was talking about no changing the behavior of the debug build.

@jdemeyer

jdemeyer commented Jul 1, 2019

Copy link
Copy Markdown
Contributor

There were some problems with the Azure CI. Can you close and re-open such that it gets tested again?

@jdemeyer

jdemeyer commented Jul 1, 2019

Copy link
Copy Markdown
Contributor

currrently, it's possible to catch and handle the exception

Yes, it's possible but it's completely meaningless to do that. It indicates a serious bug in the code, not something that should be handled in a try/except block.

@vstinner

vstinner commented Jul 1, 2019

Copy link
Copy Markdown
Member Author

There are discussions against this change in https://bugs.python.org/issue37406 My plan is to finish an article to explain the problem I am trying to solve here.

@jdemeyer jdemeyer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the code paths using PyErr_BadArgument instead of PyErr_BadInternalCall are meant for debug builds only. It's raising a TypeError and catching that is a reasonable thing to do. So this could cause applications that work fine in debug to crash in non-debug mode. That would be pretty bad.

So there are two ways to fix this inconsistency:

  1. Replace PyErr_BadArgument by PyErr_BadInternalCall inside those #ifdef Py_DEBUG blocks.
  2. Only use #ifdef Py_DEBUG for code already using PyErr_BadInternalCall

I would vote for option 1.

@jdemeyer

jdemeyer commented Jul 1, 2019

Copy link
Copy Markdown
Contributor

And this absolutely deserves a NEWS entry, as it's changing the behaviour of the C API.

@vstinner

vstinner commented Jul 1, 2019

Copy link
Copy Markdown
Member Author

@jdemeyer: Please discuss https://bugs.python.org/issue37406 since more people are involved there, and I prefer to not have the discussion at two different places.

@vstinner

Copy link
Copy Markdown
Member Author

@vstinner vstinner closed this Jul 16, 2019
@vstinner
vstinner deleted the unicode_pydebug branch January 19, 2021 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants