Hi folks
The attached patch series adds support for detecting coding errors where a stack-allocated ErrorContextCallback is not popped from the error_context_stack before the variable leaves scope.
With the attached patches this code will emit a backtrace and abort() on cassert builds at the "return" statement where it leaks the stack pointer:
ErrorContextCallback cb pg_errcontext_check();
...
cb.previous = error_context_stack;
error_context_stack = &cb;
...
if (something)
return; /* whoops! */
...
error_context_stack = error_context_stack->previous;
Note the pgl_errcontext_check() in the above, which enables the check.
The infrastructure added by the patch could well be useful for other similar validation steps. In fact, I've added a similar validator that detects escapes from PG_TRY() blocks, so you get a neat assertion failure at the point of escape instead of a crash at a __longjmp() with a nonsensical stack. Example using a deliberately introduced mistake in postgres_fdw as a test:
Without the PG_TRY() patch:
#0 __longjmp () at ../sysdeps/x86_64/__longjmp.S:111
#1 0xc35a5d9e1a902cfc in ?? ()
Backtrace stopped: Cannot access memory at address 0xc0ea5d9e1a902cfc
With the PG_TRY() patch (abbreviated):
#3 0x0000000000abbd4b in pg_try_check_return_guard
#4 0x00007f8e2e20e41a in fetch_more_data
#5 0x00007f8e2e20a80a in postgresIterateForeignScan
...
Note that valgrind's memcheck, gcc's -fstack-protector, etc are all quite bad at detecting this class of error, so being able to do so automatically should be nice. I've had an interesting time chasing down error context stack mistakes a few times in the past
We unfortunately cannot use it for RAII-style coding because the underlying __attribute__((callback(..))) is not available on MSVC. As usual. But it remains useful for checking and assertions.
The patch is supplied as a small series:
(1) Add some documentation on error context callback usage
(2) Add pg_errcontext_check() to detect ErrorContextCallback escapes
(3) Enable pg_errcontext_check() for all in-tree users
(4) Document that generally PG_CATCH() should PG_RE_THROW()
(5) Assert() if returning from inside a PG_TRY() / PG_CATCH() block
(6) [RFC] Add a more succinct API for error context stack callbacks
(7) [RFC] Use __has_attribute for compiler attribute detection where possible
I also added a patch (6) on top as a RFC of sorts, which adds a more succinct API for error context setup and teardown. It's not required for these changes, but it might make sense to adopt it at the same time if we're changing usage across the tree anyway.
A further patch (7) switches over to using __has_attribute instead of explicit compiler version checks. Again, optional, more of a RFC.
