public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Tom Lane <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: [email protected]
Subject: Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p
Date: Mon, 21 Jul 2025 12:39:36 +1200
Message-ID: <CAApHDvqH_gsb4UwHAQGy9w_oJYyB8y-esQbHNUoGsB668RZ9gQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CAApHDvqrhFfnetbcwgGkJ=z63T8HfQ_OyP=vX8BYiXyxFKt67w@mail.gmail.com>
<[email protected]>
<CAApHDvqf4tKwE0oP8ybL2Eu-SD=C34pdZ9vVDSJwvqDXi88nUg@mail.gmail.com>
<[email protected]>
<CAApHDvrFdXjbrV6KCx_GHKYSufUbNDYSsjppcJQiGOURfJE6qg@mail.gmail.com>
<[email protected]>
<CAApHDvoOnd4PBNV0qyJVLmbWvWUjztzcMH5xY2AGp5Vov6XU3Q@mail.gmail.com>
<[email protected]>
<CAApHDvp-WCUTDU767Lpjw3m698gS+mCrNAnTEppZiXixneVxGQ@mail.gmail.com>
<[email protected]>
On Thu, 17 Jul 2025 at 16:20, Tom Lane <[email protected]> wrote:
>
> David Rowley <[email protected]> writes:
> > Unsure what other repercussions there are of compiling with C11 and
> > have not looked into what it would take to adjust the meson build
> > scripts to default to that for MSVC builds.
>
> In theory there should be no repercussions, because we already
> support compiling with C11: C99 is a minimum not a maximum.
Oh, right.
> But I've not checked the meson scripts either.
The attached seems to work.
It's my first time hacking on meson build scripts. I was surprised to
see the "-std=c99" test just does not work for MSVC since the flag is
spelt "/std:c99" for that compiler. The reason it's not causing
trouble is due to the code compiling without that flag, so the test
that passes the flag is never executed. For the test I added, I had
to use the MSVC naming convention, which seems fine as it's only run
when "cc.get_id() == 'msvc'".
I've not gone over the other users of __builtin_constant_p() to
replace them with the pg_builtin_constant(). That's probably a good
exercise to do as I'm not sure how well the MSVC _Generic trick works
or if there might be some expressions it doesn't work with.
I also opted not to have the pg_builtin_constant() macro return 0 when
it's unsupported as I was worred some code might make too many
assumptions about the variability of the expression if that were to
return false. I don't quite have a problem case in mind for that.
Doing it that way was mostly erring on the side of caution.
For now, I've left in a commented out "return NULL;". I get no
compiler warnings from that.
David
Attachments:
[application/octet-stream] v2-0001-Detect-elog-ERROR-can-t-return-in-MSVC-when-using.patch (4.5K, 2-v2-0001-Detect-elog-ERROR-can-t-return-in-MSVC-when-using.patch)
download | inline diff:
From 26442d786cc65211f0176e803dfa269e9e93c8bd Mon Sep 17 00:00:00 2001
From: David Rowley <[email protected]>
Date: Thu, 17 Jul 2025 15:49:03 +1200
Subject: [PATCH v2] Detect elog(ERROR) can't return in MSVC when using C11
---
meson.build | 24 ++++++++++++++++++++++++
src/backend/backup/basebackup_target.c | 4 ++--
src/include/c.h | 17 +++++++++++++++++
src/include/utils/elog.h | 6 +++---
4 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/meson.build b/meson.build
index 5365aaf95e6..e23a99c7b1d 100644
--- a/meson.build
+++ b/meson.build
@@ -1704,6 +1704,18 @@ endif
# Compiler tests
###############################################################
+# Test to see if the compiler supports C11
+c11_test = '''
+#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
+#error "C11 not supported"
+#endif
+
+int main(int argc, char **argv)
+{
+ return 0;
+}
+'''
+
# Do we need -std=c99 to compile C99 code? We don't want to add -std=c99
# unnecessarily, because we optionally rely on newer features.
c99_test = '''
@@ -1737,6 +1749,18 @@ int main(int argc, char **argv)
}
'''
+# The current minimum version of MSVC we currently support supports at least
+# C11, so lets try to compile with C11. Check if we need to actually pass the
+# /std:c11 flag for that to work. If this test ends up adding the C11 flag,
+# then the C99 test below should always pass and never add the C99 flag.
+if cc.get_id() == 'msvc' and not cc.compiles(c11_test, name: 'c11', args: test_c_args)
+ if cc.compiles(c11_test, name: 'c11 with /std:c11',
+ args: test_c_args + ['/std:c11'])
+ test_c_args += '/std:c11'
+ cflags += '/std:c11'
+ endif
+endif
+
if not cc.compiles(c99_test, name: 'c99', args: test_c_args)
if cc.compiles(c99_test, name: 'c99 with -std=c99',
args: test_c_args + ['-std=c99'])
diff --git a/src/backend/backup/basebackup_target.c b/src/backend/backup/basebackup_target.c
index 84b1309d3bd..01705368950 100644
--- a/src/backend/backup/basebackup_target.c
+++ b/src/backend/backup/basebackup_target.c
@@ -145,8 +145,8 @@ BaseBackupGetTargetHandle(char *target, char *target_detail)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("unrecognized target: \"%s\"", target)));
- /* keep compiler quiet */
- return NULL;
+ /* comment out for testing. Not for commit. */
+ /* return NULL; */
}
/*
diff --git a/src/include/c.h b/src/include/c.h
index 6d4495bdd9f..48a8cedec2d 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -332,6 +332,23 @@
#define pg_unreachable() abort()
#endif
+/*
+ * Define a compiler-independent macro for determining if an expression is a
+ * compile-time const. With MSVC using C11 and above, we can use a trick with
+ * _Generic to make this work. 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 a built-in constant. Callers may check if this
+ * macro is defined by checking if HAVE_PG_BUILTIN_CONSTANT is defined.
+ */
+#if defined(HAVE__BUILTIN_CONSTANT_P)
+#define pg_builtin_constant(x) __builtin_constant_p(x)
+#define HAVE_PG_BUILTIN_CONSTANT
+#elif defined(_MSC_VER) && defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
+#define pg_builtin_constant(x) \
+ _Generic((1 ? ((void *) ((x) * (uintptr_t) 0)) : &(int) {1}), int *: 1, void *: 0)
+#define HAVE_PG_BUILTIN_CONSTANT
+#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 675f4f5f469..7b575b59ed3 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -137,15 +137,15 @@ struct Node;
* prevents gcc from making the unreachability deduction at optlevel -O0.
*----------
*/
-#ifdef HAVE__BUILTIN_CONSTANT_P
+#ifdef HAVE_PG_BUILTIN_CONSTANT
#define ereport_domain(elevel, domain, ...) \
do { \
pg_prevent_errno_in_scope(); \
- if (__builtin_constant_p(elevel) && (elevel) >= ERROR ? \
+ if (pg_builtin_constant(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_constant(elevel) && (elevel) >= ERROR) \
pg_unreachable(); \
} while(0)
#else /* !HAVE__BUILTIN_CONSTANT_P */
--
2.40.1.windows.1
view thread (16+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p
In-Reply-To: <CAApHDvqH_gsb4UwHAQGy9w_oJYyB8y-esQbHNUoGsB668RZ9gQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox