homepage

Attached patch changes PyObject_Call() and PyCFunction_Call() to raise a SystemError and destroy the result (Py_DECREF) if a function returns a result with an exception set.
I also refactored PyCFunction_Call() and added "assert(!PyErr_Occurred());" at the beginning of PyCFunction_Call().
I removed duplicate checks in ceval.c.
--
I didn't check the impact on performances.
If the patch has a significant overhead: _Py_CheckFunctionResult() may be marked to be inlined, and PyCFunction_Call() and PyObject_Call() checks may be marked as unlikely using GCC __builtin_expect(), something like:
#ifdef __GNUC__
# define Py_likely(x) __builtin_expect((x),1)
# define Py_unlikely(x) __builtin_expect((x),0)
#else
# define Py_likely(x) x
# define Py_unlikely(x) x
#endif
Be careful of __builtin_expect(), misused it can make the code slower! (benchmark, benchmark, benchmark!)

> If the patch has a significant overhead: _Py_CheckFunctionResult() may be
> marked to be inlined, and PyCFunction_Call() and PyObject_Call() checks may
> be marked as unlikely using GCC __builtin_expect(), something like:
Could you please open separate issue or even a topic on Python-Ideas for 
Py_likely/Py_unlikely? I prefer spelling _Py_LIKELY.

2015年03月03日 13:49 GMT+01:00 Serhiy Storchaka <report@bugs.python.org>:
>> If the patch has a significant overhead: _Py_CheckFunctionResult() may be
>> marked to be inlined, and PyCFunction_Call() and PyObject_Call() checks may
>> be marked as unlikely using GCC __builtin_expect(), something like:
>
> Could you please open separate issue or even a topic on Python-Ideas for
> Py_likely/Py_unlikely? I prefer spelling _Py_LIKELY.
I'm not really interested by micro-benchmark here :-) I also propose
solutions *if* my patch makes the code slower.
Since __builtin_expect() may have a negative effect on performances, I
would prefer to avoid it if we can :-)

I ran http://hg.python.org/benchmarks/
Result:
---
$ python3 perf.py -r -b default ~/prog/python/default/python.orig ~/prog/python/default/python.patched 
INFO:root:Automatically selected timer: perf_counter
(...)
Report on Linux smithers 3.18.3-201.fc21.x86_64 #1 SMP Mon Jan 19 15:59:31 UTC 2015 x86_64 x86_64
Total CPU cores: 8
### etree_parse ###
Min: 0.273882 -> 0.297294: 1.09x slower
Avg: 0.277947 -> 0.312787: 1.13x slower
Significant (t=-11.54)
Stddev: 0.00446 -> 0.02987: 6.6906x larger
### fastunpickle ###
Min: 0.510721 -> 0.526520: 1.03x slower
Avg: 0.521282 -> 0.542610: 1.04x slower
Significant (t=-8.33)
Stddev: 0.01024 -> 0.02348: 2.2931x larger
The following not significant results are hidden, use -v to show them:
2to3, django_v2, etree_generate, etree_iterparse, etree_process, fastpickle, json_dump_v2, json_load, nbody, regex_v8, tornado_http.
---
I ran again etree_parse alone and the slowdown disappeared :-p
---
The following not significant results are hidden, use -v to show them:
etree_parse.
---
Output when I ran fastunpickle alone:
---
### fastunpickle ###
Min: 0.510994 -> 0.522419: 1.02x slower
Avg: 0.524633 -> 0.536876: 1.02x slower
Significant (t=-2.69)
Stddev: 0.02983 -> 0.03436: 1.1518x larger
---
2% slower on a single test doesn't look to be significant. Raising a SystemError is more important than the minor slowdown. What do you think?

I ran pybench, even if I know that it's not really revelant: overall 0.3% slowdown. But pybench doesn't look reliable: some tests are faster, which looks like noise in the benchmark.
-------------------------------------------------------------------------------
PYBENCH 2.1
-------------------------------------------------------------------------------
* using CPython 3.5.0a1+ (default:648b35f22b91+, Mar 6 2015, 13:18:57) [GCC 4.9.2 20141101 (Red Hat 4.9.2-1)]
* disabled garbage collection
* system check interval set to maximum: 2147483647
* using timer: time.perf_counter
* timer: resolution=1e-09, implementation=clock_gettime(CLOCK_MONOTONIC)
-------------------------------------------------------------------------------
Benchmark: orig.pybench
-------------------------------------------------------------------------------
 Rounds: 10
 Warp: 10
 Timer: time.perf_counter
 Machine Details:
 Platform ID: Linux-3.18.3-201.fc21.x86_64-x86_64-with-fedora-21-Twenty_One
 Processor: x86_64
 
 Python:
 Implementation: CPython
 Executable: /home/haypo/prog/python/default/python.orig
 Version: 3.5.0a1+
 Compiler: GCC 4.9.2 20141101 (Red Hat 4.9.2-1)
 Bits: 64bit
 Build: Mar 6 2015 13:19:47 (#default:648b35f22b91)
 Unicode: UCS4
-------------------------------------------------------------------------------
Comparing with: patch.pybench
-------------------------------------------------------------------------------
 Rounds: 10
 Warp: 10
 Timer: time.perf_counter
 Machine Details:
 Platform ID: Linux-3.18.3-201.fc21.x86_64-x86_64-with-fedora-21-Twenty_One
 Processor: x86_64
 
 Python:
 Implementation: CPython
 Executable: /home/haypo/prog/python/default/python.patched
 Version: 3.5.0a1+
 Compiler: GCC 4.9.2 20141101 (Red Hat 4.9.2-1)
 Bits: 64bit
 Build: Mar 6 2015 13:18:57 (#default:648b35f22b91+)
 Unicode: UCS4
Test minimum run-time average run-time
 this other diff this other diff
-------------------------------------------------------------------------------
 BuiltinFunctionCalls: 41ms 41ms -1.1% 41ms 41ms -1.5%
 BuiltinMethodLookup: 24ms 24ms +0.3% 24ms 25ms -0.8%
 CompareFloats: 26ms 26ms +2.2% 27ms 26ms +1.2%
 CompareFloatsIntegers: 59ms 60ms -0.9% 60ms 61ms -2.5%
 CompareIntegers: 38ms 36ms +4.6% 38ms 37ms +3.5%
 CompareInternedStrings: 26ms 25ms +0.6% 26ms 26ms -0.5%
 CompareLongs: 22ms 21ms +1.1% 22ms 22ms +0.0%
 CompareStrings: 22ms 23ms -1.3% 23ms 23ms -1.3%
 ComplexPythonFunctionCalls: 40ms 40ms -1.5% 40ms 41ms -2.5%
 ConcatStrings: 28ms 28ms -0.3% 28ms 29ms -1.2%
 CreateInstances: 41ms 42ms -1.7% 42ms 43ms -2.6%
 CreateNewInstances: 31ms 32ms -2.2% 32ms 33ms -3.2%
 CreateStringsWithConcat: 59ms 56ms +5.6% 60ms 58ms +2.7%
 DictCreation: 43ms 48ms -10.3% 44ms 50ms -12.7%
 DictWithFloatKeys: 36ms 37ms -4.8% 36ms 38ms -6.0%
 DictWithIntegerKeys: 28ms 29ms -4.1% 29ms 31ms -6.4%
 DictWithStringKeys: 24ms 26ms -8.9% 24ms 27ms -10.3%
 ForLoops: 22ms 22ms -0.9% 22ms 23ms -2.2%
 IfThenElse: 31ms 33ms -5.9% 31ms 34ms -7.9%
 ListSlicing: 35ms 35ms -0.1% 36ms 36ms -2.0%
 NestedForLoops: 34ms 35ms -2.3% 34ms 36ms -4.7%
 NestedListComprehensions: 37ms 37ms -1.2% 37ms 40ms -7.6%
 NormalClassAttribute: 77ms 71ms +8.1% 77ms 74ms +3.9%
 NormalInstanceAttribute: 37ms 36ms +2.1% 38ms 38ms -0.6%
 PythonFunctionCalls: 35ms 34ms +1.7% 35ms 35ms -0.5%
 PythonMethodCalls: 45ms 44ms +3.3% 46ms 45ms +0.7%
 Recursion: 60ms 58ms +3.7% 60ms 59ms +1.0%
 SecondImport: 33ms 33ms -0.0% 33ms 33ms -1.9%
 SecondPackageImport: 34ms 34ms +0.1% 34ms 34ms -2.0%
 SecondSubmoduleImport: 83ms 86ms -3.5% 85ms 87ms -2.4%
 SimpleComplexArithmetic: 23ms 23ms +0.7% 23ms 24ms -2.2%
 SimpleDictManipulation: 51ms 53ms -3.8% 52ms 55ms -5.7%
 SimpleFloatArithmetic: 24ms 24ms +0.3% 24ms 25ms -2.0%
 SimpleIntFloatArithmetic: 31ms 30ms +1.1% 31ms 31ms -0.1%
 SimpleIntegerArithmetic: 30ms 30ms +0.0% 30ms 31ms -2.0%
 SimpleListComprehensions: 30ms 32ms -4.6% 31ms 33ms -5.7%
 SimpleListManipulation: 27ms 26ms +2.4% 27ms 27ms +0.4%
 SimpleLongArithmetic: 21ms 21ms +2.1% 22ms 22ms +0.2%
 SmallLists: 36ms 36ms -1.3% 36ms 37ms -3.0%
 SmallTuples: 43ms 43ms -0.5% 44ms 45ms -2.5%
 SpecialClassAttribute: 75ms 72ms +4.3% 75ms 73ms +2.5%
 SpecialInstanceAttribute: 38ms 41ms -7.7% 38ms 42ms -9.0%
 StringMappings: 77ms 76ms +1.4% 77ms 78ms -0.3%
 StringPredicates: 43ms 44ms -2.6% 44ms 45ms -3.9%
 StringSlicing: 39ms 38ms +3.2% 39ms 39ms +1.0%
 TryExcept: 21ms 21ms -2.8% 21ms 22ms -4.8%
 TryFinally: 29ms 29ms +0.2% 29ms 29ms -1.2%
 TryRaiseExcept: 11ms 11ms -0.2% 11ms 11ms -1.1%
 TupleSlicing: 42ms 43ms -0.8% 45ms 45ms +0.5%
 WithFinally: 45ms 45ms -0.8% 45ms 45ms -1.2%
 WithRaiseExcept: 36ms 36ms -0.9% 36ms 36ms -1.5%
-------------------------------------------------------------------------------
Totals: 1920ms 1927ms -0.3% 1941ms 1980ms -2.0%
(this=orig.pybench, other=patch.pybench)

Even worse benchmark: timeit :-)
python -m timeit -s 'def f(): pass' 'f()'
* original: 67.5 nanoseconds
* patched: 64.1 nanoseconds (faster!?)

New patch which removes some assertions and move other assertions on result/PyErr_Occurred.
I removed useless duplicated assertions and move the final assertion at the end of PyEval_EvalFrameEx() to guarantee that the function behave correctly.

New patch to take in account Serhiy's comments. I also fixed call_function(), I forgot to call _Py_CheckFunctionResult() in two cases.

I'm not sure that all removed assertions are not needed. There are many ways to get raised exception, not only by calling a function.

Updated patch to take in account more Serhiy's comments.

Could you please run microbenchmarks from issue23507?

Oh, test_io and test_sqlite are failing in release mode with check_result-4.patch. io and sqlite modules only call PyErr_Clear() in debug mode. It's my fault, I added them, but I made them conditionnal to not impact performances. It's now fixed in the new patch.
Patch 5:
- "./python -m test -j0 -rW" succeeded in debug mode
- "./python -m test -j0 -rW" succeeded in release mode
- pybench: overall 0.6% slower with the patch (on the minimum runtime column)
Micro-benchmarks of issue #23507:
$ ./python -m timeit -s "f = lambda x: x" -s "s = list(range(1000))" -- "list(filter(f, s))"
Unpatched: 10000 loops, best of 3: 96.3 usec per loop
Patched: 10000 loops, best of 3: 99.2 usec per loop (+3.0%)
$ ./python -m timeit -s "f = lambda x: x" -s "s = list(range(1000))" -- "list(map(f, s))"
Unpatched: 10000 loops, best of 3: 89 usec per loop
Patched: 10000 loops, best of 3: 92.3 usec per loop (+3.7%)
$ ./python -m timeit -s "f = lambda x: x" -s "s = list(range(1000))" -- "sorted(s, key=f)"
Unpatched: 10000 loops, best of 3: 104 usec per loop
Patched: 10000 loops, best of 3: 106 usec per loop (+1.9%)

We can ignore such little slow down (if this is not just compiler artifact).
The patch LGTM. I afraid there will be issues with third-party code that will need an adaptation to 3.5, but on other hand, this patch could help with catching some hidden bugs.

2015年03月06日 16:55 GMT+01:00 Serhiy Storchaka <report@bugs.python.org>:
> We can ignore such little slow down (if this is not just compiler artifact).
Maybe it's time to play with micro-optimizations like __builtin_expect
:-) But I would prefer to do that in a separated issue. The patch is
already long and complex.
> The patch LGTM. I afraid there will be issues with third-party code that will need an adaptation to 3.5, but on other hand, this patch could help with catching some hidden bugs.
My patch doesn't make more strict. Without my patch, if a function
returns a result and raise an exception, the exception will still be
noticed, but probably later. The purpose of my change is to notify
immediatly that the function raised an exception.
Without my patch, if a function returns a result and raise an
exception, it's possible that the exception is cleared and so never
seen.
In older Python versions, Python/ceval.c displayed the message "XXX
undetected error" when an exception was raised but also a result was
returned.
Anyway, you must fix your code :-)

New changeset 97ef38236dc1 by Victor Stinner in branch 'default':
Issue #23571: PyObject_Call(), PyCFunction_Call() and call_function() now
https://hg.python.org/cpython/rev/97ef38236dc1 

Thanks for the very useful reviews Serhiy!

New changeset 52c7017fdcdd by Victor Stinner in branch 'default':
Issue #23571: Oops, fix #ifdef assert()
https://hg.python.org/cpython/rev/52c7017fdcdd 

New changeset 91fbe0fff882 by Victor Stinner in branch 'default':
Issue #23571: Restore removed assert(!PyErr_Occurred()); in
https://hg.python.org/cpython/rev/91fbe0fff882 

Can we add a note (probably with an example) to the Porting to Python 3.5 section of https://docs.python.org/3.5/whatsnew/3.5.html#porting-to-python-3-5
I believe that this patch exposes some subtle bugs in Django (see https://gist.github.com/berkerpeksag/8b8dbe594eb1a1c51275) and it would be great to add a note for third party libraries.

Serhiy was right: we should mention the name of the function in the
exception. In the Django traceback, it's not easy to guess what raised the
SystemError.
Ok to mention the change in What's New in Python 3.5.

Instead of an assert(), you could use Py_FatalError() at the end of _Py_CheckFunctionResult().

New changeset f30a5f6a665c by Victor Stinner in branch 'default':
Issue #23571: _Py_CheckFunctionResult() now gives the name of the function
https://hg.python.org/cpython/rev/f30a5f6a665c 

> I believe that this patch exposes some subtle bugs in Django (see https://gist.github.com/berkerpeksag/8b8dbe594eb1a1c51275) and it would be great to add a note for third party libraries.
Berker, can you please rerun your test with my new commit to check if you see the function error in the error?
(I didn't update the doc yet, not investigated Antoine's suggestion, I will do that later.)

Non-Linux buildbots failed.
http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%203.x/builds/2798/steps/test/logs/stdio
======================================================================
FAIL: test_return_result_with_error (test.test_capi.CAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_capi.py", line 206, in test_return_result_with_error
 self.assertIn(b'_Py_CheckFunctionResult: Assertion', err)
AssertionError: b'_Py_CheckFunctionResult: Assertion' not found in b'2015-03-21 10:28:29.114 defaults[68384:903] \nThe domain/default pair of (com.apple.CrashReporter, DialogType) does not exist\nAssertion failed: (!err_occurred), function _Py_CheckFunctionResult, file Objects/abstract.c, line 2088.\nFatal Python error: Aborted\n\nCurrent thread 0x00007fff71296cc0 (most recent call first):\n File "<string>", line 6 in <module>'
----------------------------------------------------------------------
http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/5906/steps/test/logs/stdio
======================================================================
FAIL: test_return_result_with_error (test.test_capi.CAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_capi.py", line 206, in test_return_result_with_error
 self.assertIn(b'_Py_CheckFunctionResult: Assertion', err)
AssertionError: b'_Py_CheckFunctionResult: Assertion' not found in b'2015-03-21 10:28:29.114 defaults[68384:903] \nThe domain/default pair of (com.apple.CrashReporter, DialogType) does not exist\nAssertion failed: (!err_occurred), function _Py_CheckFunctionResult, file Objects/abstract.c, line 2088.\nFatal Python error: Aborted\n\nCurrent thread 0x00007fff71296cc0 (most recent call first):\n File "<string>", line 6 in <module>'
----------------------------------------------------------------------
Why this assert is needed? Why not always raise SystemError?

New changeset 970f33dff5ca by Victor Stinner in branch 'default':
Issue #23571: Fix test_capi
https://hg.python.org/cpython/rev/970f33dff5ca 

Serhiy Storchaka added the comment:
> Why this assert is needed? Why not always raise SystemError?
A SystemError exception may be ignored by a generic "except Exception:
pass" or logged at debug level at then ignored.
I consider that the bug is important, and that you must fix it. The
debug mode is to detect bugs earlier, right?

This exception or assertion is triggered by the bug in CPython or in an extension. CPython developer uses release and debug builds of CPython and can get both exception or assertion. An extension developer usually uses release build of CPython with release and debug builds of an extension and can't get an assertion in CPython. Common Python user uses release builds of CPython and extensions and can only report about an exception. So this assertion can be used only in CPython developing and doesn't help to catch a bug in extensions. But an assertion itself provides less information than an exception. Debug build is less informative than release build.
May be add a runtime flag to control the reaction on system errors? If it set, all raised SystemError will be converted to fatal errors.

Le 21 mars 2015 18:05, "Serhiy Storchaka" <report@bugs.python.org> a écrit :
> But an assertion itself provides less information than an exception.
Debug build is less informative than release build.
I like Antoine's idea to replace the assertion with Py_FatalError() in
debug mode.
I'm more concerned by bugs in Python itself. A fatal error/assertion should
be noticed quickly on buildbots which compile python in debug mode.
It's just fine if other people use the release mode and get an exception.
> May be add a runtime flag to control the reaction on system errors? If it
set, all raised SystemError will be converted to fatal errors.
It's already what I do when running Python test suite with pyfailmalloc. I
modify manually SystemError constructor in the C code. A flag may help, but
you have to know that many tests expect SystemError (I don't remember which
ones).
Victor

Here's an exception in Django after the latest patch. The Django code block in the last exception catches ValueError, but this doesn't seem to work any longer since it's "wrapped" in SystemError. As Berker mentioned, some upgrade tips would be great as I'm not sure what adaptions in Django need to be made.
Traceback (most recent call last):
 File "/home/tim/code/django/django/contrib/contenttypes/views.py", line 17, in shortcut
 content_type = ContentType.objects.get(pk=content_type_id)
 File "/home/tim/code/django/django/db/models/manager.py", line 127, in manager_method
 return getattr(self.get_queryset(), name)(*args, **kwargs)
 File "/home/tim/code/django/django/db/models/query.py", line 387, in get
 self.model._meta.object_name
django.contrib.contenttypes.models.DoesNotExist: ContentType matching query does not exist.
...
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
ValueError: could not convert string to float: request_path
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
....
 File "/home/tim/code/django/django/template/base.py", line 792, in __init__
 self.literal = float(var)
SystemError: <class 'ValueError'> returned a result with an error set

Minimal reproducer:
>>> try:
... 1/0
... except:
... int('spam')
... 
Traceback (most recent call last):
 File "<stdin>", line 2, in <module>
ZeroDivisionError: division by zero
During handling of the above exception, another exception occurred:
ValueError: invalid literal for int() with base 10: 'spam'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
 File "<stdin>", line 4, in <module>
SystemError: <class 'ValueError'> returned a result with an error set

New changeset efb2c9ac2f88 by Victor Stinner in branch '3.4':
Issue #23571: Enhance Py_FatalError()
https://hg.python.org/cpython/rev/efb2c9ac2f88
New changeset 6303795f035a by Victor Stinner in branch 'default':
(Merge 3.4) Issue #23571: Enhance Py_FatalError()
https://hg.python.org/cpython/rev/6303795f035a 

New changeset 2e14ca478a57 by Victor Stinner in branch 'default':
Issue #23571: PyErr_FormatV() and PyErr_SetObject() now always clear the
https://hg.python.org/cpython/rev/2e14ca478a57 

That last commit fixed compatibility with Django.

New changeset 850b9dcd0534 by Victor Stinner in branch 'default':
Issue #23571: In debug mode, _Py_CheckFunctionResult() now calls
https://hg.python.org/cpython/rev/850b9dcd0534
New changeset da252f12352a by Victor Stinner in branch '3.4':
Issue #23571: Py_FatalError() now tries to flush sys.stdout and sys.stderr
https://hg.python.org/cpython/rev/da252f12352a
New changeset c9bcf669d807 by Victor Stinner in branch 'default':
(Merge 3.4) Issue #23571: Py_FatalError() now tries to flush sys.stdout and
https://hg.python.org/cpython/rev/c9bcf669d807
New changeset 57550e1f57d9 by Victor Stinner in branch 'default':
Issue #23571: Update test_capi
https://hg.python.org/cpython/rev/57550e1f57d9 

New changeset 2a18b958f1e1 by Victor Stinner in branch 'default':
Issue #23571: Enhance _Py_CheckFunctionResult()
https://hg.python.org/cpython/rev/2a18b958f1e1 

New changeset 1c2376825dd2 by Victor Stinner in branch '3.4':
Issue #23571: Fix reentrant call to Py_FatalError()
https://hg.python.org/cpython/rev/1c2376825dd2
New changeset e9ba95418af8 by Victor Stinner in branch 'default':
(Merge 3.4) Issue #23571: Fix reentrant call to Py_FatalError()
https://hg.python.org/cpython/rev/e9ba95418af8 

New changeset 004e3870d9e6 by Victor Stinner in branch '3.4':
Issue #23571: If io.TextIOWrapper constructor fails in _Py_DisplaySourceLine(),
https://hg.python.org/cpython/rev/004e3870d9e6 

> That last commit fixed compatibility with Django.
Good.
> Instead of an assert(), you could use Py_FatalError() at the end of _Py_CheckFunctionResult().
_Py_CheckFunctionResult() now calls Py_FatalError() in debug mode. By the way, I fixed various issues in Py_FatalError() to be able to display the exception and the traceback in more cases.
@Serhiy: Are you ok with that?
Can I close the issue?
FYI I write the PEP 490 as a "spin off" of this issue, to continue my work on the enhancement of exceptions in the C code: "PEP 490 - Chain exceptions at C level".

The second (exception == NULL) check in _Py_PrintFatalError() looks suspicious. When it is possible? And if it is possible, can it cause leaks?

Serhiy Storchaka added the comment:
> The second (exception == NULL) check in _Py_PrintFatalError() looks suspicious. When it is possible? And if it is possible, can it cause leaks?
Sorry, I have no idea. I didn't write this code myself. It comes from
PyErr_PrintEx():
 PyErr_Fetch(&exception, &v, &tb);
 if (exception == NULL)
 return;
 PyErr_NormalizeException(&exception, &v, &tb);
 if (tb == NULL) {
 tb = Py_None;
 Py_INCREF(tb);
 }
 PyException_SetTraceback(v, tb);
 if (exception == NULL)
 return;
I read again PyErr_NormalizeException(). I'm not sure that the case
can occur in practice. Maybe it can be replaced with an assertion?
Since Py_FatalError() is called in catastrophic cases, I chose to not
try to drop safety checks :-) You can modify PyErr_PrintEx() or even
_Py_PrintFatalError() if you feel more brave than me :-D

I close the issue.