RFC: Error Backtraces v2 by ericnorris · Pull Request #17056 · php/php-src · GitHub

This pull request seeks to continue the work from #5642, but using an implementation more in line with what nikic suggested in https://externals.io/message/110302#113264:

To put my feedback into more actionable form: Rather than adding an ini
setting that enables error backtraces for all diagnostics, I'd make it a
mask of error types for which a backtrace should be captured, and default
it to fatal errors only. So error_reporting=E_ALL,
error_backtrace=E_ERROR|E_CORE_ERROR|E_COMPILE_ERROR|E_USER_ERROR|E_RECOVERABLE_ERROR|E_PARSE.
It might be handy to expose the internal E_FATAL_ERRORS constant we have
for that.

I think this should give a very sensible default behavior, and people who
want to capture backtraces for all errors still have an option to do so.

Finally, I'm wondering if the backtrace in error_get_last() shouldn't be in
array form, rather than string form.

The RFC will be at https://wiki.php.net/rfc/error_backtraces_v2.

I've created a new INI setting, error_backtrace_recording, which users can set to an error mask to enable backtraces for those errors. It defaults to E_FATAL_ERRORS, which I've also exposed as a userland constant.

Considering the default, any fatal error will now have a backtrace associated with it. For example, a script timeout will now look like:

Fatal error: Maximum execution time of 1 second exceeded in example.php on line 23
Stack trace:
#0 example.php(23): usleep(10000)
#1 example.php(24): recurse()
#2 example.php(24): recurse()
...

This would have been useful to us very recently: we had an issue in production that presented as "Maximum execution time of X second exceeded" errors, and it was difficult for us to identity the underlying culprit. We eventually identified it as an infinite loop, which would have been incredibly obvious in a trace if it was available.

The implementation respects the zend.exception_ignore_args INI setting and the SensitiveParameter attributes, so users can ensure that sensitive arguments do not end up in the backtrace.

I have some open questions about this implementation:

Why is zend_startup responsible for setting EG(error_reporting), and not executor_globals_ctor? I've done the same for EG(error_backtrace_recording) purely for consistency.
Is there any appetite for modifying the signature of zend_error_cb? If so, I could pass the backtrace as a nullable argument instead of setting an executor global.

I've updated all tests that are now failing due to the stack trace in the second commit, to keep the diff reviewable.

Apologies for the additional notifications, there are understandably quite a lot of tests that trigger fatal errors.

I really don't like the amount of changed files. The default for test runner should be to not display it IMHO.

I really don't like the amount of changed files. The default for test runner should be to not display it IMHO.

It is unfortunate; I have a similar feature I was working on that changed error output and the amount of tests it involved touching was really annoying, both to update tests and to review.

Can you please add error_backtrace_recording=0 to $ini_overwrites in run-tests.php (and revert most of the phpt changes) - it's generally just noise in all tests.

Can you please add error_backtrace_recording=0 to $ini_overwrites in run-tests.php (and revert most of the phpt changes) - it's generally just noise in all tests.

I'll take that into consideration - I'm waiting on the RFC discussion and potential vote before making any further changes at this time. Please chime in there if you have any other thoughts on this proposal, I'd appreciate it!

That said, I've kept the changes to a separate commit, which should allow people to review the first commit in isolation without needing to go through the changed tests. I don't really feel strongly here, however, so if people either want this to default to off (which is a voting option), or just to default to off for tests (which we could decide in the implementation), I'm fine with making the change once we've moved further along in the discussion.

I'll add that I am also not in favour of changes to alllll the tests. I would also add, that the PHP project does not prefix their commit messages with feat: etc.

Not a huge fan of the code churn. IMO, the stack trace should be avoided for compile errors, where the trace is really just noise.

Apologies for bumping this PR again - I'm going to look into backporting this internally as I assume the code won't change at this point - but any chance this could get merged sometime this week?

Maybe @iluuu1994 or @dstogov still have comments, but I don't see problems anymore.

I don't like the RFC, but it's accepted and I don't see technical problems in the implementation.

Hey @dstogov, thanks for taking a look! If you don't like the RFC, would you mind elaborating on what about it you don't like, either privately at my email address, or here? I have some interest in thinking more about error handling and formatting, although I'm happy this RFC at least solves the initial problem I aimed to solve. I'll note that I shared some thoughts on changing how the error callback works in https://externals.io/message/126110#126121, but I didn't get any follow-up discussion to that.

I just merged the latest master and added NEWS/UPGRADING. Will merge once the CI passes.

Now merged. Don't forget to adjust the status in the Wiki.

Thanks @TimWolla, and done.

I appreciate everyone's feedback, and again, if you have additional thoughts on error handling, feel free to reach out to me.

Since phpGH-17056, the result may return the backtrace array.

Since GH-17056, the result may return the backtrace array. Closes GH-17641

This became visible after phpGH-17056 was merged, but technically the lack of setting the opline is also present on lower branches. We set the opline to mirror the SAVE_OPLINE() from ZEND_INIT_STATIC_METHOD_CALL().

This became visible after GH-17056 was merged, but technically the lack of setting the opline is also present on lower branches. We set the opline to mirror the SAVE_OPLINE() from ZEND_INIT_STATIC_METHOD_CALL(). Closes GH-17732.

php.ini not updated

This is a useful feature, but enabling it by default requires rewriting every PHPT file's output section. Since that would be a hellish diff to make and to review, I think the best option is unfortunately, another INI option. We can enable this for prod/dev recommended INIs, but make sure it's disabled for the test runner. This takes some inspiration from the discussion in phpGH-17056, which has similar problems to this PR.

ericnorris requested review from bukka and dstogov as code owners December 5, 2024 17:56

github-actions Bot added Category: Engine Extension: standard ABI break labels Dec 5, 2024

ericnorris changed the title Feat error backtrace v2 feat: error backtrace v2 Dec 5, 2024

ericnorris force-pushed the feat-error-backtrace-v2 branch from 1559069 to be75a57 Compare December 6, 2024 16:25

ericnorris requested review from Girgias, derickr and ndossche as code owners December 6, 2024 16:25

github-actions Bot added Extension: date Extension: opcache Extension: pcre Extension: session Extension: spl Extension: tokenizer Extension: xml SAPI: phpdbg labels Dec 6, 2024

Girgias reviewed Dec 6, 2024

View reviewed changes

Comment thread Zend/zend.c Outdated

TimWolla reviewed Dec 6, 2024

View reviewed changes

Comment thread Zend/zend.c Outdated

bukka reviewed Dec 8, 2024

View reviewed changes

Comment thread Zend/zend.c Outdated

derickr reviewed Dec 12, 2024

View reviewed changes

NattyNarwhal mentioned this pull request Dec 14, 2024

Aggressively use actual function parameters in php_verror #12276

Merged

iluuu1994 reviewed Dec 17, 2024

View reviewed changes

Comment thread main/main.c Outdated

Comment thread Zend/zend.c Outdated

rename fatal_error_backtrace -> last_fatal_error_backtrace

9f29e80

dstogov approved these changes Jan 28, 2025

View reviewed changes

TimWolla added 2 commits January 29, 2025 10:42

Merge branch 'master' into feat-error-backtrace-v2

126a785

NEWS/UPGRADING

a6c9063

TimWolla merged commit 0a14ab1 into php:master Jan 29, 2025

ericnorris deleted the feat-error-backtrace-v2 branch January 29, 2025 15:44

iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Jan 30, 2025

Fix return type of error_get_last()

8db4b85

Since phpGH-17056, the result may return the backtrace array.

iluuu1994 mentioned this pull request Jan 30, 2025

Fix return type of error_get_last() #17641

Closed

iluuu1994 added a commit that referenced this pull request Jan 31, 2025

Fix return type of error_get_last()

47b7779

Since GH-17056, the result may return the backtrace array. Closes GH-17641

DanielEScherzer mentioned this pull request Feb 3, 2025

[RFC] Add support for attributes on compile-time constants #16952

Merged

ndossche mentioned this pull request Feb 7, 2025

Fix GH-17727: JIT SEGV on OOM in dtor when creating backtrace #17732

Closed

jrfnl mentioned this pull request Oct 25, 2025

PHP 8.5 | NewIniDirectives: detect new fatal_error_backtraces ini directive (RFC) PHPCompatibility/PHPCompatibility#1946

Merged

arnaud-lb mentioned this pull request Jun 30, 2026

Zend: Fix stack overflow crash in recursive function #22524

Open

RFC: Error Backtraces v2 by ericnorris · Pull Request #17056 · php/php-src · GitHub

Uh oh!

RFC: Error Backtraces v2 - #17056

RFC: Error Backtraces v2#17056TimWolla merged 12 commits intophp:masterphp/php-src:masterfrom ericnorris:feat-error-backtrace-v2ericnorris/php-src:feat-error-backtrace-v2Copy head branch name to clipboard

Conversation

ericnorris commented Dec 5, 2024

Uh oh!

ericnorris commented Dec 6, 2024

Uh oh!

Uh oh!

Uh oh!

bukka commented Dec 8, 2024

Uh oh!

Uh oh!

NattyNarwhal commented Dec 11, 2024

Uh oh!

bwoebi commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericnorris commented Dec 11, 2024

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ericnorris commented Jan 27, 2025

Uh oh!

ndossche commented Jan 27, 2025

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

ericnorris commented Jan 28, 2025

Uh oh!

TimWolla commented Jan 29, 2025

Uh oh!

TimWolla commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericnorris commented Jan 29, 2025

Uh oh!

remicollet commented Jul 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

RFC: Error Backtraces v2#17056
TimWolla merged 12 commits into
php:masterfrom
ericnorris:feat-error-backtrace-v2

bwoebi commented Dec 11, 2024 •
edited
Loading

TimWolla commented Jan 29, 2025 •
edited
Loading