Re: pg_attribute_noreturn(), MSVC, C11

Hi,

On 2025-03-10 13:27:07 +0100, Peter Eisentraut wrote:
> From: Peter Eisentraut <peter(at)eisentraut(dot)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(at)eisentraut(dot)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