pg_attribute_noreturn(), MSVC, C11

Hi,

On 2024-12-13 14:10:13 -0500, Andres Freund wrote:

I just encountered another
warning C4715: 'XYZ: not all control paths return a value

with msvc in CI in a case where it should be trivial for the compiler to
recognize that the function return isn't reachable.

Which made me check if these days msvc has something like gcc's
__attribute__((noreturn)).

And it turns out that yes! The _Noreturn attribute has been added to C11 and
msvc supports C11:
https://learn.microsoft.com/en-us/cpp/c-language/noreturn?view=msvc-170

Besides the _Noreturn keyword the standard also added a stdnoreturn.h which
provides the 'noreturn' macro.

I first thought we could just implement pg_attribute_noreturn() using
_Noreturn if available. However, our current pg_attribute_noreturn() is in the
wrong place for that to work :(. _Noreturn is to be placed with the return
type, whereas function attributes with the __attribute__(()) syntax are after
the parameter list.

The point about __attribute__(()) being after the parameter list is wrong, I
confused myself there. But that doesn't change that much, the common current
placement doesn't work for _Noreturn.

C11 has been out a while, so I'm somewhat inclined to adopt _Noreturn/noreturn
in a conditional way. Older compilers would still work, just not understand
noreturn.

One wrinkle: _Noreturn/noreturn have been deprecated in C23, because that
adopted C++11's attribute syntax (i.e. [[noreturn]]). But that's at least in
the same place as _Noreturn/return.

We can't remove [[noreturn]] with preprocessor magic, so it's not really
viable to use that for, uhm, quite a while.

If we were to use _Noreturn, I think it could just be something like:

I think it should suffice to do something like

#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
#define pg_noreturn _Noreturn
#else
#define pg_noreturn
#endif

(or alternatively include stdreturn if __STDC_VERSION__ indicates support and
define a bare 'noreturn' if not)

Another wrinkle: While __attribute__((noreturn)) works for function pointers
(or function pointer typedefs) _Noreturn doesn't. Gah. We only use it that
way in two places, but still :(

Greetings,

Andres Freund

On 13.12.24 20:10, Andres Freund wrote:

C11 has been out a while, so I'm somewhat inclined to adopt _Noreturn/noreturn
in a conditional way. Older compilers would still work, just not understand
noreturn.

One wrinkle: _Noreturn/noreturn have been deprecated in C23, because that
adopted C++11's attribute syntax (i.e. [[noreturn]]). But that's at least in
the same place as _Noreturn/return.

We can't remove [[noreturn]] with preprocessor magic, so it's not really
viable to use that for, uhm, quite a while.

If we were to use _Noreturn, I think it could just be something like:

I think it should suffice to do something like

#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
#define pg_noreturn _Noreturn
#else
#define pg_noreturn
#endif

This looks reasonable to me. We also have pg_nodiscard. (That's got a
slightly different history in the C standard, but I mean it's also
"pg_someattribute".)

(or alternatively include stdreturn if __STDC_VERSION__ indicates support and
define a bare 'noreturn' if not)

For msvc that mean we'd need to add /std:c11 (or /std:c17) to the compiler
flags, as it otherwise it results in a weird mix of c89 an c99). But that
might be a good idea anyway. With one minor change [1] the tests pass with
msvc when using /std:c17.

According to my notes, C11 requires MSVC 2019, and we currently require
2015, so this will require a bit of logic.

On 13.12.24 20:54, Andres Freund wrote:

Another wrinkle: While __attribute__((noreturn)) works for function pointers
(or function pointer typedefs) _Noreturn doesn't. Gah. We only use it that
way in two places, but still :(

Yeah, I wrote an experimental patch for noreturn support some years ago,
and that was also my result back then. (I assume you have a current
patch, otherwise I can dig out that one.) I had also written down that
there were some problems with Perl and Tcl headers, FWIW. Did you have
any problems with those?

I think we can take a small loss here and move with the standard.
Unless you can think of a way to define
pg_noreturn_but_for_function_pointers in a systematic way.

Hi,

On 2024-12-14 18:15:13 +0100, Peter Eisentraut wrote:

On 13.12.24 20:10, Andres Freund wrote:

(or alternatively include stdreturn if __STDC_VERSION__ indicates support and
define a bare 'noreturn' if not)

For msvc that mean we'd need to add /std:c11 (or /std:c17) to the compiler
flags, as it otherwise it results in a weird mix of c89 an c99). But that
might be a good idea anyway. With one minor change [1] the tests pass with
msvc when using /std:c17.

According to my notes, C11 requires MSVC 2019, and we currently require
2015, so this will require a bit of logic.

Yea. Not hard though:

@@ -298,6 +298,7 @@ elif host_system == 'windows'
   if cc.get_id() == 'msvc'
     ldflags += '/INCREMENTAL:NO'
     ldflags += '/STACK:@0@'.format(cdata.get('WIN32_STACK_RLIMIT'))
+    cflags += cc.first_supported_argument('/std:c17', '/std:c11')
     # ldflags += '/nxcompat' # generated by msbuild, should have it for ninja?
   else
     ldflags += '-Wl,--stack,@0@'.format(cdata.get('WIN32_STACK_RLIMIT'))

It's not quite the way meson wants you to do it (declare it in
default_options), but with our minimum meson version that's just a single
option, not a list...

Greetings,

Andres Freund

Hi,

On 2024-12-14 18:18:35 +0100, Peter Eisentraut wrote:

On 13.12.24 20:54, Andres Freund wrote:

Another wrinkle: While __attribute__((noreturn)) works for function pointers
(or function pointer typedefs) _Noreturn doesn't. Gah. We only use it that
way in two places, but still :(

Yeah, I wrote an experimental patch for noreturn support some years ago, and
that was also my result back then.

It's quite annoying...

(I assume you have a current patch, otherwise I can dig out that one.)

Yea, I do. Not pretty, but ...

I guess I'll try to pretty it up a bit and post it then.

I had also written down that there were some problems with Perl and Tcl
headers, FWIW. Did you have any problems with those?

Not so far.

I think we can take a small loss here and move with the standard. Unless you
can think of a way to define pg_noreturn_but_for_function_pointers in a
systematic way.

The small loss unfortunately isn't that small, because clang treats
__attribute__((noreturn)) to be part of the function signature, but not
_Noreturn. Which means you can't just put __attribute__((noreturn)) to the
function pointer's signature, because it'll complain about incompatible
function pointers:

../../../../../home/andres/src/postgresql/src/backend/backup/basebackup_incremental.c:179:20: error: incompatible function pointer types assigning to 'json_manifest_error_callback' (aka 'void (*)(struct JsonManifestParseContext *, const char *, ...) __attribute__((noreturn))') from 'void (JsonManifestParseContext *, const char *, ...)' (aka 'void (struct JsonManifestParseContext *, const char *, ...)') [-Wincompatible-function-pointer-types]
179 | context->error_cb = manifest_report_error;

A workaround would be to have pg_nodiscard to just specify both
__attribute__((noreturn)) and _Nodiscard, and
pg_noreturn_but_for_function_pointers just specify __attribute__((noreturn)).
But at that point it's not obvious why we'd use _Nodiscard at all.

I wonder if we should switch to pg_nodiscard in the position that _Nodiscard
would be used and implement it as __attribute__((noreturn)) on gnu like
compilers and __declspec(noreturn) on msvc.

Kinda curious that we have pg_nodiscard, but didn't use __declspec(nodiscard).

Greetings,

Andres Freund

On 14.12.24 18:18, Peter Eisentraut wrote:

On 13.12.24 20:54, Andres Freund wrote:

Another wrinkle: While __attribute__((noreturn)) works for function
pointers
(or function pointer typedefs) _Noreturn doesn't. Gah.  We only use it
that
way in two places, but still :(

Yeah, I wrote an experimental patch for noreturn support some years ago,
and that was also my result back then.  (I assume you have a current
patch, otherwise I can dig out that one.)  I had also written down that
there were some problems with Perl and Tcl headers, FWIW.  Did you have
any problems with those?

I think we can take a small loss here and move with the standard. Unless
you can think of a way to define pg_noreturn_but_for_function_pointers
in a systematic way.

I have a patch proposal here. I discovered a few more complications
that need to be paid attention to.

First, we can't use bare "noreturn". There are third-party header files
(such as Tcl) that use __attribute__((noreturn)), and that will get
confused. That's the same reason we don't use bare restrict but
pg_restrict.

I suggest we define pg_noreturn as

1. If C11 is supported, then _Noreturn, else
2. If GCC-compatible, then __attribute__((noreturn)), else
3. If MSVC, then __declspec((noreturn))

When PostgreSQL starts requiring C11, then the latter two options can be
dropped.

Note that this also fixes a possible conflict where some third-party
code includes <stdnoreturn.h> and then includes our c.h. I don't think
this has been reported, but it's surely bound to happen.

For the function pointers, I don't think there is a good solution. The
current behavior is evidently inconsistent and incompatible and not well
documented, and I don't see how it's going to get better in these
aspects any time soon. I think the way forward in the mid-term is to
avoid designing more interfaces like that and provide wrapper functions
like json_manifest_parse_failure() where you can enforce the return
behavior in the normal way.

Finally, while I was looking at this, I figured we could also add
pg_nodiscard support to non-GCC compilers that support C23 (probably
none right now, but eventually), by defining pg_nodiscard as
[[nodiscard]]. But that revealed that clang requires these attributes
to appear before the extern/static keywords, which is not how it's
currently written. So I changed that, too, and also wrote the
pg_noreturn patch in the same style. And then I also added a definition
of pg_noreturn as [[noreturn]] (actually, [[__noreturn__]], for the
reasons given earlier), for consistency, and also as a hedge in case
some compiler drops C11 support in a few decades. (I tried to
language-lawyer my way through this, and I don't know that clang is
correct here, but this issue is widely reported out there and everyone
agrees that the fix is to just swap the things around. At least we can
enforce some stylistic consistency that way.)

Peter Eisentraut <peter@eisentraut.org> writes:

I suggest we define pg_noreturn as

1. If C11 is supported, then _Noreturn, else
2. If GCC-compatible, then __attribute__((noreturn)), else

Would it be worth also checking __has_attribute(noreturn)? Or do all
compilers that have __attribute__((noreturn)) claim to be GCC?

3. If MSVC, then __declspec((noreturn))

- ilmari

On 03.01.25 21:51, Dagfinn Ilmari Mannsåker wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

I suggest we define pg_noreturn as

1. If C11 is supported, then _Noreturn, else
2. If GCC-compatible, then __attribute__((noreturn)), else

Would it be worth also checking __has_attribute(noreturn)? Or do all
compilers that have __attribute__((noreturn)) claim to be GCC?

I don't think that would expand the set of supported compilers in a
significant way. We can always add it if we find one, of course.

On 06.01.25 15:52, Peter Eisentraut wrote:

On 03.01.25 21:51, Dagfinn Ilmari Mannsåker wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

I suggest we define pg_noreturn as

1. If C11 is supported, then _Noreturn, else
2. If GCC-compatible, then __attribute__((noreturn)), else

Would it be worth also checking __has_attribute(noreturn)?  Or do all
compilers that have __attribute__((noreturn)) claim to be GCC?

I don't think that would expand the set of supported compilers in a
significant way.  We can always add it if we find one, of course.

In fact, as another thought, we could even drop #2. Among the
GCC-compatible compilers, both GCC and Clang have supported #1 for ages,
and the only other candidate I could find on the build farm is the
Solaris compiler, which also supports C11 by default, per its documentation.

On 22.01.25 19:16, Peter Eisentraut wrote:

On 06.01.25 15:52, Peter Eisentraut wrote:

On 03.01.25 21:51, Dagfinn Ilmari Mannsåker wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

I suggest we define pg_noreturn as

1. If C11 is supported, then _Noreturn, else
2. If GCC-compatible, then __attribute__((noreturn)), else

Would it be worth also checking __has_attribute(noreturn)?  Or do all
compilers that have __attribute__((noreturn)) claim to be GCC?

I don't think that would expand the set of supported compilers in a
significant way.  We can always add it if we find one, of course.

In fact, as another thought, we could even drop #2.  Among the GCC-
compatible compilers, both GCC and Clang have supported #1 for ages, and
the only other candidate I could find on the build farm is the Solaris
compiler, which also supports C11 by default, per its documentation.

3. If MSVC, then __declspec((noreturn))

Here is an updated patch set that contains the above small change and
fixes some conflicts that have arisen in the meantime.

On 13.02.25 16:34, Peter Eisentraut wrote:

On 22.01.25 19:16, Peter Eisentraut wrote:

On 06.01.25 15:52, Peter Eisentraut wrote:

On 03.01.25 21:51, Dagfinn Ilmari Mannsåker wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

I suggest we define pg_noreturn as

1. If C11 is supported, then _Noreturn, else
2. If GCC-compatible, then __attribute__((noreturn)), else

Would it be worth also checking __has_attribute(noreturn)?  Or do all
compilers that have __attribute__((noreturn)) claim to be GCC?

I don't think that would expand the set of supported compilers in a
significant way.  We can always add it if we find one, of course.

In fact, as another thought, we could even drop #2.  Among the GCC-
compatible compilers, both GCC and Clang have supported #1 for ages,
and the only other candidate I could find on the build farm is the
Solaris compiler, which also supports C11 by default, per its
documentation.

3. If MSVC, then __declspec((noreturn))

Here is an updated patch set that contains the above small change and
fixes some conflicts that have arisen in the meantime.

Another rebased patch set, no further changes.

Hi,

On 2025-03-10 13:27:07 +0100, Peter Eisentraut wrote:

From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 13 Feb 2025 16:01:06 +0100
Subject: [PATCH v3 1/4] pg_noreturn to replace pg_attribute_noreturn()

We want to support a "noreturn" decoration on more compilers besides
just GCC-compatible ones, but for that we need to move the decoration
in front of the function declaration instead of either behind it or
wherever, which is the current style afforded by GCC-style attributes.
Also rename the macro to "pg_noreturn" to be similar to the C11
standard "noreturn".

pg_noreturn is now supported on all compilers that support C11 (using
_Noreturn), as well as MSVC (using __declspec). (When PostgreSQL
requires C11, the latter variant can be dropped.) (We don't need the
previously used variant for GCC-compatible compilers using
__attribute__, because all reasonable candidates already support C11,
so that variant would be dead code in practice.)

Now, all supported compilers effectively support pg_noreturn, so the
extra code for !HAVE_PG_ATTRIBUTE_NORETURN can be dropped.

This also fixes a possible problem if third-party code includes
stdnoreturn.h, because then the current definition of

#define pg_attribute_noreturn() __attribute__((noreturn))

would cause an error.

Note that the C standard does not support a noreturn attribute on
function pointer types. So we have to drop these here. There are
only two instances at this time, so it's not a big loss.

That's still somewhat sad, but it's probably not worth having a separate
attribute just for those cases...

With the minor comment suggestion below, I think this looks good.

diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index ec8eddad863..d32c0d318fb 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -601,8 +601,8 @@ SlabAllocFromNewBlock(MemoryContext context, Size size, int flags)
*		want to avoid that.
*/
pg_noinline
+pg_noreturn
static void
-pg_attribute_noreturn()
SlabAllocInvalidSize(MemoryContext context, Size size)
{
SlabContext *slab = (SlabContext *) context;

Hm, is it good to put the pg_noreturn after the pg_noinline?

+/*
+ * pg_noreturn corresponds to the C11 noreturn/_Noreturn function specifier.
+ * We can't use the standard name "noreturn" because some third-party code
+ * uses __attribute__((noreturn)) in headers, which would get confused if
+ * "noreturn" is defined to "_Noreturn", as is done by <stdnoreturn.h>.
+ */
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
+#define pg_noreturn _Noreturn
+#elif defined(_MSC_VER)
+#define pg_noreturn __declspec(noreturn)
+#else
+#define pg_noreturn
+#endif

I think it'd be good to add a comment explaining the placement of pg_noreturn
in a declaration, it's getting pickier...

Subject: [PATCH v3 2/4] Add another pg_noreturn

WFM

Subject: [PATCH v3 3/4] Swap order of extern/static and pg_nodiscard

Clang in C23 mode requires all attributes to be before extern or
static. So just swap these. This also keeps the order consistent
with the previously introduced pg_noreturn.

WFM.

From 0d9f2f63e2166592bd703320cf18dfb61a47ab28 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 28 Dec 2024 11:00:24 +0100
Subject: [PATCH v3 4/4] Support pg_nodiscard on non-GNU compilers that support
C23

Support pg_nodiscard on compilers that support C23 attribute syntax.
Previously, only GCC-compatible compilers were supported.

Also, define pg_noreturn similarly using C23 attribute syntax if the
compiler supports C23. This doesn't have much of an effect right now,
since all compilers that support C23 surely also support the existing
C11-compatible code. But it keeps pg_nodiscard and pg_noreturn
consistent.

This is a bit unexciting, but I guess worth it.

Greetings,

Andres Freund

On 10.03.25 14:58, Andres Freund wrote:

diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index ec8eddad863..d32c0d318fb 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -601,8 +601,8 @@ SlabAllocFromNewBlock(MemoryContext context, Size size, int flags)
*		want to avoid that.
*/
pg_noinline
+pg_noreturn
static void
-pg_attribute_noreturn()
SlabAllocInvalidSize(MemoryContext context, Size size)
{
SlabContext *slab = (SlabContext *) context;

Hm, is it good to put the pg_noreturn after the pg_noinline?

I probably just did them alphabetically. I don't think there would be a
problem, since pg_noinline is an attribute, and they can generally be
put everywhere. At least until we learn otherwise.

Hi,

On March 10, 2025 10:37:43 AM EDT, Peter Eisentraut <peter@eisentraut.org> wrote:

On 10.03.25 14:58, Andres Freund wrote:

diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index ec8eddad863..d32c0d318fb 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -601,8 +601,8 @@ SlabAllocFromNewBlock(MemoryContext context, Size size, int flags)
*		want to avoid that.
*/
pg_noinline
+pg_noreturn
static void
-pg_attribute_noreturn()
SlabAllocInvalidSize(MemoryContext context, Size size)
{
SlabContext *slab = (SlabContext *) context;

Hm, is it good to put the pg_noreturn after the pg_noinline?

I probably just did them alphabetically. I don't think there would be a problem, since pg_noinline is an attribute, and they can generally be put everywhere. At least until we learn otherwise.

Just seems easier to document that no return etc should go first. But it's not important.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

I committed the first two patches (squashed together) (about
pg_noreturn). I had to make one change: I put back the GCC fallback
that I had removed between v1 and v2. This is needed for GCC versions
before C11 became the default (gcc 5) and also for situations like
buildfarm member mylodon that builds with -std=c99 explicitly.
(Otherwise, that configuration would get a bunch of compiler warnings
about uninitialized variables etc.) I also added the additional comment
about placement that you had requested.

I'm going to postpone the remaining two patches (about pg_nodiscard).
After experimenting a bit more, I'm less sure about what the correct
placement of C23 attributes is meant to be, and without understanding
that, I fear this would make the earlier question about the correct
placement of pg_noreturn unnecessarily more complicated. This can be a
future project.

On 13.03.25 13:43, Peter Eisentraut wrote:

I committed the first two patches (squashed together) (about
pg_noreturn).  I had to make one change: I put back the GCC fallback
that I had removed between v1 and v2.  This is needed for GCC versions
before C11 became the default (gcc 5) and also for situations like
buildfarm member mylodon that builds with -std=c99 explicitly.
(Otherwise, that configuration would get a bunch of compiler warnings
about uninitialized variables etc.)  I also added the additional comment
about placement that you had requested.

I'm going to postpone the remaining two patches (about pg_nodiscard).
After experimenting a bit more, I'm less sure about what the correct
placement of C23 attributes is meant to be, and without understanding
that, I fear this would make the earlier question about the correct
placement of pg_noreturn unnecessarily more complicated.  This can be a
future project.

After some reflection, I committed the middle patch ("Swap order of
extern/static and pg_nodiscard") after all. The code comment about the
provenance of the name needed updating anyway, and it made sense in that
context to adjust the order to make it more future-proof and make it
consistent with pg_noreturn.

I'll leave the last patch out for now, though.