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