RFC: Error Backtraces v2

[ericnorris]

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.

[ericnorris]

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

[bukka]

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

[NattyNarwhal]

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.

[bwoebi]

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.

[ericnorris]

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.

[derickr]

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.

[iluuu1994]

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.

[ericnorris]

Anyone have an idea as to why bug73037.phpt is failing in CI? It's not failing on my arm64 Macbook.

[nielsdos]

@ericnorris If I had to take a guess, it's a crash somewhere in the server. I'll make a checkout of your branch and see what's going on.

[nielsdos]

@ericnorris I can't reproduce this, but note that you don't initialize error_backtrace on NTS, executor_globals_ctor is only executed on ZTS and that's the only place where error_backtrace gets initialized. So I think that's the problem.
Please try the following:

From c571e797e3f974315f1514cda3fed50cda2a0b70 Mon Sep 17 00:00:00 2001
From: Niels Dossche <[email protected]>
Date: Wed, 8 Jan 2025 21:28:30 +0100
Subject: [PATCH] Move initialization of error_backtrace

---
 Zend/zend.c             | 1 -
 Zend/zend_execute_API.c | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Zend/zend.c b/Zend/zend.c
index 9c8ab08a0eb..a85336f2515 100644
--- a/Zend/zend.c
+++ b/Zend/zend.c
@@ -812,7 +812,6 @@ static void executor_globals_ctor(zend_executor_globals *executor_globals) /* {{
 	executor_globals->in_autoload = NULL;
 	executor_globals->current_execute_data = NULL;
 	executor_globals->current_module = NULL;
-	ZVAL_UNDEF(&executor_globals->error_backtrace);
 	executor_globals->exit_status = 0;
 #if XPFPA_HAVE_CW
 	executor_globals->saved_fpu_cw = 0;
diff --git a/Zend/zend_execute_API.c b/Zend/zend_execute_API.c
index 6f4afa77341..bf96d5e58d2 100644
--- a/Zend/zend_execute_API.c
+++ b/Zend/zend_execute_API.c
@@ -140,6 +140,8 @@ void init_executor(void) /* {{{ */
 	original_sigsegv_handler = signal(SIGSEGV, zend_handle_sigsegv);
 #endif
 
+	ZVAL_UNDEF(&EG(error_backtrace));
+
 	EG(symtable_cache_ptr) = EG(symtable_cache);
 	EG(symtable_cache_limit) = EG(symtable_cache) + SYMTABLE_CACHE_SIZE;
 	EG(no_extensions) = 0;
-- 
2.47.1

[ericnorris]

Nice catch @nielsdos, that did in fact appear to be the issue. It wasn't super clear to me where the various initialization bits should go (see the "Why is zend_startup responsible for setting..." part of my PR message), and it seems like I was off the mark. Thank you!

[nielsdos]

executor_globals_ctor basically decouples the globals from the startup thread where necessary and initializes the thread local variants.
fatal_error_backtraces does not need explicit initialization because it's handled by the ini handler.

[ericnorris]

I hope folks don't mind a friendly nudge here - would someone mind taking a final look when they can and letting me know if there are any necessary changes, or otherwise merging this? I'd like to backport this to our internal PHP package so we can start using it, but I'd prefer to wait until it's merged.

[nielsdos]

I don't really see significant problems since I last looked at this. Please take a look at my comment.

[nielsdos]

This is supposed to be readable in HTML, but I doubt the backtrace will be displayed in a readable manner. Perhaps, this should be wrapped in a <pre> tag or smth alike?

[ericnorris]

Good catch, I'll look into that.

Semi-related tangent so feel free to disregard: Is it on anyone's radar to refactor this function at all? I saw "Deprecate the error_prepend_string and error_append_string INI directives" in https://wiki.php.net/rfc/deprecations_php_8_5, which would help make this a little more readable, but I wonder if there's more we could do simplify this logic. I acknowledge that is probably easier said than done, however.

[nielsdos]

I doubt refactoring this function is on someone's TODO list. There are indeed some outdated pieces (e.g. the ones you mentioned but also xmlrpc_errors imo). However, the question with refactoring is always how much it's worth spending time on this. This function likely is not on any hot code path and refactoring also brings in a risk of breaking things 🤷

[bukka]

would be also nice to break up those huge long lines when you are in it...

[ericnorris]

@nielsdos apologies, but what's the best way to wrap a zend_string in <pre>? strpprintf? Or would it be the worst thing to just change this to %s%s%s and then pass in ZSTR_LEN(backtrace) ? "<pre>Stack trace:\n" : "", ZSTR_VAL(backtrace), ZSTR_LEN(backtrace) ? "</pre>" : ""? We could also drop adding the pre at all since we already have some fatal errors with stack traces that do not have pre: https://github.com/search?q=repo%3Aphp%2Fphp-src+%22%3Cb%3EFatal+error%3C%2Fb%3E%22&type=code, e.g.

<br />
<b>Fatal error</b>:  Uncaught Error: Cannot access private property test::$pri in %s004.test.php:8
Stack trace:
#0 {main}
  thrown in <b>%s004.test.php</b> on line <b>8</b><br />

@bukka I received feedback elsewhere to not break up long lines, though maybe here is a good counterpoint. I'd personally like to err on keeping it as-is since I'd like to be able to merge this relatively soon, but if people feel strongly I could break this up. This is kind of what I was getting at with my earlier comment - this function could probably be simplified to make it more readable, besides just the long line length.

[nielsdos]

If we wrap it with pre it should be done consistently.
If I were to write the code, I would go for the ZSTR_LEN() ternary trick.

[ericnorris]

If we wrap it with pre it should be done consistently.

I'm not sure I follow - what I was saying was that we currently do not wrap it with <pre> (for errors that already have backtraces before my change), so if we wrap it with <pre> here we'd be inconsistent with existing errors like:

php-src/sapi/cgi/tests/004.phpt

Lines 43 to 46 in 72708f2

	<b>Fatal error</b>: Uncaught Error: Cannot access private property test::$pri in %s004.test.php:8
	Stack trace:
	#0 {main}
	thrown in <b>%s004.test.php</b> on line <b>8</b><br />

If I were to write the code, I would go for the ZSTR_LEN() ternary trick.

Thanks, I was leaning that way but I wanted your opinion. I'm still not convinced we should add the <pre> based on the above, but if you think we should I'll go the ZSTR_LEN route.

[TimWolla]

I received feedback elsewhere to not break up long lines,

In case you are referring to my remark here: #16937 (comment) - this case of a printf-style function is quite different and splitting it across multiple lines would be reasonable, because it accurately represents the complexity of the statement.

[nielsdos]

Ok let's not do the <pre> conversion.

[ericnorris]

@TimWolla I was! and when I said "maybe here is a good counterpoint", I meant that I could see that you might agree with splitting this up. My preference was to not do this myself though, since for consistency I should probably do that for all of them, and I was concerned with making it a topic for bikeshedding.

[nielsdos]

barring any coding style changes, this looks good to me now.

[ericnorris]

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?

[nielsdos]

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

[dstogov]

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

[ericnorris]

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.

[TimWolla]

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

[TimWolla]

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

[ericnorris]

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.