Teach MSVC that elog/ereport ERROR doesn't return

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

diff --git a/src/include/c.h b/src/include/c.h
index 5847ecda8396119e1a20cfb581749031161967f0..7fe083c3afb8660c40f85b46566580db9067a8e4 100644 (file)
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -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

diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 675f4f5f4694dd395bd3d3c016e0a20fc762fe3d..b4945eb7ee0fc301107db48f83235f6bdcb92696 100644 (file)
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -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__)