Thread: Proposal: Add more compile-time asserts to expose inconsistencies.
Dear Hackers, I have identified some OSS code where more compile-time asserts could be added. Mostly these are asserting that arrays have the necessary length to accommodate the enums that are used to index into them. In general the code is already commented with warnings such as: * "If you add a new entry, remember to ..." * "When modifying this enum, update the table in ..." * "Display names for enums in ..." * etc. But comments can be accidentally overlooked, so adding the compile-time asserts can help eliminate human error. Please refer to the attached patch. Kind Regards, Peter Smith --- Fujitsu Australia
Attachment
On Wed, Sep 18, 2019 at 06:46:24AM +0000, Smith, Peter wrote: > I have identified some OSS code where more compile-time asserts could be added. > > Mostly these are asserting that arrays have the necessary length to > accommodate the enums that are used to index into them. > > In general the code is already commented with warnings such as: > * "If you add a new entry, remember to ..." > * "When modifying this enum, update the table in ..." > * "Display names for enums in ..." > * etc. > > But comments can be accidentally overlooked, so adding the > compile-time asserts can help eliminate human error. For some of them it could help, and we could think about a better location for that stuff than an unused routine. The indentation of your patch is weird, with "git diff --check" complaining a lot. If you want to discuss more about that, could you add that to the next commit fest? Here it is: https://commitfest.postgresql.org/25/ -- Michael
Attachment
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Michael Paquier <michael@paquier.xyz> writes: > On Wed, Sep 18, 2019 at 06:46:24AM +0000, Smith, Peter wrote: >> I have identified some OSS code where more compile-time asserts could be added. >> >> Mostly these are asserting that arrays have the necessary length to >> accommodate the enums that are used to index into them. >> >> In general the code is already commented with warnings such as: >> * "If you add a new entry, remember to ..." >> * "When modifying this enum, update the table in ..." >> * "Display names for enums in ..." >> * etc. >> >> But comments can be accidentally overlooked, so adding the >> compile-time asserts can help eliminate human error. > > For some of them it could help, and we could think about a better > location for that stuff than an unused routine. Postgres doesn't seem to have it, but it would be possible to define a StaticAssertDecl macro that can be used at the file level, outside any function. See for example Perl's STATIC_ASSERT_DECL: https://github.com/Perl/perl5/blob/v5.30.0/perl.h#L3455-L3488 - ilmari -- "The surreality of the universe tends towards a maximum" -- Skud's Law "Never formulate a law or axiom that you're not prepared to live with the consequences of." -- Skud's Meta-Law
-----Original Message----- From: Michael Paquier <michael@paquier.xyz> Sent: Wednesday, 18 September 2019 5:40 PM > For some of them it could help, and we could think about a better location for that stuff than an unused routine. Theindentation of your patch is weird, with "git diff --check" complaining a lot. > > If you want to discuss more about that, could you add that to the next commit fest? Here it is: https://commitfest.postgresql.org/25/ Hi Michael, Thanks for your feedback and advice. I have modified the patch to clean up the whitespace issues, and added it to the next commit fest. Kind Regards, --- Peter Smith Fujitsu Australia
Attachment
On Wed, Sep 18, 2019 at 04:46:30PM +0100, Dagfinn Ilmari Mannsåker wrote: > Postgres doesn't seem to have it, but it would be possible to define a > StaticAssertDecl macro that can be used at the file level, outside any > function. See for example Perl's STATIC_ASSERT_DECL: > > https://github.com/Perl/perl5/blob/v5.30.0/perl.h#L3455-L3488 That sounds like a cleaner alternative. Thanks for the pointer. -- Michael
Attachment
At Thu, 19 Sep 2019 10:07:40 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190919010740.GC22307@paquier.xyz> > On Wed, Sep 18, 2019 at 04:46:30PM +0100, Dagfinn Ilmari Mannsåker wrote: > > Postgres doesn't seem to have it, but it would be possible to define a > > StaticAssertDecl macro that can be used at the file level, outside any > > function. See for example Perl's STATIC_ASSERT_DECL: > > > > https://github.com/Perl/perl5/blob/v5.30.0/perl.h#L3455-L3488 > > That sounds like a cleaner alternative. Thanks for the pointer. The cause for StaticAssertStmt not being usable outside of functions is enclosing do-while, which is needed to avoid "mixed declaration" warnings, which we are inhibiting to use as of now. Therefore just defining another macro defined as just _Static_assert() works fine. I don't find an alternative way for the tool chains that don't have static assertion feature. In the attached diff the macro is defined as nothing. I don't find a way to warn that the assertion is ignored. regards. diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 90ffd89339..822b9846bf 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4601,7 +4601,6 @@ static void write_auto_conf_file(int fd, const char *filename, ConfigVariable *h static void replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p, const char *name, const char *value); - /* * Some infrastructure for checking malloc/strdup/realloc calls */ diff --git a/src/include/c.h b/src/include/c.h index f461628a24..a386a81a19 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -836,11 +836,14 @@ extern void ExceptionalCondition(const char *conditionName, #ifdef HAVE__STATIC_ASSERT #define StaticAssertStmt(condition, errmessage) \ do { _Static_assert(condition, errmessage); } while(0) +#define StaticAssertDecl(condition, errmessage) \ + _Static_assert(condition, errmessage) #define StaticAssertExpr(condition, errmessage) \ ((void) ({ StaticAssertStmt(condition, errmessage); true; })) #else /* !HAVE__STATIC_ASSERT */ #define StaticAssertStmt(condition, errmessage) \ ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; })) +#define StaticAssertDecl(condition, errmessage) /* no alternatives exist */ #define StaticAssertExpr(condition, errmessage) \ StaticAssertStmt(condition, errmessage) #endif /* HAVE__STATIC_ASSERT */ @@ -848,11 +851,14 @@ extern void ExceptionalCondition(const char *conditionName, #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410 #define StaticAssertStmt(condition, errmessage) \ static_assert(condition, errmessage) +#define StaticAssertDecl(condition, errmessage) \ + static_assert(condition, errmessage) #define StaticAssertExpr(condition, errmessage) \ ({ static_assert(condition, errmessage); }) #else #define StaticAssertStmt(condition, errmessage) \ do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0) +#define StaticAssertDecl(condition, errmessage) /* no alternatives exist */ #define StaticAssertExpr(condition, errmessage) \ ((void) ({ StaticAssertStmt(condition, errmessage); })) #endif
-----Original Message----- From: Michael Paquier <michael@paquier.xyz> Sent: Thursday, 19 September 2019 11:08 AM >On Wed, Sep 18, 2019 at 04:46:30PM +0100, Dagfinn Ilmari Mannsåker wrote: >> Postgres doesn't seem to have it, but it would be possible to define a >> StaticAssertDecl macro that can be used at the file level, outside any >> function. See for example Perl's STATIC_ASSERT_DECL: >> >> https://github.com/Perl/perl5/blob/v5.30.0/perl.h#L3455-L3488 > >That sounds like a cleaner alternative. Thanks for the pointer. In the attached patch example I have defined a new macro StaticAssertDecl. A typical usage of it is shown in the relpath.cfile. The goal was to leave all existing Postgres static assert macros unchanged, but to allow static asserts to be added in thecode at file scope without the need for the explicit ct_asserts function. Notice, in reality the StaticAssertDecl macro still uses a static function as a wrapper for the StaticAssertStmt, but nowthe function is not visible in the source. If this strategy is acceptable I will update my original patch to remove all those ct_asserts functions, and instead puteach StaticAssertDecl nearby the array that it is asserting (e.g. just like relpath.c) Kind Regards, Peter Smith --- Fujitsu Australia
Attachment
Hi, On 2019-09-19 04:46:27 +0000, Smith, Peter wrote: > In the attached patch example I have defined a new macro > StaticAssertDecl. A typical usage of it is shown in the relpath.c > file. I'm in favor of adding that - in fact, when I was working on adding a a static_assert wrapper, I was advocating for only supporting compilers that know static_assert, so we can put these in the global scope. The number of compilers that don't support static_assert is pretty small today, especially compared to 2012, when we added these. https://www.postgresql.org/message-id/E1TIW1p-0002yE-AY%40gemulon.postgresql.org https://www.postgresql.org/message-id/27446.1349024252%40sss.pgh.pa.us Tom, you were arguing for restricting to file scope to get broader compatibility, are you ok with adding a seperate *Decl version? Or perhaps it's time to just remove the fallback implementation? I think we'd have to add proper MSVC support, but that seems like something we should do anyway. Back then I was wondering about using tyepedef to emulate static assert that works both in file and block scope, but that struggles with needing unique names. FWIW, the perl5 implementation has precisely that problem. If it's used in multiple headers (or a header and a .c file), two static asserts may not be on the same line... - which one will only notice when using an old compiler. I wonder if defining the fallback static assert code to something like extern void static_assert_func(int static_assert_failed[(condition) ? 1 : -1]); isn't a solution, however. I *think* that's standard C. Seems to work at least with gcc, clang, msvc, icc. Re standard: C99's "6.7 Declarations" + 6.7.1 defines 'declaration' to include extern specifiers and in 6.7.1 5) says "The declaration of an identifier for a function that has block scope shall have no explicit storage-class specifier other than extern.". And "6.8 Statements and blocks", via "6.8.2 Compound statement" allows declarations in statements. You can play with a good few compilers at: https://godbolt.org/z/fl0Mzu > The goal was to leave all existing Postgres static assert macros unchanged, but to allow static asserts to be added inthe code at file scope without the need for the explicit ct_asserts function. It'd probably worthwhile to move many of the current ones. > Notice, in reality the StaticAssertDecl macro still uses a static function as a wrapper for the StaticAssertStmt, butnow the function is not visible in the source. I think this implementation is not ok, due to the unique-name issue. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2019-09-19 04:46:27 +0000, Smith, Peter wrote: >> In the attached patch example I have defined a new macro >> StaticAssertDecl. A typical usage of it is shown in the relpath.c >> file. > I'm in favor of adding that - in fact, when I was working on adding a a > static_assert wrapper, I was advocating for only supporting compilers > that know static_assert, so we can put these in the global scope. The > number of compilers that don't support static_assert is pretty small > today, especially compared to 2012, when we added these. > https://www.postgresql.org/message-id/E1TIW1p-0002yE-AY%40gemulon.postgresql.org > https://www.postgresql.org/message-id/27446.1349024252%40sss.pgh.pa.us > Tom, you were arguing for restricting to file scope to get broader > compatibility, are you ok with adding a seperate *Decl version? It could use another look now that we require C99. I'd be in favor of having a decl-level static assert if practical. regards, tom lane
Hi, On September 30, 2019 10:20:54 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Andres Freund <andres@anarazel.de> writes: >> On 2019-09-19 04:46:27 +0000, Smith, Peter wrote: >>> In the attached patch example I have defined a new macro >>> StaticAssertDecl. A typical usage of it is shown in the relpath.c >>> file. > >> I'm in favor of adding that - in fact, when I was working on adding a >a >> static_assert wrapper, I was advocating for only supporting compilers >> that know static_assert, so we can put these in the global scope. The >> number of compilers that don't support static_assert is pretty small >> today, especially compared to 2012, when we added these. >> >https://www.postgresql.org/message-id/E1TIW1p-0002yE-AY%40gemulon.postgresql.org >> >https://www.postgresql.org/message-id/27446.1349024252%40sss.pgh.pa.us >> Tom, you were arguing for restricting to file scope to get broader >> compatibility, are you ok with adding a seperate *Decl version? > >It could use another look now that we require C99. I'd be in favor >of having a decl-level static assert if practical. What do you think about my proposal further down in the email to rely on extern function declarations to have one fallbackthat works in the relevant scopes (not in expressions, but we already treat that differently)? Seems to work on commoncompilers and seems standard conform? Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
From: Andres Freund <andres@anarazel.de> Sent: Tuesday, 1 October 2019 3:14 AM >I wonder if defining the fallback static assert code to something like > extern void static_assert_func(int static_assert_failed[(condition) ? 1 : -1]); isn't a solution, however. I *think* that'sstandard C. Seems to work at least with gcc, clang, msvc, icc. > >Re standard: C99's "6.7 Declarations" + 6.7.1 defines 'declaration' to include extern specifiers and in 6.7.1 5) says "Thedeclaration of an identifier for a function that has block scope shall have >no explicit storage-class specifier otherthan extern.". And "6.8 Statements and blocks", via "6.8.2 Compound statement" allows declarations in statements. > >You can play with a good few compilers at: https://godbolt.org/z/fl0Mzu I liked your idea of using an extern function declaration for implementing the file-scope compile-time asserts. AFAIK itis valid standard C. Thank you for the useful link to that compiler explorer. I tried many scenarios of the new StaticAssertDecl and all seemedto work ok. https://godbolt.org/z/fDrmXi The patch has been updated accordingly. All assertions identified in the original post are now adjacent the global variablesthey are asserting. Kind Regards -- Peter Smith Fujitsu Australia
Attachment
On 2019-10-10 00:52, Smith, Peter wrote: > I liked your idea of using an extern function declaration for implementing the file-scope compile-time asserts. AFAIK itis valid standard C. > > Thank you for the useful link to that compiler explorer. I tried many scenarios of the new StaticAssertDecl and all seemedto work ok. > https://godbolt.org/z/fDrmXi > > The patch has been updated accordingly. All assertions identified in the original post are now adjacent the global variablesthey are asserting. > The problem with this implementation is that you get a crappy error message when the assertion fails, namely something like: ../../../../src/include/c.h:862:84: error: size of array 'static_assert_failure' is negative Ideally, the implementation should end up calling _Static_assert() somehow, so that we get the compiler's native error message. We could do a configure check for whether _Static_assert() works at file scope. I don't know what the support for that is, but it seems to work in gcc and clang. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On October 26, 2019 6:06:07 AM PDT, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: >On 2019-10-10 00:52, Smith, Peter wrote: >> I liked your idea of using an extern function declaration for >implementing the file-scope compile-time asserts. AFAIK it is valid >standard C. >> >> Thank you for the useful link to that compiler explorer. I tried many >scenarios of the new StaticAssertDecl and all seemed to work ok. >> https://godbolt.org/z/fDrmXi >> >> The patch has been updated accordingly. All assertions identified in >the original post are now adjacent the global variables they are >asserting. >> > >The problem with this implementation is that you get a crappy error >message when the assertion fails, namely something like: > >../../../../src/include/c.h:862:84: error: size of array >'static_assert_failure' is negative My proposal for this really was just to use this as a fallback for when static assert isn't available. Which in turn I wasjust suggesting because Tom wanted a fallback. >Ideally, the implementation should end up calling _Static_assert() >somehow, so that we get the compiler's native error message. > >We could do a configure check for whether _Static_assert() works at >file >scope. I don't know what the support for that is, but it seems to work >in gcc and clang. I think it should work everywhere that has static assert. So we should need a separate configure check. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
From: Andres Freund <andres@anarazel.de> Sent: Sunday, 27 October 2019 12:03 PM >>Ideally, the implementation should end up calling _Static_assert() >>somehow, so that we get the compiler's native error message. OK. I can work on that. >>We could do a configure check for whether _Static_assert() works at >>file scope. I don't know what the support for that is, but it seems to >>work in gcc and clang > I think it should work everywhere that has static assert. So we should need a separate configure check. Er, that's a typo right? I think you meant: "So we *shouldn't* need a separate configure check" Kind Regards --- Peter Smith Fujitsu Australia
On 2019-10-27 11:44:54 +0000, Smith, Peter wrote: > From: Andres Freund <andres@anarazel.de> Sent: Sunday, 27 October 2019 12:03 PM > > I think it should work everywhere that has static assert. So we should need a separate configure check. > > Er, that's a typo right? I think you meant: "So we *shouldn't* need a separate configure check" Yes.
From: Andres Freund <andres@anarazel.de> Sent: Sunday, 27 October 2019 12:03 PM > My proposal for this really was just to use this as a fallback for when static assert isn't available. Which in turn Iwas just suggesting because Tom wanted a fallback. The patch is updated to use "extern" technique only when "_Static_assert" is unavailable. PSA. Kind Regards, --- Peter Smith Fujitsu Australia
Attachment
Hello. At Mon, 28 Oct 2019 00:30:11 +0000, "Smith, Peter" <peters@fast.au.fujitsu.com> wrote in > From: Andres Freund <andres@anarazel.de> Sent: Sunday, 27 October 2019 12:03 PM > > My proposal for this really was just to use this as a fallback for when static assert isn't available. Which in turnI was just suggesting because Tom wanted a fallback. > > The patch is updated to use "extern" technique only when "_Static_assert" is unavailable. > > PSA. It is missing the __cplusplus case? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Sent: Monday, 28 October 2019 1:26 PM > It is missing the __cplusplus case? My use cases for the macro are only in C code, so that's all I was interested in at this time. If somebody else wants to extend the patch for C++ also (and test it) they can do. Kind Regards, --- Peter Smith Fujitsu Australia
On Mon, Oct 28, 2019 at 03:42:02AM +0000, Smith, Peter wrote: > From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Sent: Monday, 28 October 2019 1:26 PM >> It is missing the __cplusplus case? > > My use cases for the macro are only in C code, so that's all I was interested in at this time. > If somebody else wants to extend the patch for C++ also (and test it) they can do. It seems to me that there is a good point to be consistent with the treatment of StaticAssertStmt and StaticAssertExpr in c.h, which have fallback implementations in *all* the configurations supported. @@ -858,7 +863,6 @@ extern void ExceptionalCondition(const char *conditionName, #endif #endif /* C++ */ - /* A nit: noise diffs. (No need to send a new version just for that.) -- Michael
Attachment
Hi Peter, Peter, :)
On 2019-10-28 00:30:11 +0000, Smith, Peter wrote:
> From: Andres Freund <andres@anarazel.de> Sent: Sunday, 27 October 2019 12:03 PM
> > My proposal for this really was just to use this as a fallback for when static assert isn't available. Which in
turnI was just suggesting because Tom wanted a fallback.
>
> The patch is updated to use "extern" technique only when "_Static_assert" is unavailable.
Cool.
> /*
> * forkname_to_number - look up fork number by name
> diff --git a/src/include/c.h b/src/include/c.h
> index d752cc0..3e24ff4 100644
> --- a/src/include/c.h
> +++ b/src/include/c.h
> @@ -838,11 +838,16 @@ extern void ExceptionalCondition(const char *conditionName,
> do { _Static_assert(condition, errmessage); } while(0)
> #define StaticAssertExpr(condition, errmessage) \
> ((void) ({ StaticAssertStmt(condition, errmessage); true; }))
> +/* StaticAssertDecl is suitable for use at file scope. */
> +#define StaticAssertDecl(condition, errmessage) \
> + _Static_assert(condition, errmessage)
> #else /* !HAVE__STATIC_ASSERT */
> #define StaticAssertStmt(condition, errmessage) \
> ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
> #define StaticAssertExpr(condition, errmessage) \
> StaticAssertStmt(condition, errmessage)
> +#define StaticAssertDecl(condition, errmessage) \
> + extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
> #endif /* HAVE__STATIC_ASSERT */
> #else /* C++ */
> #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
> @@ -858,7 +863,6 @@ extern void ExceptionalCondition(const char *conditionName,
> #endif
> #endif /* C++
> */
Peter Smith:
Is there a reason to not just make StaticAssertExpr and StaticAssertStmt
be the same? I don't want to proliferate variants that users have to
understand if there's no compelling need. Nor do I think do we really
need two different fallback implementation for static asserts.
As far as I can tell we should be able to use the prototype based
approach in all the cases where we currently use the "negative bit-field
width" approach?
Should then also update
* Otherwise we fall back on a kluge that assumes the compiler will complain
* about a negative width for a struct bit-field. This will not include a
* helpful error message, but it beats not getting an error at all.
Peter Eisentraut:
Looking at the cplusplus variant, I'm somewhat surprised to see that you
made both fallback and plain version unconditionally use GCC style
compound expressions:
commit a2c8e5cfdb9d82ae6d4bb8f37a4dc7cbeca63ec1
Author: Peter Eisentraut <peter_e@gmx.net>
Date: 2016-08-30 12:00:00 -0400
Add support for static assertions in C++
...
+#if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
+#define StaticAssertStmt(condition, errmessage) \
+ static_assert(condition, errmessage)
+#define StaticAssertExpr(condition, errmessage) \
+ StaticAssertStmt(condition, errmessage)
+#else
+#define StaticAssertStmt(condition, errmessage) \
+ do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
+#define StaticAssertExpr(condition, errmessage) \
+ ({ StaticAssertStmt(condition, errmessage); })
+#endif
Was that intentional? The C version intentionally uses compound
expressions only for the _Static_assert case, where configure tests for
the compound expression support? As far as I can tell this'll not allow
using our headers e.g. with msvc in C++ mode if somebody introduce a
static assertion in a header - which seems like a likely and good
outcome with the changes proposed here?
Btw, it looks to me like msvc supports using the C++ static_assert()
even in C mode:
https://godbolt.org/z/b_dxDW
Greetings,
Andres Freund
From: Andres Freund <andres@anarazel.de> Sent: Wednesday, 13 November 2019 6:01 AM >Peter Smith: > > Is there a reason to not just make StaticAssertExpr and StaticAssertStmt be the same? I don't want to proliferate variantsthat users have to understand if there's no compelling > need. Nor do I think do we really need two different fallback implementation for static asserts. > > As far as I can tell we should be able to use the prototype based approach in all the cases where we currently use the"negative bit-field width" approach? I also thought that the new "prototype negative array-dimension" based approach (i.e. StaticAssertDecl) looked like an improvementover the existing "negative bit-field width" approach (i.e. StaticAssertStmt), because it seems to work for morescenarios (e.g. file scope). But I did not refactor existing code to use the new way because I was fearful that there might be some subtle reason whythe StaticAssertStmt was deliberately made that way (e.g. as do/while), and last thing I want to do was break workingcode. > Should then also update > * Otherwise we fall back on a kluge that assumes the compiler will complain > * about a negative width for a struct bit-field. This will not include a > * helpful error message, but it beats not getting an error at all. Kind Regards. Peter Smith --- Fujitsu Australia
On 2019-11-12 20:00, Andres Freund wrote: > Looking at the cplusplus variant, I'm somewhat surprised to see that you > made both fallback and plain version unconditionally use GCC style > compound expressions: > Was that intentional? The C version intentionally uses compound > expressions only for the _Static_assert case, where configure tests for > the compound expression support? As far as I can tell this'll not allow > using our headers e.g. with msvc in C++ mode if somebody introduce a > static assertion in a header - which seems like a likely and good > outcome with the changes proposed here? I don't recall all the details anymore, but if you're asking, why is the fallback implementation in C++ different from the one in C, then that's because the C variant didn't work in C++. I seem to recall that I did this work in order to get an actual C++-using extension to compile, so it worked(tm) at some point, but I probably didn't try it with a not-gcc compatible compiler at the time. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019-11-13 03:23:06 +0000, Smith, Peter wrote: > From: Andres Freund <andres@anarazel.de> Sent: Wednesday, 13 November 2019 6:01 AM > > >Peter Smith: > > > > Is there a reason to not just make StaticAssertExpr and StaticAssertStmt be the same? I don't want to proliferate variantsthat users have to understand if there's no compelling > > need. Nor do I think do we really need two different fallback implementation for static asserts. > > > > > As far as I can tell we should be able to use the prototype based approach in all the cases where we currently use the"negative bit-field width" approach? > > I also thought that the new "prototype negative array-dimension" based > approach (i.e. StaticAssertDecl) looked like an improvement over the > existing "negative bit-field width" approach (i.e. StaticAssertStmt), > because it seems to work for more scenarios (e.g. file scope). > > But I did not refactor existing code to use the new way because I was > fearful that there might be some subtle reason why the > StaticAssertStmt was deliberately made that way (e.g. as do/while), > and last thing I want to do was break working code. That'll just leave us with cruft. And it's not like this stuff will break in a subtle way or such.... Greetings, Andres Freund
Hi Andres,
>>> As far as I can tell we should be able to use the prototype based approach in all the cases where we currently use
the"negative bit-field width" approach?
>> ...
>> But I did not refactor existing code to use the new way because I was
>> fearful that there might be some subtle reason why the
>> StaticAssertStmt was deliberately made that way (e.g. as do/while),
>> and last thing I want to do was break working code.
>That'll just leave us with cruft. And it's not like this stuff will break in a subtle way or such....
FYI - I did try, per your suggestion, to replace the existing StaticAssertStmt to also use the same fallback "extern"
syntaxform as the new StaticAssertDecl, but the code broke as I suspected it might do:
====================
path.c: In function 'first_dir_separator':
../../src/include/c.h:847:2: error: expected expression before 'extern'
extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
^
../../src/include/c.h:849:2: note: in expansion of macro 'StaticAssertStmt'
StaticAssertStmt(condition, errmessage)
^
../../src/include/c.h:1184:3: note: in expansion of macro 'StaticAssertExpr'
(StaticAssertExpr(__builtin_types_compatible_p(__typeof(expr), const underlying_type), \
^
path.c:127:11: note: in expansion of macro 'unconstify'
return unconstify(char *, p);
^
====================
Kind Regards.
---
Peter Smith
Fujitsu Australia
> It seems to me that there is a good point to be consistent with the treatment of StaticAssertStmt and StaticAssertExprin c.h, which have fallback implementations in *all* the configurations supported. Consistency is good, but: * That is beyond the scope for what I wanted my patch to achieve; my use-cases are C code only * It is too risky for me to simply cut/paste my C version of StaticAssertDecl and hope it will work OK for C++. It needslots of testing because there seems evidence that bad things can happen. E.g. Peter Eisentraut wrote "if you're asking,why is the fallback implementation in C++ different from the one in C, then that's because the C variant didn't workin C++." ~ I am happy if somebody else with more ability to test C++ properly wants to add the __cplusplus variant of the new macro. Meanwhile, I've attached latest re-based version of this patch. Kind Regards. -- Peter Smith Fujitsu Australia
Attachment
On Wed, Nov 27, 2019 at 12:23:33PM +0000, Smith, Peter wrote: > * That is beyond the scope for what I wanted my patch to achieve; my > * use-cases are C code only. Well, FWIW, I do have some extensions using __cplusplus and I am pretty sure that I am not the only one with that. The thing is that with your patch folks would not get any compilation failures *now* because all the declarations of StaticAssertDecl() are added within the .c files, but once a patch which includes a declaration in a header, something very likely to happen, is merged then we head into breaking suddenly the compilation of those modules. And that's not nice. That's also a point raised by Andres upthread. > I am happy if somebody else with more ability to test C++ properly > wants to add the __cplusplus variant of the new macro. In short, attached is an updated version of your patch which attempts to solve that. I have tested this with some cplusplus stuff, and GCC for both versions (static_assert is available in GCC >= 6, but a manual change of c.h does the trick). I have edited the patch a bit while on it, your assertions did not use project-style grammar, the use of parenthesis was inconsistent (see relpath.c for example), and pgindent has complained a bit. Also, I am bumping the patch to next CF for now. Do others have thoughts to share about this version? I would be actually fine to commit that, even if the message generated for the fallback versions is a bit crappy with a complain about a negative array size, but that's not new to this patch as we use that as well with StaticAssertStmt(). -- Michael
Attachment
Hi,
On 2019-11-29 11:11:25 +0900, Michael Paquier wrote:
> On Wed, Nov 27, 2019 at 12:23:33PM +0000, Smith, Peter wrote:
> > * That is beyond the scope for what I wanted my patch to achieve; my
> > * use-cases are C code only.
I really don't think that's justification enough for having diverging
implementations, nor imcomplete coverage. Following that chain of
arguments we'd just end up with more and more cruft, without ever
actually cleaning anything up.
> diff --git a/src/include/c.h b/src/include/c.h
> index 00e41ac546..91d6d50e76 100644
> --- a/src/include/c.h
> +++ b/src/include/c.h
> @@ -845,11 +845,16 @@ extern void ExceptionalCondition(const char *conditionName,
> do { _Static_assert(condition, errmessage); } while(0)
> #define StaticAssertExpr(condition, errmessage) \
> ((void) ({ StaticAssertStmt(condition, errmessage); true; }))
> +/* StaticAssertDecl is suitable for use at file scope. */
> +#define StaticAssertDecl(condition, errmessage) \
> + _Static_assert(condition, errmessage)
> #else /* !HAVE__STATIC_ASSERT */
> #define StaticAssertStmt(condition, errmessage) \
> ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
> #define StaticAssertExpr(condition, errmessage) \
> StaticAssertStmt(condition, errmessage)
> +#define StaticAssertDecl(condition, errmessage) \
> + extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
> #endif /* HAVE__STATIC_ASSERT */
I think this a) needs an updated comment above, explaining this approach
(note the explanation for the array approach) b) I still think we ought
to work towards also using this implementation for StaticAssertStmt.
Now that I'm back from vacation, I'll try to take a stab at b). It
should definitely doable to use the same approach for StaticAssertStmt,
the problematic case might be StaticAssertExpr.
> #else /* C++ */
> #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
> @@ -857,12 +862,16 @@ extern void ExceptionalCondition(const char *conditionName,
> static_assert(condition, errmessage)
> #define StaticAssertExpr(condition, errmessage) \
> ({ static_assert(condition, errmessage); })
> -#else
> +#define StaticAssertDecl(condition, errmessage) \
> + static_assert(condition, errmessage)
> +#else /* !__cpp_static_assert */
> #define StaticAssertStmt(condition, errmessage) \
> do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
> #define StaticAssertExpr(condition, errmessage) \
> ((void) ({ StaticAssertStmt(condition, errmessage); }))
> -#endif
> +#define StaticAssertDecl(condition, errmessage) \
> + extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
> +#endif /* __cpp_static_assert */
> #endif /* C++ */
I wonder if it's worth moving the fallback implementation into an else
branch that's "unified" between C and C++.
> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
> + "LockTagTypeNames array inconsistency");
> +
These error messages strike me as somewhat unhelpful. I'd probably just
reword them as "array length mismatch" or something like that.
I think this patch ought to include at least one StaticAssertDecl in a
header, to make sure we get that part right across compilers. E.g. the
one in PageIsVerified()?
Greetings,
Andres Freund
On Mon, Dec 02, 2019 at 07:55:45AM -0800, Andres Freund wrote:
> On 2019-11-29 11:11:25 +0900, Michael Paquier wrote:
>> diff --git a/src/include/c.h b/src/include/c.h
>> index 00e41ac546..91d6d50e76 100644
>> --- a/src/include/c.h
>> +++ b/src/include/c.h
>> [...]
>
> I think this a) needs an updated comment above, explaining this approach
> (note the explanation for the array approach) b) I still think we ought
> to work towards also using this implementation for StaticAssertStmt.
Sure. I was not completely sure which addition would be helpful
except than adding in the main comment lock that Decl() is useful at
file scope.
> Now that I'm back from vacation, I'll try to take a stab at b). It
> should definitely doable to use the same approach for StaticAssertStmt,
> the problematic case might be StaticAssertExpr.
So you basically want to minimize the amount of code relying on
external compiler expressions? Sounds like a plan. At quick glance,
it seems that this should work. I haven't tested though. I'll wait
for what you come up with then.
>> #else /* C++ */
>> #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
>> @@ -857,12 +862,16 @@ extern void ExceptionalCondition(const char *conditionName,
>> static_assert(condition, errmessage)
>> #define StaticAssertExpr(condition, errmessage) \
>> ({ static_assert(condition, errmessage); })
>>
>> [...]
>>
>> +#define StaticAssertDecl(condition, errmessage) \
>> + extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
>> +#endif /* __cpp_static_assert */
>> #endif /* C++ */
>
> I wonder if it's worth moving the fallback implementation into an else
> branch that's "unified" between C and C++.
I suspect that you would run into problems with StaticAssertExpr() and
StaticAssertStmt().
>> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
>> + "LockTagTypeNames array inconsistency");
>> +
>
> These error messages strike me as somewhat unhelpful. I'd probably just
> reword them as "array length mismatch" or something like that.
That's indeed better. Now I think that it is useful to have the
structure name in the error message as well, no?
> I think this patch ought to include at least one StaticAssertDecl in a
> header, to make sure we get that part right across compilers. E.g. the
> one in PageIsVerified()?
No objections to have one, but I don't think that your suggestion is a
good choice. This static assertion is based on size_t and BLCKSZ, and
is located close to a code path where we have a specific logic based
on both things. If in the future this code gets removed, then we'd
likely miss to remove the static assertion if they are separated
across multiple files.
--
Michael
Attachment
On 2019-12-02 16:55, Andres Freund wrote: >> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1), >> + "LockTagTypeNames array inconsistency"); >> + > These error messages strike me as somewhat unhelpful. I'd probably just > reword them as "array length mismatch" or something like that. I'd prefer it if we could just get rid of the second argument and show the actual expression in the error message, like run-time assertions work. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-12-04 15:16:25 +0900, Michael Paquier wrote: > On Mon, Dec 02, 2019 at 07:55:45AM -0800, Andres Freund wrote: > > Now that I'm back from vacation, I'll try to take a stab at b). It > > should definitely doable to use the same approach for StaticAssertStmt, > > the problematic case might be StaticAssertExpr. > > So you basically want to minimize the amount of code relying on > external compiler expressions? Sounds like a plan. At quick glance, > it seems that this should work. I haven't tested though. I'll wait > for what you come up with then. I don't know what you mean by "external compiler expressions"? > >> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1), > >> + "LockTagTypeNames array inconsistency"); > >> + > > > > These error messages strike me as somewhat unhelpful. I'd probably just > > reword them as "array length mismatch" or something like that. > > That's indeed better. Now I think that it is useful to have the > structure name in the error message as well, no? No. I think the cost of having the different error messages is much higher than the cost of not having the struct name in there. Note that you'll commonly get an error message including the actual source code for the offending expression. > > I think this patch ought to include at least one StaticAssertDecl in a > > header, to make sure we get that part right across compilers. E.g. the > > one in PageIsVerified()? > > No objections to have one, but I don't think that your suggestion is a > good choice. This static assertion is based on size_t and BLCKSZ, and > is located close to a code path where we have a specific logic based > on both things. Well, but it's a reliance that goes beyond that specific source code location > If in the future this code gets removed, then we'd likely miss to > remove the static assertion if they are separated across multiple > files. It'll never get removed. There's just plainly no way that we'd use a block size that's not a multiple of size_t. Greetings, Andres Freund
Hi, On 2019-12-04 10:09:28 +0100, Peter Eisentraut wrote: > On 2019-12-02 16:55, Andres Freund wrote: > > > +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1), > > > + "LockTagTypeNames array inconsistency"); > > > + > > These error messages strike me as somewhat unhelpful. I'd probably just > > reword them as "array length mismatch" or something like that. > > I'd prefer it if we could just get rid of the second argument and show the > actual expression in the error message, like run-time assertions work. Well, _Static_assert has an error message, so we got to pass something. And having something like "array length mismatch", without referring again to the variable, doesn't strike me as that bad. We could of course just again pass the expression, this time stringified, but that doesn't seem an improvement. Greetings, Andres Freund
-----Original Message-----
From: Andres Freund <andres@anarazel.de> Sent: Tuesday, 3 December 2019 2:56 AM
> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
> + "LockTagTypeNames array inconsistency");
> +
>
>These error messages strike me as somewhat unhelpful. I'd probably just reword them as "array length mismatch" or
somethinglike that.
OK. I have no problem to modify all my current assertion messages to your suggested text ("array length mismatch") if
youthink it is better.
Please correct me if I am wrong, but I didn't think the error message text is of very great significance here because
itis a compile-time issue meaning the *only* person who would see the message is the 1 developer who accidentally
introduceda bug just moments beforehand. The compile will fail with a source line number, and when the developer sees
theStaticAssertDecl at that source line the cause of the error is anyway self-evident by the condition parameter.
Kind Regards
--
Peter Smith
Fujitsu Australia
Hello Michael, > In short, attached is an updated version of your patch which attempts to solve that. I have tested this with some cplusplusstuff, and GCC for both versions (static_assert is available in GCC >= 6, but a manual change of c.h does the trick). > I have edited the patch a bit while on it, your assertions did not use project-style grammar, the use of parenthesis wasinconsistent (see relpath.c for example), and pgindent has complained a bit. Thanks for your updates. ~~ Hello Andres, >> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1), >> + "LockTagTypeNames array inconsistency"); >> + > These error messages strike me as somewhat unhelpful. I'd probably just reword them as "array length mismatch" or somethinglike that. I updated the most recent patch (_5 from Michael) so it now has your suggested error message rewording. PSA patch _6 Kind Regards ---- Peter Smith Fujitsu Australia
Attachment
On Wed, Dec 04, 2019 at 09:54:47AM -0800, Andres Freund wrote: > Well, _Static_assert has an error message, so we got to pass > something. And having something like "array length mismatch", without > referring again to the variable, doesn't strike me as that bad. We could > of course just again pass the expression, this time stringified, but > that doesn't seem an improvement. Yeah, I would rather keep the second argument. I think that's also more helpful as it gives more flexibility to extension authors willing to make use of it. -- Michael
Attachment
On Fri, Dec 20, 2019 at 01:08:47AM +0000, Smith, Peter wrote: > I updated the most recent patch (_5 from Michael) so it now has your > suggested error message rewording. Hmm. Based on the last messages, and this one in particular: https://www.postgresql.org/message-id/20191202155545.yzbfzuppjritidqr@alap3.anarazel.de I am still seeing that a couple of points need an extra lookup: - Addition of a Decl() in at least one header of the core code. - Perhaps unifying the fallback implementation between C and C++, with a closer lookup in particular at StaticAssertStmt() and StaticAssertExpr(). Peter, could you look at that? In order to test the C++ portion, you could use this dummy C++ extension I have just published on my github account: https://github.com/michaelpq/pg_plugins/tree/master/blackhole_cplusplus That's a dirty 15-min hack, but that would be enough to check the C++ part with PGXS. For now, I am switching the patch as waiting on author for now. -- Michael
Attachment
On Tue, Dec 24, 2019 at 02:47:29PM +0900, Michael Paquier wrote: > I am still seeing that a couple of points need an extra lookup: > - Addition of a Decl() in at least one header of the core code. I agree with the addition of Decl() definition in a header, and could not think about something better than one for bufpage.h for the all-zero check case, so I went with that. Attached is a 0001 which adds the definition for StaticAssertDecl() for C and C++ for all code paths. If there are no objections, I would like to commit this version. There is no fancy refactoring in it, and small progress is better than no progress. I have also reworked the comments in the patch, and did some testing on Windows. > - Perhaps unifying the fallback implementation between C and C++, with > a closer lookup in particular at StaticAssertStmt() and StaticAssertExpr(). Seeing nothing happening on this side. I took a shot at all that, and I have hacked my way through it with 0002 which is an attempt to unify the fallback implementation for C and C++. This is not fully baked yet, and it is perhaps a matter of taste if this makes the code more readable or not. I think it does, because it reduces the parts dedicated to assertion definitions from four to three. Anyway, let's discuss about that. -- Michael
Attachment
At Fri, 31 Jan 2020 11:47:01 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Tue, Dec 24, 2019 at 02:47:29PM +0900, Michael Paquier wrote: > > I am still seeing that a couple of points need an extra lookup: > > - Addition of a Decl() in at least one header of the core code. > > I agree with the addition of Decl() definition in a header, and could > not think about something better than one for bufpage.h for the > all-zero check case, so I went with that. Attached is a 0001 which > adds the definition for StaticAssertDecl() for C and C++ for all code > paths. If there are no objections, I would like to commit this > version. There is no fancy refactoring in it, and small progress is > better than no progress. I have also reworked the comments in the > patch, and did some testing on Windows. As a cross check, it cleanly applied and worked as expected. The fallback definition of StaticAssertDecl for C worked for gcc 8.3. - * Macros to support compile-time assertion checks. + * Macros to support compile-time and declaration assertion checks. All the StaticAssert things check compile-time assertion. I think that the name StaticAssertDecl doesn't mean "declaration assertion", but means "static assertion as a declaration". Is the change needed? - * If the "condition" (a compile-time-constant expression) evaluates to false, - * throw a compile error using the "errmessage" (a string literal). + * If the "condition" (a compile-time-constant or declaration expression) + * evaluates to false, throw a compile error using the "errmessage" (a + * string literal). I'm not sure what the "declaration expression" here means. I think the term generally means "a variable declaration in expressions", something like "r = y * (int x = blah)". In that sense, the parameter for StaticAssertDecl is just a compile-time constant expression. Is it a right rewrite? > > - Perhaps unifying the fallback implementation between C and C++, with > > a closer lookup in particular at StaticAssertStmt() and StaticAssertExpr(). > > Seeing nothing happening on this side. I took a shot at all that, and > I have hacked my way through it with 0002 which is an attempt to unify > the fallback implementation for C and C++. This is not fully baked > yet, and it is perhaps a matter of taste if this makes the code more > readable or not. I think it does, because it reduces the parts > dedicated to assertion definitions from four to three. Anyway, let's > discuss about that. +1 as far as the unification is right. I'm not sure, but at least it worked for gcc 8.3. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Fri, Jan 31, 2020 at 02:15:42PM +0900, Kyotaro Horiguchi wrote: > As a cross check, it cleanly applied and worked as expected. The > fallback definition of StaticAssertDecl for C worked for gcc 8.3. Thanks for the review. > - * Macros to support compile-time assertion checks. > + * Macros to support compile-time and declaration assertion checks. > > All the StaticAssert things check compile-time assertion. I think > that the name StaticAssertDecl doesn't mean "declaration assertion", > but means "static assertion as a declaration". Is the change needed? Hmm. Yeah, that sounds right. > - * If the "condition" (a compile-time-constant expression) evaluates to false, > - * throw a compile error using the "errmessage" (a string literal). > + * If the "condition" (a compile-time-constant or declaration expression) > + * evaluates to false, throw a compile error using the "errmessage" (a > + * string literal). > > I'm not sure what the "declaration expression" here means. I think > the term generally means "a variable declaration in expressions", > something like "r = y * (int x = blah)". In that sense, the parameter > for StaticAssertDecl is just a compile-time constant expression. Is it > a right rewrite? Actually, thinking more about it, I'd rather remove this part as well, keeping only the change in the third paragraph of this comment block. -- Michael
Attachment
On Fri, Jan 31, 2020 at 02:46:16PM +0900, Michael Paquier wrote: > Actually, thinking more about it, I'd rather remove this part as well, > keeping only the change in the third paragraph of this comment block. I have tweaked a bit the comments in this area, and applied the patch. I'll begin a new thread with the rest of the refactoring. There are a couple of things I'd like to double-check first. -- Michael
Attachment
Hi Michael, Sorry I was AWOL for a couple of months. Thanks for taking the patch further, and committing it. Kind Regards --- Peter Smith Fujitsu Australia
On Tue, Mar 10, 2020 at 12:22:32AM +0000, Smith, Peter wrote: > Sorry I was AWOL for a couple of months. Thanks for taking the patch > further, and committing it. No problem, I am glad to see you around. -- Michael