gh-127604: Don't rely on dprintf() for faulthandler C stacks#132800
gh-127604: Don't rely on dprintf() for faulthandler C stacks#132800ZeroIntensity wants to merge 5 commits intopython:mainfrom
dprintf() for faulthandler C stacks#132800Conversation
ZeroIntensity
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit dc39d9a 🤖 Results will be shown at: If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Did you forget fclose(). I think that you need also dup().
|
Should be fixed now. |
Python/fileutils.c
Outdated
| int res = vfprintf(handle, fmt, vargs); | ||
| va_end(vargs); | ||
| fclose(handle); | ||
| close(newfd); |
There was a problem hiding this comment.
You don't need close() here, because fclose() closes the file descriptor.
There was a problem hiding this comment.
Sorry, I'm not used to going between FILE * and int like this. Fixed it.
|
I'm not sure what to do about WASM lacking |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
close() is still used when fopen() fails.
On WASM, an alternative implementation can use vsnpritf() and a fixed-size buffer. Does WASM even support access to a call stack?
Also, some platforms can use dprintf() directly, instead of a fallback.
That's intentional. I'd like to lean away from |
|
I meant that removing the |
|
It's a deprecated alias on Windows, but I don't think they'll go removing it anytime soon. I'll add it back for completeness. |
|
It looks like there is a I guess we could add an internal-only |
|
@vstinner, do we need to use |
|
_Py_dup() is more complicated to ensure that the file descriptor is non-inheritable. I don't think that it's useful here. |
| || info[i].dli_fname[0] == '\0' | ||
| ) { | ||
| dprintf(fd, " Binary file '<unknown>' [%p]\n", array[i]); | ||
| _Py_fdprintf(fd, " Binary file '<unknown>' [%p]\n", array[i]); |
There was a problem hiding this comment.
It would be safer to not use printf family but use traceback.c functions to write directly into the fd.
There was a problem hiding this comment.
Yeah, but then we have to deal with stack limits. I'll probably end up using those regardless, though.
There was a problem hiding this comment.
I wrote #132854 to replace dprintf() with _Py_write_noraise().
There was a problem hiding this comment.
Yeah, but then we have to deal with stack limits
Which stack limits? traceback.c code is designed to use little stack memory and be async-signal safe.
There was a problem hiding this comment.
I was thinking of a sprintf that would be incrementally printed. Your solution works a lot better.
|
Closing in favor of #132854 |
4 participants
I thought it wasn't possible for a system to have glibc's
backtrace()but not glibc'sdprintf(). Apparently, I was wrong.This uses
fdopen()/vfprintf()instead, which should be portable across anything POSIX, and even Windows.faulthandler#127604