Horton

Thread

Commits

  1. Use standard C23 and C++ attributes if available

  2. Swap order of extern/static and pg_nodiscard

  3. pg_noreturn to replace pg_attribute_noreturn()

  1. pg_attribute_noreturn(), MSVC, C11

    Andres Freund <andres@anarazel.de> — 2024-12-13T19:10:13Z

    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...

  2. Re: pg_attribute_noreturn(), MSVC, C11

    Andres Freund <andres@anarazel.de> — 2024-12-13T19:54:11Z

    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

  3. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-14T17:15:13Z

    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.

  4. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-14T17:18:35Z

    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.

  5. Re: pg_attribute_noreturn(), MSVC, C11

    Andres Freund <andres@anarazel.de> — 2024-12-15T15:53:12Z

    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

  6. Re: pg_attribute_noreturn(), MSVC, C11

    Andres Freund <andres@anarazel.de> — 2024-12-15T16:15:23Z

    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

  7. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-28T10:28:40Z

    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.)

  8. Re: pg_attribute_noreturn(), MSVC, C11

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2025-01-03T20:51:13Z

    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

  9. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-06T14:52:15Z

    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))

  10. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-22T18:16:03Z

    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))

  11. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-13T15:34:55Z

    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.

  12. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-10T12:27:07Z

    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.

  13. Re: pg_attribute_noreturn(), MSVC, C11

    Andres Freund <andres@anarazel.de> — 2025-03-10T13:58:57Z

    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

  14. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-10T14:37:43Z

    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.

  15. Re: pg_attribute_noreturn(), MSVC, C11

    Andres Freund <andres@anarazel.de> — 2025-03-10T14:38:59Z

    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.

  16. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-13T12:43:30Z

    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.

  17. Re: pg_attribute_noreturn(), MSVC, C11

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-14T08:32:51Z

    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.