Refactor compile-time assertion checks for C/C++

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

Refactor compile-time assertion checks for C/C++

Michael Paquier-2
Hi all,

As of [1], I have been playing with the compile time assertions that
we have for expressions, declarations and statements.  And it happens
that it is visibly possible to consolidate the fallback
implementations for C and C++.  Attached is the result of what I am
getting at.  I am adding this patch to next CF.  Thoughts are
welcome.

[1]: https://www.postgresql.org/message-id/201DD0641B056142AC8C6645EC1B5F62014B8E8030@SYD1217

Thanks,
--
Michael

0001-Refactor-assertion-definitions-in-c.h.patch (2K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Georgios Kokolatos
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

In my humble opinion the patch improves readability, hence gets my +1.

No further suggestions. Passing on to a committer to judge further.

The new status of this patch is: Ready for Committer
Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Tom Lane-2
In reply to this post by Michael Paquier-2
Michael Paquier <[hidden email]> writes:
> As of [1], I have been playing with the compile time assertions that
> we have for expressions, declarations and statements.  And it happens
> that it is visibly possible to consolidate the fallback
> implementations for C and C++.  Attached is the result of what I am
> getting at.  I am adding this patch to next CF.  Thoughts are
> welcome.

cfbot reports this doesn't work with MSVC.  Not sure why --- maybe
it defines __cpp_static_assert differently than you're expecting?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Michael Paquier-2
On Sat, Mar 07, 2020 at 04:44:48PM -0500, Tom Lane wrote:
> cfbot reports this doesn't work with MSVC.  Not sure why --- maybe
> it defines __cpp_static_assert differently than you're expecting?

I don't think that's the issue.  The CF bot uses MSVC 12.0 which
refers to the 2013.  __cpp_static_assert being introduced in MSVC
2017, this error is visibly telling us that this environment does not
like the C++ fallback implementation, which is actually what my
previous version of the patch was using (I can reproduce the error
with my MSVC 2015 VM as well).  I think that this points to an error
in the patch: for the refactoring, the fallback implementation of C
and C++ should use the fallback implementation for C that we have
currently on HEAD.

With the updated patch attached, the error goes away for me.  Let's
see what Mr. Robot thinks.  The patch was marked as ready for
committer, I am switching it back to "Needs review".
--
Michael

v2-0001-Refactor-assertion-definitions-in-c.h.patch (2K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Georgios Kokolatos
Thank you for updating the status of the issue.

I have to admit that I completely missed the CF bot. Lesson learned.

For whatever is worth, my previous comment that the patch improves
readability also applies to the updated version of the patch.

The CF bot seems happy now, which means that your assessment as
to the error and fix are correct.

So :+1: from me.
Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Michael Paquier-2
On Wed, Mar 11, 2020 at 12:31:19PM +0000, Georgios Kokolatos wrote:
> For whatever is worth, my previous comment that the patch improves
> readability also applies to the updated version of the patch.

v2 has actually less diffs for the C++ part.

> The CF bot seems happy now, which means that your assessment as
> to the error and fix are correct.

Indeed the bot is happy now.  By looking at the patch, one would note
that what it just does is unifying the fallback "hack-ish"
implementations so as C and C++ use the same thing, which is the
fallback implementation for C of HEAD.  I would prefer hear first from
more people to see if they like this change.  Or not.
--
Michael

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

Re: Refactor compile-time assertion checks for C/C++

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> Indeed the bot is happy now.  By looking at the patch, one would note
> that what it just does is unifying the fallback "hack-ish"
> implementations so as C and C++ use the same thing, which is the
> fallback implementation for C of HEAD.  I would prefer hear first from
> more people to see if they like this change.  Or not.

I looked at this and tried it on an old (non HAVE__STATIC_ASSERT)
gcc version.  Seems to work, but I have a couple cosmetic suggestions:

1. The comment block above this was never updated to mention that
we're now catering for C++ too.  Maybe something like

  * gcc 4.6 and up supports _Static_assert(), but there are bizarre syntactic
  * placement restrictions.  Macros StaticAssertStmt() and StaticAssertExpr()
  * make it safe to use as a statement or in an expression, respectively.
  * The macro StaticAssertDecl() is suitable for use at file scope (outside of
  * any function).
  *
+ * On recent C++ compilers, we can use standard static_assert().
+ *
  * 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.


2. I think you could simplify the #elif to just

#elif defined(__cplusplus) && __cpp_static_assert >= 200410

Per the C standard, an unrecognized identifier in an #if condition
is replaced by zero.  So the condition will come out false as desired
if __cpp_static_assert isn't defined; you don't need to test that
separately.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Michael Paquier-2
On Thu, Mar 12, 2020 at 12:33:21AM -0400, Tom Lane wrote:
> I looked at this and tried it on an old (non HAVE__STATIC_ASSERT)
> gcc version.  Seems to work, but I have a couple cosmetic suggestions:

Thanks for the review.

> 1. The comment block above this was never updated to mention that
> we're now catering for C++ too.  Maybe something like
>
> + * On recent C++ compilers, we can use standard static_assert().
> + *

Sounds fine to me.  Looking here this is present since GCC 4.3:
https://gcc.gnu.org/projects/cxx-status.html#cxx11

For MSVC, actually I was a bit wrong, only the flavor without error
message is supported since VS 2017, and the one we use is much older:
https://docs.microsoft.com/en-us/cpp/cpp/static-assert?view=vs-2015

So, should we add a reference about both in the new comment?  I would
actually not add them, so I have used your suggestion in the attached,
but the comment block above does that for _Static_assert().  Do you
think it is better to add some references to some of those compilers
(say GCC 4.3, MSVC)?  Just stick with your suggestion?  Or stick with
your version and replace the reference to GCC 4.6 with something like
"recent compilers"?

> 2. I think you could simplify the #elif to just
>
> #elif defined(__cplusplus) && __cpp_static_assert >= 200410
>
> Per the C standard, an unrecognized identifier in an #if condition
> is replaced by zero.  So the condition will come out false as desired
> if __cpp_static_assert isn't defined; you don't need to test that
> separately.

Thanks, indeed.
--
Michael

v3-0001-Refactor-assertion-definitions-in-c.h.patch (3K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> So, should we add a reference about both in the new comment?  I would
> actually not add them, so I have used your suggestion in the attached,
> but the comment block above does that for _Static_assert().  Do you
> think it is better to add some references to some of those compilers
> (say GCC 4.3, MSVC)?  Just stick with your suggestion?  Or stick with
> your version and replace the reference to GCC 4.6 with something like
> "recent compilers"?

I don't feel a need to expend a whole lot of sweat there.  The existing
text is fine, it just bugged me that the code deals with three cases
while the comment block only acknowledged two.  So I'd just go with
what you have in v3.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Michael Paquier-2
On Thu, Mar 12, 2020 at 09:43:54AM -0400, Tom Lane wrote:
> I don't feel a need to expend a whole lot of sweat there.  The existing
> text is fine, it just bugged me that the code deals with three cases
> while the comment block only acknowledged two.  So I'd just go with
> what you have in v3.

Thanks, Tom.  I have committed v3 then.
--
Michael

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

Re: Refactor compile-time assertion checks for C/C++

Michael Paquier-2
On Fri, Mar 13, 2020 at 03:12:34PM +0900, Michael Paquier wrote:
> On Thu, Mar 12, 2020 at 09:43:54AM -0400, Tom Lane wrote:
>> I don't feel a need to expend a whole lot of sweat there.  The existing
>> text is fine, it just bugged me that the code deals with three cases
>> while the comment block only acknowledged two.  So I'd just go with
>> what you have in v3.
>
> Thanks, Tom.  I have committed v3 then.

Hmm.  v3 actually broke the C++ fallback of StaticAssertExpr() and
StaticAssertStmt() (v1 did not), a simple fix being something like
the attached.

The buildfarm does not really care about that, but it could for
example by using the only c++ code compiled in the tree in
src/backend/jit/?  That also means that only builds using --with-llvm
with a compiler old enough would trigger that stuff.
--
Michael

cpp-fallback-fix.patch (799 bytes) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> Hmm.  v3 actually broke the C++ fallback of StaticAssertExpr() and
> StaticAssertStmt() (v1 did not), a simple fix being something like
> the attached.

The buildfarm seems happy, so why do you think it's broken?

If we do need to change it, I'd be inclined to just use the do{}
block everywhere, not bothering with the extra #if test.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Michael Paquier-2
On Fri, Mar 13, 2020 at 11:00:33AM -0400, Tom Lane wrote:
> Michael Paquier <[hidden email]> writes:
>> Hmm.  v3 actually broke the C++ fallback of StaticAssertExpr() and
>> StaticAssertStmt() (v1 did not), a simple fix being something like
>> the attached.
>
> The buildfarm seems happy, so why do you think it's broken?

Extensions like the attached don't appreciate it, and we have nothing
like that in core.  Perhaps we could, but it is not really appealing
for all platforms willing to run the tests to require CXX or such..

> If we do need to change it, I'd be inclined to just use the do{}
> block everywhere, not bothering with the extra #if test.

Not sure what you mean here because we cannot use the do{} flavor
either for the C fallback, no?  See for example the definitions of
unconstify() in c.h.
--
Michael

blackhole_cplusplus.tar.gz (1K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> On Fri, Mar 13, 2020 at 11:00:33AM -0400, Tom Lane wrote:
>> If we do need to change it, I'd be inclined to just use the do{}
>> block everywhere, not bothering with the extra #if test.

> Not sure what you mean here because we cannot use the do{} flavor
> either for the C fallback, no?  See for example the definitions of
> unconstify() in c.h.

Sorry for being unclear --- I just meant that we could use do{}
in StaticAssertStmt for both C and C++.  Although now I notice
that the code is trying to use StaticAssertStmt for StaticAssertExpr,
which you're right isn't going to do.  But I think something like
this would work and be a bit simpler than what you proposed:

 #else
 /* Fallback implementation for C and C++ */
 #define StaticAssertStmt(condition, errmessage) \
- ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
+ do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
 #define StaticAssertExpr(condition, errmessage) \
- StaticAssertStmt(condition, errmessage)
+ ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
 #define StaticAssertDecl(condition, errmessage) \


                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Michael Paquier-2
On Mon, Mar 16, 2020 at 10:32:36AM -0400, Tom Lane wrote:

> Sorry for being unclear --- I just meant that we could use do{}
> in StaticAssertStmt for both C and C++.  Although now I notice
> that the code is trying to use StaticAssertStmt for StaticAssertExpr,
> which you're right isn't going to do.  But I think something like
> this would work and be a bit simpler than what you proposed:
>
>  #else
>  /* Fallback implementation for C and C++ */
>  #define StaticAssertStmt(condition, errmessage) \
> - ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
> + do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
>  #define StaticAssertExpr(condition, errmessage) \
> - StaticAssertStmt(condition, errmessage)
> + ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
>  #define StaticAssertDecl(condition, errmessage) \
C++ does not allow defining a struct inside a sizeof() call, so in
this case StaticAssertExpr() does not work with the previous extension
in C++.  StaticAssertStmt() does the work though.

One alternatine I can think of for C++ would be something like the
following, though C does not like this flavor either:
typedef char static_assert_struct[condition ? 1 : -1]
--
Michael

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

Re: Refactor compile-time assertion checks for C/C++

Tom Lane-2
Michael Paquier <[hidden email]> writes:

> On Mon, Mar 16, 2020 at 10:32:36AM -0400, Tom Lane wrote:
>> Sorry for being unclear --- I just meant that we could use do{}
>> in StaticAssertStmt for both C and C++.  Although now I notice
>> that the code is trying to use StaticAssertStmt for StaticAssertExpr,
>> which you're right isn't going to do.  But I think something like
>> this would work and be a bit simpler than what you proposed:
>>
>> #else
>> /* Fallback implementation for C and C++ */
>> #define StaticAssertStmt(condition, errmessage) \
>> - ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
>> + do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
>> #define StaticAssertExpr(condition, errmessage) \
>> - StaticAssertStmt(condition, errmessage)
>> + ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
>> #define StaticAssertDecl(condition, errmessage) \

> C++ does not allow defining a struct inside a sizeof() call, so in
> this case StaticAssertExpr() does not work with the previous extension
> in C++.  StaticAssertStmt() does the work though.

[ scratches head... ]  A do{} is okay in an expression in C++ ??

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Michael Paquier-2
On Mon, Mar 16, 2020 at 10:35:05PM -0400, Tom Lane wrote:
> Michael Paquier <[hidden email]> writes:
>> C++ does not allow defining a struct inside a sizeof() call, so in
>> this case StaticAssertExpr() does not work with the previous extension
>> in C++.  StaticAssertStmt() does the work though.
>
> [ scratches head... ]  A do{} is okay in an expression in C++ ??

cpp-fallback-fix.patch in [1] was doing that.

The fun does not stop here.  gcc is fine when using that for C and
C++:
#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); }))

But then problems come from MSVC which does not like the do{} part for
statements, and this works:
#define StaticAssertStmt(condition, errmessage) \
    ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
#define StaticAssertExpr(condition, errmessage) \
    StaticAssertStmt(condition, errmessage)

[1]: https://postgr.es/m/20200313115033.GA183471@...
--
Michael

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

Re: Refactor compile-time assertion checks for C/C++

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> The fun does not stop here.  gcc is fine when using that for C and
> C++:
> #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); }))

Hm, I'm not so sure.  I just noticed that cpluspluscheck is failing
for me now:

$ src/tools/pginclude/cpluspluscheck
In file included from /tmp/cpluspluscheck.HRgpVA/test.cpp:4:
./src/include/common/int128.h: In function 'void int128_add_int64_mul_int64(INT128*, int64, int64)':
./src/include/common/int128.h:180: error: types may not be defined in 'sizeof' expressions

which of course is pointing at

    StaticAssertStmt(((int64) -1 >> 1) == (int64) -1,
                     "arithmetic right shift is needed");

so the existing "C and C++" fallback StaticAssertStmt doesn't work for
older g++.  (This is g++ 4.4.7 from RHEL6.)

> But then problems come from MSVC which does not like the do{} part for
> statements, and this works:

Huh?  Surely do{} is a legal statement.

Maybe we should just revert b7f64c64d instead of putting more time
into this.  It's looking like we're going to end up with four or so
implementations no matter what, so it's getting hard to see any
real benefit.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Refactor compile-time assertion checks for C/C++

Michael Paquier-2
On Sat, Mar 21, 2020 at 07:22:41PM -0400, Tom Lane wrote:
> which of course is pointing at
>
>     StaticAssertStmt(((int64) -1 >> 1) == (int64) -1,
>                      "arithmetic right shift is needed");
>
> so the existing "C and C++" fallback StaticAssertStmt doesn't work for
> older g++.  (This is g++ 4.4.7 from RHEL6.)

Hmm.  Thanks.  I just have an access down to 7.5 on my machine.

> Huh?  Surely do{} is a legal statement.

Yep, still my VS-2015 compiler complains when using the fallback with
do{} for statements, and I am not sure why.  An extra choice coming to
my mind would be to use a more native MS implementation, as documented
here:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/assert-asserte-assert-expr-macros?view=vs-2019
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/static-assert-macro?view=vs-2019

This requires an extra branch in our implementation set which is not
really appealing, with I guess the following mapping (not tested):
- _STATIC_ASSERT for StaticAssertDecl and StaticAssertExpr.
- _ASSERT_EXPR for and StaticAssertStmt.
I think that one advantage is that this would allow to simplify the
fallback implementations for C/C++ to use do{}s.

> Maybe we should just revert b7f64c64d instead of putting more time
> into this.  It's looking like we're going to end up with four or so
> implementations no matter what, so it's getting hard to see any
> real benefit.

Indeed.  I have tried a couple of other things I could think of, but I
cannot really get down to 3 implementations, so there is no actual
benefit.

I have done a complete revert to keep the history cleaner for release
notes and such, including this part:
- * On recent C++ compilers, we can use standard static_assert().
Don't you think that we should keep this comment at the end?  It is
still true.

Thanks for the discussion!
--
Michael

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

Re: Refactor compile-time assertion checks for C/C++

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> On Sat, Mar 21, 2020 at 07:22:41PM -0400, Tom Lane wrote:
>> Maybe we should just revert b7f64c64d instead of putting more time
>> into this.  It's looking like we're going to end up with four or so
>> implementations no matter what, so it's getting hard to see any
>> real benefit.

> Indeed.  I have tried a couple of other things I could think of, but I
> cannot really get down to 3 implementations, so there is no actual
> benefit.
> I have done a complete revert to keep the history cleaner for release
> notes and such, including this part:
> - * On recent C++ compilers, we can use standard static_assert().
> Don't you think that we should keep this comment at the end?  It is
> still true.

Yeah, the comment needs an update; but if we have four implementations
then it ought to describe each of them, IMO.

                        regards, tom lane


12