Thread: pg_attribute_noreturn(), MSVC, C11
Hi, 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. So we probably can't transparently switch between these. 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) 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. Before realizing that we'd have to change our existing annotations and that there's no way to use both old and new syntax, depending on compiler support, I was thinking this would be an obvious thing to do. I'm still leaning on it being worth it, but not as clearly as before. For an example of _Noreturn being used: https://godbolt.org/z/j15d35dao Greetings, Andres Freund [1] The msvc implementation of VA_ARGS_NARGS only works with the traditional preprocessor, but C11 support enables the new one. But we can detect that with something like (!defined(_MSVC_TRADITIONAL) || _MSVC_TRADITIONAL) See https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170 and https://learn.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=msvc-170 Without adapting the definition of VA_ARGS_NARGS compilation fails as it results in the macro resulting in VA_ARGS_NARGS_(prefix, 63, 62, ...) in the using code...
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 functionpointer types assigning to 'json_manifest_error_callback' (aka 'void (*)(struct JsonManifestParseContext *, constchar *, ...) __attribute__((noreturn))') from 'void (JsonManifestParseContext *, const char *, ...)' (aka 'void (structJsonManifestParseContext *, 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
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. >> 3. If MSVC, then __declspec((noreturn))
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.
Attachment
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,
andthey 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.