Proposal: Add more compile-time asserts to expose inconsistencies.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
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

0001-Add-compile-time-asserts.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Dagfinn Ilmari Mannsåker
Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

RE: Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
In reply to this post by Michael Paquier-2
-----Original Message-----
From: Michael Paquier <[hidden email]>  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.  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/

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

add_more_ct_asserts.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Michael Paquier-2
In reply to this post by Dagfinn Ilmari Mannsåker
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Kyotaro Horiguchi-4
At Thu, 19 Sep 2019 10:07:40 +0900, Michael Paquier <[hidden email]> wrote in <[hidden email]>
> 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
Reply | Threaded
Open this post in threaded view
|

RE: Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
In reply to this post by Michael Paquier-2
-----Original Message-----
From: Michael Paquier <[hidden email]> 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.c file.

The goal was to leave all existing Postgres static assert macros unchanged, but to allow static asserts to be added in the code 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 now the 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 put each StaticAssertDecl nearby the array that it is asserting (e.g. just like relpath.c)

Kind Regards,
Peter Smith
---
Fujitsu Australia

add_more_ct_asserts_StaticAssertDecl.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Andres Freund
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 in the 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,  but now the function is not visible in the source.

I think this implementation is not ok, due to the unique-name issue.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Tom Lane-2
Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Andres Freund
Hi,

On September 30, 2019 10:20:54 AM PDT, Tom Lane <[hidden email]> wrote:

>Andres Freund <[hidden email]> 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 fallback that works in the relevant scopes (not in expressions, but we already treat that differently)? Seems to work on common compilers and seems standard conform?

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.


Reply | Threaded
Open this post in threaded view
|

RE: Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
In reply to this post by Andres Freund
From: Andres Freund <[hidden email]> 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'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

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.

Kind Regards
--
Peter Smith
Fujitsu Australia

ct_asserts_StaticAssertDecl_2.patch (9K) Download Attachment