Issue 11393: Integrate faulthandler module into Python 3.3

AntoinePitrou

http://bugs.python.org/review/11393/diff/2216/5072 File Doc/library/faulthandler.rst (right): http://bugs.python.org/review/11393/diff/2216/5072#newcode5 Doc/library/faulthandler.rst:5: :synopsis: Dump the Python traceback. You need a "versionadded" ...

2 years, 2 months ago #1

victor.stinner_haypocalc.com

http://bugs.python.org/review/11393/diff/2216/5072 File Doc/library/faulthandler.rst (right): http://bugs.python.org/review/11393/diff/2216/5072#newcode5 Doc/library/faulthandler.rst:5: :synopsis: Dump the Python traceback. On 2011/03/26 00:07:28, Antoine ...

2 years, 2 months ago #2

http://bugs.python.org/review/11393/diff/2216/5072
File Doc/library/faulthandler.rst (right):

http://bugs.python.org/review/11393/diff/2216/5072#newcode5
Doc/library/faulthandler.rst:5: :synopsis: Dump  the Python traceback.
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> You need a "versionadded" directive somewhere.
> 

Done.

http://bugs.python.org/review/11393/diff/2216/5072#newcode94
Doc/library/faulthandler.rst:94: .. function:: register(signum, file=sys.stderr,
all_threads=False)
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Is the default argument for ``file`` computed when the handler is registered,
or
> when it is called?

It reads sys.stderr at each call.

> In other words, if I first call register() and then change sys.stderr to point
> to another file, will the handler use the original or the modified sys.stderr?

register() keeps a reference to sys.stderr seen at the last call. If you do
sys.stderr=open(...), register() will not be aware of this change. But if you
call register() again, it will use the new file.

register() installs a signal handler: the signal handler is unable to get
sys.stderr, it uses the reference stored by register(). Well, it doesn't use the
reference: it uses the file descriptor.

enable(), register(), dump_traceback_later() do the same: keep a reference to a
file and store the file descriptor number of this file.

http://bugs.python.org/review/11393/diff/2216/5072#newcode119
Doc/library/faulthandler.rst:119: Functions to test the fault handler
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Do we really want to make these functions a public API??
> 
> I would not document them and call them _sigsegv(), etc.
> (or perhaps even put them in _testcapi instead, since providing functions to
> crash Python in an otherwise useful module doesn't strike me as a good idea).

I added a "_" prefix to these functions. There are other ways to crash Python,
faulthandler is described as a debug module.

http://bugs.python.org/review/11393/diff/2216/5072#newcode148
Doc/library/faulthandler.rst:148: use ``version >> 8`` to get the major version,
and ``version & 255`` to get the
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Can't it be less cryptic? Say, a tuple of integers? Or a string? Also, you
> should use `__version__` for such purposes.
> 

Done: faulthandler.__version__ is now (1, 6).

http://bugs.python.org/review/11393/diff/2216/5073
File Doc/using/cmdline.rst (right):

http://bugs.python.org/review/11393/diff/2216/5073#newcode505
Doc/using/cmdline.rst:505: This is equivalent to :option:`-X` ``faulthandler=1``
option.
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> What is the syntax? PYTHONFAULTHANDLER=yes? Any value?
> 

I tried to do something complex: I wanted to prepare the code to support -X
faulthandler=no and PYTHONFAULTHANDLER=no later. But you make me realize that it
is too much complex, so I simplied the doc. The code was already correct:
PYTHONFAULTHANDLER value is ignored, the variable just have to be present.

If we choose to enable faulthandler by default later, we can add -X
nofaulthandler and PYTHONNOFAULTHANDLER to *disable* the fault handler at
startup.

http://bugs.python.org/review/11393/diff/2216/5074
File Include/traceback.h (right):

http://bugs.python.org/review/11393/diff/2216/5074#newcode36
Include/traceback.h:36: PyAPI_DATA(const char*) _Py_DumpTracebackThreads(
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Can you add a comment saying what the return value ("const char *")
represents?
> 

It is already explained in traceback.c. As said on IRC, I moved the doc from
traceback.c to traceback.h.

http://bugs.python.org/review/11393/diff/2216/5074#newcode36
Include/traceback.h:36: PyAPI_DATA(const char*) _Py_DumpTracebackThreads(
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Can you add a comment saying what the return value ("const char *")
represents?
> 

It is already explained in traceback.c. As said on IRC, I moved the doc from
traceback.c to traceback.h.

http://bugs.python.org/review/11393/diff/2216/5075
File Lib/test/test_faulthandler.py (right):

http://bugs.python.org/review/11393/diff/2216/5075#newcode2
Lib/test/test_faulthandler.py:2: import faulthandler
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Is faulthandler guaranteed to compile on all platforms?

I hope so. If it is doesn't compile, the module have to be fixed.

http://bugs.python.org/review/11393/diff/2216/5075#newcode17
Lib/test/test_faulthandler.py:17: skipIf = unittest.skipIf
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> It would be better not to put any compatibility code in the stdlib, IMO.

Oh, I forgot to remove this code. Fixed.

http://bugs.python.org/review/11393/diff/2216/5075#newcode56
Lib/test/test_faulthandler.py:56: os.unlink(filename)
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> support.unlink() might be better.

Done.

http://bugs.python.org/review/11393/diff/2216/5075#newcode61
Lib/test/test_faulthandler.py:61: def setUp(self):
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> No tearDown() method?

I added one to restore the state of the fault handler (enabled / disabled).

http://bugs.python.org/review/11393/diff/2216/5075#newcode74
Lib/test/test_faulthandler.py:74: process = subprocess.Popen(
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Why don't you use test.script_helper instead? (either assert_python_ok or
> assert_python_failure)
> We don't want to duplicate such code everywhere.
> 

Done.

http://bugs.python.org/review/11393/diff/2216/5075#newcode83
Lib/test/test_faulthandler.py:83: output = re.sub(r"\[\d+ refs\]\r?\n?$", "",
output)
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> See strip_python_stderr() in test.support.
> 

Done. I didn't use it because I wrote my module outside Python code base. But
now it can be simplified and adopt Python standards (for tests).

http://bugs.python.org/review/11393/diff/2216/5075#newcode243
Lib/test/test_faulthandler.py:243: 'from __future__ import with_statement',
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Ouch. Such way of embedding Python code makes it harder to maintain and edit.
> You should pass it as a string instead:

Did you notice the '... %r' % filename? I don't like """ because I want to
control the indentation of the code. And I also prefer to ensure that the
newline is \n and not the OS newline.

http://bugs.python.org/review/11393/diff/2216/5075#newcode443
Lib/test/test_faulthandler.py:443: faulthandler.unregister(signal.SIGUSR1)
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> You should use some "try .. finally" blocks here, I think.

This function changes completly the state of the faulthandler module. If we use
the faulthandler in regrtest, it will be broken. Because there are no more
refleak, we may disable this test with a comment: "re-enable this test temporary
and run it using regtest.py -R 3:3:".

The state of register()/unregister() cannot be restored because it cannot be
read. Same issue for dump_traceback_late().

http://bugs.python.org/review/11393/diff/2216/5078
File Modules/faulthandler.c (right):

http://bugs.python.org/review/11393/diff/2216/5078#newcode19
Modules/faulthandler.c:19: #define PUTS(fd, str) write(fd, str, strlen(str))
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Is it guaranteed that str is always 0-terminated? After a segfault, the memory
> could be messed up.

Most calls get a constant string, except PUTS(fd, handler->name). I hope that
handler->name is 0-terminated.

On a segfault, we cannot trust anything. In the worst case, the fault handler
causes a new segmentation fault and the program exits without printing the
Python traceback. Is it necessary to write a complex function just for this
case? In practice, strlen() just works here.

http://bugs.python.org/review/11393/diff/2216/5078#newcode414
Modules/faulthandler.c:414: previous = signal(SIGALRM, faulthandler_alarm);
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> I think using alarm() is a bad idea, since Python code can use alarm() itself
> and register its own SIGALRM handler. Our test suite does, so this function
will
> be unusable on the buildbots.
> 
> I'm not sure how you could do that instead: perhaps a thread that simply
sleeps
> the desired time? 

faulthandler document explains that dump_traceback_later() is implemented using
alarm()+SIGALRM. So if regrest.py uses SIGALRM too, it doesn't work.

Which function/test does use SIGALRM? I already hacked regrtest.py once to dump
the traceback, using dump_traceback_later(), if a test takes more than 5
minutes: it just worked.

I don't like threads because you can have deadlocks, especially with Python
threads and the GIL. Signals are more low-level and can be used to debug blocked
threads.

But if you are able to hack the module to use a sleeping thread, I would be
interested to test it. It may help on Windows, because Windows doesn't have
alarm().

http://bugs.python.org/review/11393/diff/2216/5078#newcode595
Modules/faulthandler.c:595: Py_DECREF(user->file);
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Wouldn't Py_CLEAR() be better?

I don't think so: user->file is reassigned two lines below.

http://bugs.python.org/review/11393/diff/2216/5078#newcode613
Modules/faulthandler.c:613: Py_DECREF(user->file);
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Wouldn't Py_CLEAR() be better?

Is it useless because the entry in the user_signals array is no more used. But I
patched the code to use Py_CLEAR() because it cannot hurt.

http://bugs.python.org/review/11393/diff/2216/5078#newcode668
Modules/faulthandler.c:668: int x = 1, y = 0, z;
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> If you really want a floating-point exception, wouldn't it be better to use
> doubles? Or doesn't it make a difference?

Yes, the signal is called "Floating point exception" (FPE), but it is also used
for division by zero. It is easier to test a division by zero than test IEEE 754
exceptions depending on some FPU flags and maybe other things.

http://bugs.python.org/review/11393/diff/2216/5078#newcode677
Modules/faulthandler.c:677: while(1)
On 2011/03/26 00:07:28, Antoine Pitrou wrote:
> Why while(1)?

In an application, SIGBUS is raised by the kernel, not by the application
itself. But here we kill ourself. The problem is that the fault handler restores
the previous signal handler and then just return instead of calling directly the
previous signal handler. This behaviour is documented in
faulthandler_fatal_error() comment:

(...) Display the current Python traceback and restore the previous handler. The
previous handler will be called when the fault handler exits, because the fault
will occur again. (...)

Without while(1), the function does exit and the execution continues normally!

I don't know a portable way to call the previous signal handler, because the
signal handler prototype may be different depending on how it was installed
(e.g. with or without the signal info argument).

--

I added a comment to explain the "trick".

Sign in to reply to this message.

AntoinePitrou

http://bugs.python.org/review/11393/diff/2216/5078 File Modules/faulthandler.c (right): http://bugs.python.org/review/11393/diff/2216/5078#newcode414 Modules/faulthandler.c:414: previous = signal(SIGALRM, faulthandler_alarm); > Which function/test does use ...

2 years, 2 months ago #3

Only a test for meta-tracker issue #382.

Sign in to reply to this message.

#11393: Integrate faulthandler module into Python 3.3

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Patch Set 7 #

Patch Set 8 #

Patch Set 9 #

Messages