Thread: make pg_attribute_noreturn() work for msvc?
Hi, At the bottom of https://www.postgresql.org/message-id/20191112192716.emrqs2afuefunw6v%40alap3.anarazel.de I mused about the somewhat odd coding pattern at the end of WalSndShutdown(): /* * Handle a client's connection abort in an orderly manner. */ static void WalSndShutdown(void) { /* * Reset whereToSendOutput to prevent ereport from attempting to send any * more messages to the standby. */ if (whereToSendOutput == DestRemote) whereToSendOutput = DestNone; proc_exit(0); abort(); /* keep the compiler quiet */ } namely that we prox_exit() and then abort(). This case seems to be purely historical baggage (from when this was inside other functiosn, before being centralized), so we can likely just remove the abort(). But even back then, one would have hoped that proc_exit() being annotated with pg_attribute_noreturn() should have told the compiler enough. But it turns out, we don't actually implement that for MSVC. Which does explain at least some cases where we had to add "keep compiler quiet" type code specifically for MSVC. As it turns out msvc has it's own annotation for functions that don't return, __declspec(noreturn). But it unfortunately needs to be placed before where we, so far, placed pg_attribute_noreturn(), namely after the function name / parameters. Instead it needs to be before the function name. But as it turns out GCC et al's __attribute__((noreturn)) actually can also be placed there, and seemingly for a long time: https://godbolt.org/z/8v5aFS So perhaps we ought to rename pg_attribute_noreturn() to pg_noreturn(), add a __declspec(noreturn) version, and move the existing uses to it. I'm inclined to also drop the parentheses at the same time (i.e pg_noreturn rather than pg_noreturn()) - it seems easier to mentally parse the code that way. I actually find the placement closer to the return type easier to understand, so I'd find this mildly beneficial even without the msvc angle. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> So perhaps we ought to rename pg_attribute_noreturn() to pg_noreturn(),
> add a __declspec(noreturn) version, and move the existing uses to it.
> I'm inclined to also drop the parentheses at the same time (i.e
> pg_noreturn rather than pg_noreturn()) - it seems easier to mentally
> parse the code that way.
I guess my big question about that is whether pgindent will make a
hash of it.
One idea is to merge it with the "void" result type that such
a function would presumably have, along the lines of
#define pg_noreturn void __declspec(noreturn)
...
extern pg_noreturn proc_exit(int);
and if necessary, we could strongarm pgindent into believing
that pg_noreturn is a typedef.
regards, tom lane
On 2019-11-12 15:58:07 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > So perhaps we ought to rename pg_attribute_noreturn() to pg_noreturn(), > > add a __declspec(noreturn) version, and move the existing uses to it. > > > I'm inclined to also drop the parentheses at the same time (i.e > > pg_noreturn rather than pg_noreturn()) - it seems easier to mentally > > parse the code that way. > > I guess my big question about that is whether pgindent will make a > hash of it. If one writes 'pg_noreturn void', rather than 'void pg_noreturn', then there's only one place where pgindent changes something in a somewhat weird way. For tablesync.c, it indents the pg_noreturn for finish_sync_worker(). But only due to being on a separate newline, which seems unnecessary… With 'void pg_noreturn', a few prototypes in headers get indented more than pretty, e.g. in pg_upgrade.h it turns void pg_noreturn pg_fatal(const char *fmt,...) pg_attribute_printf(1, 2); into void pg_noreturn pg_fatal(const char *fmt,...) pg_attribute_printf(1, 2); I'm a bit confused as to why pg_upgrade.h doesn't use 'extern' for function declarations? Not that it's really related, except for the 'extern' otherwise hiding the effect of pgindent not liking 'void pg_noreturn'… I don't see a reason not to go for 'pg_noreturn void'? > One idea is to merge it with the "void" result type that such > a function would presumably have, along the lines of > > #define pg_noreturn void __declspec(noreturn) > ... > extern pg_noreturn proc_exit(int); > and if necessary, we could strongarm pgindent into believing > that pg_noreturn is a typedef. Yea, that'd be an alternative. But since not necessary, I'd not go there? Greetings, Andres Freund
Attachment
Andres Freund <andres@anarazel.de> writes:
> On 2019-11-12 15:58:07 -0500, Tom Lane wrote:
>> I guess my big question about that is whether pgindent will make a
>> hash of it.
> If one writes 'pg_noreturn void', rather than 'void pg_noreturn', then
> there's only one place where pgindent changes something in a somewhat
> weird way. For tablesync.c, it indents the pg_noreturn for
> finish_sync_worker(). But only due to being on a separate newline, which
> seems unnecessary…
I think that it might be like that because some previous version of
pgindent changed it to that. That's probably why we never adopted
this style generally in the first place.
> With 'void pg_noreturn', a few prototypes in headers get indented more
> than pretty, e.g. in pg_upgrade.h it turns
> void pg_noreturn pg_fatal(const char *fmt,...) pg_attribute_printf(1, 2);
> into
> void pg_noreturn pg_fatal(const char *fmt,...) pg_attribute_printf(1, 2);
> I'm a bit confused as to why pg_upgrade.h doesn't use 'extern' for
> function declarations? Not that it's really related, except for the
> 'extern' otherwise hiding the effect of pgindent not liking 'void
> pg_noreturn'…
There are various headers where people have tended to not use "extern".
I always disliked that, thinking it was not per project style, but
never bothered to force the issue. If we went around and inserted
extern in these places, it wouldn't bother me any.
> I don't see a reason not to go for 'pg_noreturn void'?
That seems kind of ugly from here. Not sure why, but at least to
my mind that's a surprising ordering.
>> One idea is to merge it with the "void" result type that such
>> a function would presumably have, along the lines of
>> #define pg_noreturn void __declspec(noreturn)
> Yea, that'd be an alternative. But since not necessary, I'd not go
> there?
I kind of liked that idea, too bad you don't. One argument for it
is that then there'd be exactly one right way to do it, not two.
Also, if we find out that there's some compiler that's pickier
about where to place the annotation, we'd have a central place
to handle it.
regards, tom lane
Hi, On 2019-11-12 17:22:05 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2019-11-12 15:58:07 -0500, Tom Lane wrote: > > I'm a bit confused as to why pg_upgrade.h doesn't use 'extern' for > > function declarations? Not that it's really related, except for the > > 'extern' otherwise hiding the effect of pgindent not liking 'void > > pg_noreturn'… > > There are various headers where people have tended to not use "extern". > I always disliked that, thinking it was not per project style, but > never bothered to force the issue. If we went around and inserted > extern in these places, it wouldn't bother me any. > > I don't see a reason not to go for 'pg_noreturn void'? > > That seems kind of ugly from here. Not sure why, but at least to > my mind that's a surprising ordering. Oh, to me it seemed a quite reasonable order. It think it feels that way to me because we put properties like 'static', 'extern', 'inline' etc also before the return type (and it's similar for variable declarations too). It's maybe also worthwhile to note that emacs parses 'pg_noreturn void' correctly, but gets confused by 'void pg_noreturn'. It's just syntax highlighting though, so whatever. > >> One idea is to merge it with the "void" result type that such > >> a function would presumably have, along the lines of > >> #define pg_noreturn void __declspec(noreturn) > > > Yea, that'd be an alternative. But since not necessary, I'd not go > > there? > > I kind of liked that idea, too bad you don't. I don't actively dislike it. It just seemed a bit more magic than necessary. One need not understand what pg_noreturn does - not that it's hard to infer from the name - to know the return type of the function. > One argument for it is that then there'd be exactly one right way to > do it, not two. Also, if we find out that there's some compiler > that's pickier about where to place the annotation, we'd have a > central place to handle it. The former seems like a good argument to me. I'm not quite sure I think the second is likely. It's worthwhile to note - I forgot this - that noreturn actually has been standardized in C11 and C++11. For C11 the keyword is _Noreturn, with a convenience macro 'noreturn' defined in stdnoreturn.h. For C++11, the syntax is (please don't get an aneurysm...): [[ noreturn ]] void funcname(params)... (yes, the [[]] are actually part of the syntax, not some BNF like thing) I *think* the standard prescribes _Noreturn to be before the return type (it's defined in the same rule as inline), but I have some difficulty parsing the standard language. Gcc at least accepts inline only before the return type, but _Noreturn in both places. Certainly all the standard examples place it before the type. While it looks tempting to just use 'noreturn', and backfill it if the current environment doesn't support it, I think that's a bit too dangerous, because it will tend to break other code like __attribute__((noreturn)) and _declspec(noreturn). As there's plenty other software using either or both of these, I don't think it's worth going there. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> It's worthwhile to note - I forgot this - that noreturn actually has
> been standardized in C11 and C++11. For C11 the keyword is _Noreturn,
> with a convenience macro 'noreturn' defined in stdnoreturn.h.
> For C++11, the syntax is (please don't get an aneurysm...):
> [[ noreturn ]] void funcname(params)...
> (yes, the [[]] are actually part of the syntax, not some BNF like thing)
Egad. I'd *want* to hide that under a macro :-(
> While it looks tempting to just use 'noreturn', and backfill it if the
> current environment doesn't support it, I think that's a bit too
> dangerous, because it will tend to break other code like
> __attribute__((noreturn)) and _declspec(noreturn). As there's plenty
> other software using either or both of these, I don't think it's worth
> going there.
Agreed, defining noreturn is too dangerous, it'll have to be
pg_noreturn. Or maybe use _Noreturn? But that feels ugly too.
Anyway, I still like the idea of merging the void keyword in with
that.
regards, tom lane
Hi, On 2019-11-12 18:15:28 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > It's worthwhile to note - I forgot this - that noreturn actually has > > been standardized in C11 and C++11. For C11 the keyword is _Noreturn, > > with a convenience macro 'noreturn' defined in stdnoreturn.h. > > > For C++11, the syntax is (please don't get an aneurysm...): > > [[ noreturn ]] void funcname(params)... > > (yes, the [[]] are actually part of the syntax, not some BNF like thing) > > Egad. I'd *want* to hide that under a macro :-( Yea, it's quite ugly. I think the only saving grace is that C++ made that the generic syntax for various annotations / attributes. Everywhere, not just for function properties. So there's [[noreturn]], [[fallthrough]], [[nodiscard]], [[maybe_unused]] etc, and that there is explicit namespacing for vendor extensions by using [[vendorname::attname]], e.g. the actually existing [[gnu::always_inline]]. There's probably not that many other forms of syntax one can add to all the various places, without running into syntax limitations, or various vendor extensions... But still. > > While it looks tempting to just use 'noreturn', and backfill it if the > > current environment doesn't support it, I think that's a bit too > > dangerous, because it will tend to break other code like > > __attribute__((noreturn)) and _declspec(noreturn). As there's plenty > > other software using either or both of these, I don't think it's worth > > going there. > > Agreed, defining noreturn is too dangerous, it'll have to be > pg_noreturn. Or maybe use _Noreturn? But that feels ugly too. Yea, I don't like that all that much. We'd have to define it in C++ mode, and it's in the explicit standard reserved namespace... > Anyway, I still like the idea of merging the void keyword in with > that. Hm. Any other opinions? Greetings, Andres Freund
On 2019-Nov-12, Andres Freund wrote: > > Anyway, I still like the idea of merging the void keyword in with > > that. > > Hm. Any other opinions? Although it feels very strange to me at first glance, one only has to learn the trick once. My initial inclination was not to do it, but I'm kinda +0.1 after thinking some more about it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 13, 2019 at 11:28 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Nov-12, Andres Freund wrote: > > > Anyway, I still like the idea of merging the void keyword in with > > > that. > > > > Hm. Any other opinions? > > Although it feels very strange to me at first glance, one only has to > learn the trick once. My initial inclination was not to do it, but I'm > kinda +0.1 after thinking some more about it. I don't care much about this either way, but I think I might be slightly more inclined to keep them separate. If we went the direction of combining them, it might be clearer if the magic word included "void" someplace inside of it, like: extern void_noreturn thunk(void); -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company