This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: infinite loop in faulthandler._stack_overflow
Type: behavior Stage:
Components: Extension Modules Versions: Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: WanderingLogic, python-dev, vstinner
Priority: normal Keywords: patch

Created on 2015-03-12 22:51 by WanderingLogic, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
faulthandler-infloop.patch WanderingLogic, 2015-03-12 22:51 Patch for the infinite loop in faulthandler._stack_overflow() review
volatile.patch vstinner, 2015-03-13 10:27 review
icc-stackoverflow.patch WanderingLogic, 2015-03-23 18:38
Messages (7)
msg237993 - (view) Author: Matt Frank (WanderingLogic) * Date: 2015-03-12 22:51
When the faulthandler module is compiled at -O3 (the default for non-debug builds) with a compiler that does tailcall optimization the Modules/faulthandler.c:stack_overflow() function may become an infinite loop that does not expand the stack.  This puts the interpreter into an infinite loop with 100% CPU utilization that won't respond to SIGINT.  (But sending SIGTERM will get it to exit.)

The Intel compiler (15.0.1 on Linux) seems to be able to prove this optimization "safe".  (And thus cause the function to turn into an infinite loop.)  While gcc 4.8.2 and clang 3.4.2 do not currently optimize this call (because their pointer analysis isn't quite strong enough to deal with the pointer to the depth argument), there's no guarantee that they won't be able to optimize it in their next versions.

This patch adds a test between the recursive call and the return statement which makes it not a tail call.
msg238020 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-13 10:27
> When the faulthandler module is compiled at -O3 (the default for non-debug builds) with a compiler that does tailcall optimization ...

test_faulthandler pass with GCC 4.9.2 (Fedora 21) when faulthandler.c is compiled in -O3 mode. I guess that such config doesn't support tailcall optimization.

I guess that this issue is currently specific to Intel C compiler (ICC), right?

> ... with a compiler that does tailcall optimization the Modules/faulthandler.c:stack_overflow() function may become an infinite loop that does not expand the stack.

Hum, I didn't expect that I would be so hard to write a reliable stack overflow :-) I modified recently the function in issue #23433 to a different issue with void*/intptr_t conversion.

My function writes into a buffer allocated on the stack. How the compiler avoids to allocate 4 KB for each loop iteration? It doesn't matter if the function is recursive or not, it only matters if each iteration allocates 4 KB on the stack.

I'm writing into the buffer allocated on the stack to ensure that the buffer is really allocated. Maybe the pattern is too reliable?

Can you try to mark the buffer with the volatile keyword? (See attached patch.) This keyword is already used in faulthandler_read_null() and faulthandler_sigfpe() to workaround issues with compiler optimizations. (See attached patch.)
msg238045 - (view) Author: Matt Frank (WanderingLogic) * Date: 2015-03-13 18:09
Yes, this is currently only a problem with the Intel compiler.

The writes to buffer[] are dead (provably won't be ever used) at the point that the recursive call occurs.  Actually gcc and llvm can figure this out.  Thus all the space allocated for the first call can be reused by every subsequent call.

The analysis that icc is doing that gcc and clang are not yet doing has to do with the pointer to depth.  I believe icc is able to determine that depth doesn't point into buffer where gcc and clang aren't sure that this is the case.  (If you change stack_overflow() so that depth is passed in and returned by value then gcc also does the tail optimization.)

volatile doesn't disable the optimization.  I think in this case it is too easy to determine that buffer is on the stack, and both buffer and sp are dead by the time of the recursive call.

It occurs to me that the right way to deal with this is with

__attribute__ ((optimize ("no-optimize-sibling-calls")))

on the function where it matters (stack_overflow()).

I'm working on a solution (need to test whether the compiler supports attributes and etc.) and will post it ASAP.
msg238447 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-18 14:16
> It occurs to me that the right way to deal with this is with
> __attribute__ ((optimize ("no-optimize-sibling-calls")))
> on the function where it matters (stack_overflow()).

Can you please write a patch for that with the right #ifdef to only add it to ICC?
msg239049 - (view) Author: Matt Frank (WanderingLogic) * Date: 2015-03-23 18:38
This is a patch that turns off the Intel Compiler's optimization for the stack_overflow() function.  It turns out that icc doesn't support gcc's __attribute__((optimize("no-optimize-sibling-calls"))).  Instead I used an ifdef'd intel-specific pragma that turns off optimization completely for just this function, and just for the intel compiler.

This particular pragma has the benefit that it should also work with the Intel compiler on Windows without needing further ifdefs.
msg239059 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-03-23 20:21
New changeset d6003de8ecc8 by Victor Stinner in branch '3.4':
Issue #23654: Fix faulthandler._stack_overflow() for the Intel C Compiler (ICC)
https://hg.python.org/cpython/rev/d6003de8ecc8
msg239060 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-23 20:22
Thanks for your contribution Matt!
History
Date User Action Args
2022-04-11 14:58:13adminsetgithub: 67842
2015-03-23 20:22:12vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg239060

versions: - Python 3.6
2015-03-23 20:21:53python-devsetnosy: + python-dev
messages: + msg239059
2015-03-23 18:38:06WanderingLogicsetfiles: + icc-stackoverflow.patch

messages: + msg239049
2015-03-18 14:16:24vstinnersetmessages: + msg238447
2015-03-13 18:09:09WanderingLogicsetmessages: + msg238045
2015-03-13 10:27:59vstinnersetfiles: + volatile.patch

messages: + msg238020
2015-03-12 23:02:30berker.peksagsetnosy: + vstinner
2015-03-12 22:51:27WanderingLogiccreate