database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-10-16 17:07:43 +03:00
Code Activity

Teach MSVC that elog/ereport ERROR doesn't return

Browse Source 
It had always been intended that this already works correctly as
pg_unreachable() uses __assume(0) on MSVC, and that directs the compiler
in a way so it knows that a given function won't return.  However, with
ereport_domain(), it didn't work...

It's now understood that the failure to determine that elog(ERROR) does not
return comes from the inability of the MSVC compiler to detect the "const
int elevel_" is the same as the "elevel" macro parameter.  MSVC seems to be
unable to make the "if (elevel_ >= ERROR) branch constantly-true when the
macro is used with any elevel >= ERROR, therefore the pg_unreachable() is
seen to only be present in a *conditional* branch rather than present
*unconditionally*.

While there seems to be no way to force the compiler into knowing that
elevel_ is equal to elevel within the ereport_domain() macro, there is a
way in C11 to determine if the elevel parameter is a compile-time
constant or not.  This is done via some hackery using the _Generic()
intrinsic function, which gives us functionality similar to GCC's
__builtin_constant_p(), albeit only for integers.

Here we define pg_builtin_integer_constant_p() for this purpose.
Callers can check for availability via HAVE_PG_BUILTIN_INTEGER_CONSTANT_P.
ereport_domain() has been adjusted to use
pg_builtin_integer_constant_p() instead of __builtin_constant_p().

It's not quite clear at this stage if this now allows us to forego doing
the likes of "return NULL; /* keep compiler quiet */" as there may be other
compilers in use that have similar struggles.  It's just a matter of time
before someone commits a function that does not "return" a value after
an elog(ERROR).  Let's make time and lack of complaints about said commit
be the judge of if we need to continue the "/* keep compiler quiet */"
palaver.

Author: David Rowley <drowleyml@gmail.com>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://postgr.es/m/CAApHDvom02B_XNVSkvxznVUyZbjGAR+5myA89ZcbEd3=PA9UcA@mail.gmail.com
This commit is contained in:
David Rowley
2025-09-27 22:41:04 +12:00
parent 66cdef4425
commit 59c2f03d1e
2 changed files with 45 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
src/include/c.h
View File

@@ -331,6 +331,33 @@
#define pg_unreachable() abort()
#endif
/*
* Define a compiler-independent macro for determining if an expression is a
* compile-time integer const. We don't define this macro to return 0 when
* unsupported due to the risk of users of the macro misbehaving if we return
* 0 when the expression *is* an integer constant. Callers may check if this
* macro is defined by checking if HAVE_PG_BUILTIN_INTEGER_CONSTANT_P is
* defined.
*/
#if defined(HAVE__BUILTIN_CONSTANT_P)
/* When __builtin_const_p() is available, use it. */
#define pg_builtin_integer_constant_p(x) __builtin_constant_p(x)
#define HAVE_PG_BUILTIN_INTEGER_CONSTANT_P
#elif defined(_MSC_VER) && defined(__STDC_VERSION__)
/*
* With MSVC we can use a trick with _Generic to make this work. This has
* been borrowed from:
* https://stackoverflow.com/questions/49480442/detecting-integer-constant-expressions-in-macros
* and only works with integer constants. Compilation will fail if given a
* constant or variable of any type other than an integer.
*/
#define pg_builtin_integer_constant_p(x) \
_Generic((1 ? ((void *) ((x) * (uintptr_t) 0)) : &(int) {1}), int *: 1, void *: 0)
#define HAVE_PG_BUILTIN_INTEGER_CONSTANT_P
#endif
/*
* pg_assume(expr) states that we assume `expr` to evaluate to true. In assert
* enabled builds pg_assume() is turned into an assertion, in optimized builds

35
src/include/utils/elog.h
View File

@@ -119,36 +119,37 @@ struct Node;
* ereport_domain() directly, or preferably they can override the TEXTDOMAIN
* macro.
*
* When __builtin_constant_p is available and elevel >= ERROR we make a call
* to errstart_cold() instead of errstart(). This version of the function is
* marked with pg_attribute_cold which will coax supporting compilers into
* generating code which is more optimized towards non-ERROR cases. Because
* we use __builtin_constant_p() in the condition, when elevel is not a
* compile-time constant, or if it is, but it's < ERROR, the compiler has no
* need to generate any code for this branch. It can simply call errstart()
* unconditionally.
* When pg_builtin_integer_constant_p is available and elevel >= ERROR we make
* a call to errstart_cold() instead of errstart(). This version of the
* function is marked with pg_attribute_cold which will coax supporting
* compilers into generating code which is more optimized towards non-ERROR
* cases. Because we use pg_builtin_integer_constant_p() in the condition,
* when elevel is not a compile-time constant, or if it is, but it's < ERROR,
* the compiler has no need to generate any code for this branch. It can
* simply call errstart() unconditionally.
*
* If elevel >= ERROR, the call will not return; we try to inform the compiler
* of that via pg_unreachable(). However, no useful optimization effect is
* obtained unless the compiler sees elevel as a compile-time constant, else
* we're just adding code bloat. So, if __builtin_constant_p is available,
* use that to cause the second if() to vanish completely for non-constant
* cases. We avoid using a local variable because it's not necessary and
* prevents gcc from making the unreachability deduction at optlevel -O0.
* we're just adding code bloat. So, if pg_builtin_integer_constant_p is
* available, use that to cause the second if() to vanish completely for
* non-constant cases. We avoid using a local variable because it's not
* necessary and prevents gcc from making the unreachability deduction at
* optlevel -O0.
*----------
*/
#ifdef HAVE__BUILTIN_CONSTANT_P
#ifdef HAVE_PG_BUILTIN_INTEGER_CONSTANT_P
#define ereport_domain(elevel, domain, ...) \
do { \
pg_prevent_errno_in_scope(); \
if (__builtin_constant_p(elevel) && (elevel) >= ERROR ? \
if (pg_builtin_integer_constant_p(elevel) && (elevel) >= ERROR ? \
errstart_cold(elevel, domain) : \
errstart(elevel, domain)) \
__VA_ARGS__, errfinish(__FILE__, __LINE__, __func__); \
if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
if (pg_builtin_integer_constant_p(elevel) && (elevel) >= ERROR) \
pg_unreachable(); \
} while(0)
#else /* !HAVE__BUILTIN_CONSTANT_P */
#else /* !HAVE_PG_BUILTIN_INTEGER_CONSTANT_P */
#define ereport_domain(elevel, domain, ...) \
do { \
const int elevel_ = (elevel); \
@@ -158,7 +159,7 @@ struct Node;
if (elevel_ >= ERROR) \
pg_unreachable(); \
} while(0)
#endif /* HAVE__BUILTIN_CONSTANT_P */
#endif /* HAVE_PG_BUILTIN_INTEGER_CONSTANT_P */
#define ereport(elevel, ...) \
ereport_domain(elevel, TEXTDOMAIN, __VA_ARGS__)