implicit casts from void*
I received on off-list report that commit e2809e3a101 causes an error
when building an extension written in C++, since $subject is in a
header file. The fix is simply to add an explicit cast, so I plan to
push the attached soon.
Bikeshedding: We could additionally change the pg_crc*.c files to make
them consistent, but I have not done that yet. It seems we prefer
explicit casts anyway but don't enforce that.
--
John Naylor
Amazon Web Services
Attachments:
v1-0001-Add-cast-from-pointer-to-void.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Add-cast-from-pointer-to-void.patchDownload
From fa45ec41de5201906ca1d6be663eb06f9ded5de4 Mon Sep 17 00:00:00 2001
From: John Naylor <john.naylor@postgresql.org>
Date: Tue, 1 Jul 2025 09:54:05 +0700
Subject: [PATCH v1] Add cast from pointer to void
Commit e2809e3a101 added code to a header which assigns a pointer
to void to a pointer to unsigned char. This causes build errors for
extensions written in C++. Fix by adding an explicit cast.
Discussion: https://postgr.es/m/
Backpatch-through: 18
---
src/include/port/pg_crc32c.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/include/port/pg_crc32c.h b/src/include/port/pg_crc32c.h
index 82313bb7fcf..ae008118ea8 100644
--- a/src/include/port/pg_crc32c.h
+++ b/src/include/port/pg_crc32c.h
@@ -72,7 +72,7 @@ pg_comp_crc32c_dispatch(pg_crc32c crc, const void *data, size_t len)
{
if (__builtin_constant_p(len) && len < 32)
{
- const unsigned char *p = data;
+ const unsigned char *p = (const unsigned char *) data;
/*
* For small constant inputs, inline the computation to avoid a
--
2.50.0
John Naylor <johncnaylorls@gmail.com> writes:
I received on off-list report that commit e2809e3a101 causes an error
when building an extension written in C++, since $subject is in a
header file. The fix is simply to add an explicit cast, so I plan to
push the attached soon.
Hmpfh. No objection to your patch, but I wonder why
"headerscheck --cplusplus" didn't find this? Can we get it
to do so?
Bikeshedding: We could additionally change the pg_crc*.c files to make
them consistent, but I have not done that yet. It seems we prefer
explicit casts anyway but don't enforce that.
Meh. There are an awful lot of places where we assume such casts
are okay. I'm willing to adopt a stricter definition in header
files, but it feels like requiring it in .c files is useless
make-work. As a perhaps not quite exact parallel, we mostly
don't object to writing
if (ptr)
as a shortcut for
if (ptr != NULL)
though it's hard to see the former as anything but an implicit
cast to boolean.
regards, tom lane
On Tue, Jul 1, 2025 at 10:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
John Naylor <johncnaylorls@gmail.com> writes:
I received on off-list report that commit e2809e3a101 causes an error
when building an extension written in C++, since $subject is in a
header file. The fix is simply to add an explicit cast, so I plan to
push the attached soon.Hmpfh. No objection to your patch, but I wonder why
"headerscheck --cplusplus" didn't find this? Can we get it
to do so?
Good question, and it turns out it catches it just fine, but you have
to configure with CPPFLAGS="-msse4.2" (or run the script on a Red Hat
9-ish system).
--
John Naylor
Amazon Web Services
John Naylor <johncnaylorls@gmail.com> writes:
On Tue, Jul 1, 2025 at 10:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hmpfh. No objection to your patch, but I wonder why
"headerscheck --cplusplus" didn't find this? Can we get it
to do so?
Good question, and it turns out it catches it just fine, but you have
to configure with CPPFLAGS="-msse4.2" (or run the script on a Red Hat
9-ish system).
Ha, indeed you are right. On my RHEL9 box, it's kinda drowned out
by complaints about
/usr/include/c++/11/bits/range_access.h:109:3: error: template with C linkage
109 | template<typename _Tp> _Tp* end(valarray<_Tp>&) noexcept;
| ^~~~~~~~
/tmp/headerscheck.u5CrRM/test.cpp:1:1: note: ‘extern "C"’ linkage started here
1 | extern "C" {
| ^~~~~~~~~~
but looking closer, I do see some
./src/include/port/pg_crc32c.h: In function ‘pg_crc32c pg_comp_crc32c_dispatch(pg_crc32c, const void*, size_t)’:
./src/include/port/pg_crc32c.h:75:42: error: invalid conversion from ‘const void*’ to ‘const unsigned char*’ [-fpermissive]
75 | const unsigned char *p = data;
| ^~~~
| |
| const void*
regards, tom lane
On Tue, Jul 1, 2025 at 9:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ha, indeed you are right. On my RHEL9 box, it's kinda drowned out
by complaints about/usr/include/c++/11/bits/range_access.h:109:3: error: template with C linkage
109 | template<typename _Tp> _Tp* end(valarray<_Tp>&) noexcept;
| ^~~~~~~~
/tmp/headerscheck.u5CrRM/test.cpp:1:1: note: ‘extern "C"’ linkage started here
1 | extern "C" {
| ^~~~~~~~~~
After pushing my fix, I looked into this, and CI works around this by
disabling ICU. A proper fix was discussed here, but it trailed off:
/messages/by-id/20230311033727.koa4saxy5wyquu6s@awork3.anarazel.de
I came up with the attached -- Andres, Peter, does this match your recollection?
--
John Naylor
Amazon Web Services
Attachments:
v1-0001-Properly-fix-cpluspluscheck-with-ICU.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Properly-fix-cpluspluscheck-with-ICU.patchDownload
From 6a2a016799a1ba8473cc66aad8509890f00b4ec4 Mon Sep 17 00:00:00 2001
From: John Naylor <john.naylor@postgresql.org>
Date: Wed, 2 Jul 2025 13:56:32 +0700
Subject: [PATCH v1] Properly fix cpluspluscheck with ICU
---
.cirrus.tasks.yml | 3 ---
src/include/utils/pg_locale.h | 5 +++++
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 92057006c93..1a366975d82 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -938,14 +938,11 @@ task:
# - Don't use ccache, the files are uncacheable, polluting ccache's
# cache
# - Use -fmax-errors, as particularly cpluspluscheck can be very verbose
- # - XXX have to disable ICU to avoid errors:
- # https://postgr.es/m/20220323002024.f2g6tivduzrktgfa%40alap3.anarazel.de
###
always:
headers_headerscheck_script: |
time ./configure \
${LINUX_CONFIGURE_FEATURES} \
- --without-icu \
--quiet \
CC="gcc" CXX"=g++" CLANG="clang-16"
make -s -j${BUILD_JOBS} clean
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 44ff60a25b4..300c78ba93c 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -15,7 +15,12 @@
#include "mb/pg_wchar.h"
#ifdef USE_ICU
+/* only include the C APIs, to avoid errors in cpluspluscheck */
+#undef U_SHOW_CPLUSPLUS_API
+#define U_SHOW_CPLUSPLUS_API 0
#include <unicode/ucol.h>
+/* restore so that extensions can include the C++ APIs */
+#undef U_SHOW_CPLUSPLUS_API
#endif
/* use for libc locale names */
--
2.50.0
On 02.07.25 09:01, John Naylor wrote:
On Tue, Jul 1, 2025 at 9:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ha, indeed you are right. On my RHEL9 box, it's kinda drowned out
by complaints about/usr/include/c++/11/bits/range_access.h:109:3: error: template with C linkage
109 | template<typename _Tp> _Tp* end(valarray<_Tp>&) noexcept;
| ^~~~~~~~
/tmp/headerscheck.u5CrRM/test.cpp:1:1: note: ‘extern "C"’ linkage started here
1 | extern "C" {
| ^~~~~~~~~~After pushing my fix, I looked into this, and CI works around this by
disabling ICU. A proper fix was discussed here, but it trailed off:/messages/by-id/20230311033727.koa4saxy5wyquu6s@awork3.anarazel.de
I came up with the attached -- Andres, Peter, does this match your recollection?
This looks sensible to me. Assuming that it works for this purpose, it
seems otherwise harmless.
John Naylor <johncnaylorls@gmail.com> writes:
After pushing my fix, I looked into this, and CI works around this by
disabling ICU. A proper fix was discussed here, but it trailed off:
/messages/by-id/20230311033727.koa4saxy5wyquu6s@awork3.anarazel.de
I came up with the attached -- Andres, Peter, does this match your recollection?
I tested this on my RHEL9 box, and confirm that I get a clean build
and headerscheck is silent.
regards, tom lane
Hi,
On 2025-07-02 14:01:13 +0700, John Naylor wrote:
On Tue, Jul 1, 2025 at 9:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ha, indeed you are right. On my RHEL9 box, it's kinda drowned out
by complaints about/usr/include/c++/11/bits/range_access.h:109:3: error: template with C linkage
109 | template<typename _Tp> _Tp* end(valarray<_Tp>&) noexcept;
| ^~~~~~~~
/tmp/headerscheck.u5CrRM/test.cpp:1:1: note: ‘extern "C"’ linkage started here
1 | extern "C" {
| ^~~~~~~~~~After pushing my fix, I looked into this, and CI works around this by
disabling ICU. A proper fix was discussed here, but it trailed off:/messages/by-id/20230311033727.koa4saxy5wyquu6s@awork3.anarazel.de
I came up with the attached -- Andres, Peter, does this match your recollection?
I think the proper fix here would be to not expose ucol.h to the world,
i.e. not include it from something like pg_locale.h.
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h index 44ff60a25b4..300c78ba93c 100644 --- a/src/include/utils/pg_locale.h +++ b/src/include/utils/pg_locale.h @@ -15,7 +15,12 @@ #include "mb/pg_wchar.h"#ifdef USE_ICU +/* only include the C APIs, to avoid errors in cpluspluscheck */ +#undef U_SHOW_CPLUSPLUS_API +#define U_SHOW_CPLUSPLUS_API 0 #include <unicode/ucol.h> +/* restore so that extensions can include the C++ APIs */ +#undef U_SHOW_CPLUSPLUS_API #endif
Does the #undef U_SHOW_CPLUSPLUS_API thing actually work, given that ucol.h
presumably won't be included again due to #ifdef protection?
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2025-07-02 14:01:13 +0700, John Naylor wrote:
I came up with the attached -- Andres, Peter, does this match your recollection?
I think the proper fix here would be to not expose ucol.h to the world,
i.e. not include it from something like pg_locale.h.
The stumbling block to that is that pg_locale_struct has a field of
type UCollator. Of course there are workarounds, but I think all of
them are strictly worse than including <ucol.h> here.
+/* restore so that extensions can include the C++ APIs */ +#undef U_SHOW_CPLUSPLUS_API
Does the #undef U_SHOW_CPLUSPLUS_API thing actually work, given that ucol.h
presumably won't be included again due to #ifdef protection?
Good point. If a .cpp file wants access to the C++ APIs, it'd have
to include <unicode/ucol.h> before not after including pg_locale.h.
That should work AFAICS, but this #undef doesn't help.
regards, tom lane
On Fri, Jul 4, 2025 at 10:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
+/* restore so that extensions can include the C++ APIs */ +#undef U_SHOW_CPLUSPLUS_APIDoes the #undef U_SHOW_CPLUSPLUS_API thing actually work, given that ucol.h
presumably won't be included again due to #ifdef protection?Good point. If a .cpp file wants access to the C++ APIs, it'd have
to include <unicode/ucol.h> before not after including pg_locale.h.
That should work AFAICS, but this #undef doesn't help.
I see that now. If extensions follow the practice of including system
headers before Postgres headers, it should be fine. I've attached v2
which removes the useless #undef and drafts an explanatory commit
message.
--
John Naylor
Amazon Web Services
Attachments:
v2-0001-Hide-ICU-C-APIs-from-pg_locale.h.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Hide-ICU-C-APIs-from-pg_locale.h.patchDownload
From 23fef0965cc2c75b32e531ac670cf31c0b4bd610 Mon Sep 17 00:00:00 2001
From: John Naylor <john.naylor@postgresql.org>
Date: Mon, 7 Jul 2025 12:28:05 +0700
Subject: [PATCH v2] Hide ICU C++ APIs from pg_locale.h
The cpluspluscheck script wraps our headers in `extern "C"`. This
disables name mangling, which is necessary for the C++ templates in
system ICU headers. This breaks cpluspluscheck when configured with
ICU (the default). CI worked around this by disabling ICU, but let's
make it work so others can run the script.
We can specify we only want the C APIs by defining U_SHOW_CPLUSPLUS_API
to be 0 in pg_locale.h. Extensions that want the C++ APIs can include
ICU headers separately before including PostgreSQL headers.
ICU documentation:
https://github.com/unicode-org/icu/blob/main/docs/processes/release/tasks/healthy-code.md#test-icu4c-headers
Suggested-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/20220323002024.f2g6tivduzrktgfa%40alap3.anarazel.de
Discussion: https://postgr.es/m/CANWCAZbgiaz1_0-F4SD%2B%3D-e9onwAnQdBGJbhg94EqUu4Gb7WyA%40mail.gmail.com
---
.cirrus.tasks.yml | 3 ---
src/include/utils/pg_locale.h | 3 +++
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 92057006c93..1a366975d82 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -938,14 +938,11 @@ task:
# - Don't use ccache, the files are uncacheable, polluting ccache's
# cache
# - Use -fmax-errors, as particularly cpluspluscheck can be very verbose
- # - XXX have to disable ICU to avoid errors:
- # https://postgr.es/m/20220323002024.f2g6tivduzrktgfa%40alap3.anarazel.de
###
always:
headers_headerscheck_script: |
time ./configure \
${LINUX_CONFIGURE_FEATURES} \
- --without-icu \
--quiet \
CC="gcc" CXX"=g++" CLANG="clang-16"
make -s -j${BUILD_JOBS} clean
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 44ff60a25b4..1cd7c76a0a7 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -15,6 +15,9 @@
#include "mb/pg_wchar.h"
#ifdef USE_ICU
+/* only include the C APIs, to avoid errors in cpluspluscheck */
+#undef U_SHOW_CPLUSPLUS_API
+#define U_SHOW_CPLUSPLUS_API 0
#include <unicode/ucol.h>
#endif
--
2.50.0
John Naylor <johncnaylorls@gmail.com> writes:
I see that now. If extensions follow the practice of including system
headers before Postgres headers, it should be fine. I've attached v2
which removes the useless #undef and drafts an explanatory commit
message.
Works for me.
regards, tom lane
On Mon, Jul 7, 2025 at 11:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
John Naylor <johncnaylorls@gmail.com> writes:
I see that now. If extensions follow the practice of including system
headers before Postgres headers, it should be fine. I've attached v2
which removes the useless #undef and drafts an explanatory commit
message.Works for me.
Pushed.
--
John Naylor
Amazon Web Services
John Naylor <johncnaylorls@gmail.com> writes:
On Mon, Jul 7, 2025 at 11:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
John Naylor <johncnaylorls@gmail.com> writes:
I see that now. If extensions follow the practice of including system
headers before Postgres headers, it should be fine. I've attached v2
which removes the useless #undef and drafts an explanatory commit
message.
Works for me.
Pushed.
Just when you thought it was safe to go back in the water ...
I tried cpluspluscheck with late-model libicu (76.1 on Fedora 42)
and darned if it didn't blow up in exactly the same way.
Investigation reveals that they've split the "U_SHOW_CPLUSPLUS_API"
symbol into two, and now if you really really don't want any C++
stuff you need to also set "U_SHOW_CPLUSPLUS_HEADER_API" to zero.
I've confirmed that this re-silences the failures:
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 931f5b3b880..2b072cafb4d 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -18,6 +18,8 @@
/* only include the C APIs, to avoid errors in cpluspluscheck */
#undef U_SHOW_CPLUSPLUS_API
#define U_SHOW_CPLUSPLUS_API 0
+#undef U_SHOW_CPLUSPLUS_HEADER_API
+#define U_SHOW_CPLUSPLUS_HEADER_API 0
#include <unicode/ucol.h>
#endifThis shouldn't complicate extensions' lives any further than
before; the rule still is "include ICU headers first
if you want their C++ symbols".
BTW, I see that you applied ed26c4e25 only to master, but don't
we want to back-patch? cpluspluscheck is not just an exercise in a
vacuum, it's to ensure that C++-coded extensions don't have trouble
with our headers.
regards, tom lane
On Wed, Aug 6, 2025 at 12:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h index 931f5b3b880..2b072cafb4d 100644 --- a/src/include/utils/pg_locale.h +++ b/src/include/utils/pg_locale.h @@ -18,6 +18,8 @@ /* only include the C APIs, to avoid errors in cpluspluscheck */ #undef U_SHOW_CPLUSPLUS_API #define U_SHOW_CPLUSPLUS_API 0 +#undef U_SHOW_CPLUSPLUS_HEADER_API +#define U_SHOW_CPLUSPLUS_HEADER_API 0 #include <unicode/ucol.h> #endifThis shouldn't complicate extensions' lives any further than
before; the rule still is "include ICU headers first
if you want their C++ symbols".BTW, I see that you applied ed26c4e25 only to master, but don't
we want to back-patch? cpluspluscheck is not just an exercise in a
vacuum, it's to ensure that C++-coded extensions don't have trouble
with our headers.
I was thinking that it was run only when developing new features, not
for backpatch-able bug fixes, but that's a flawed assumption. I'll
remedy that soon along with the new symbols above, unless you beat me
to it.
--
John Naylor
Amazon Web Services
John Naylor <johncnaylorls@gmail.com> writes:
On Wed, Aug 6, 2025 at 12:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
BTW, I see that you applied ed26c4e25 only to master, but don't
we want to back-patch? cpluspluscheck is not just an exercise in a
vacuum, it's to ensure that C++-coded extensions don't have trouble
with our headers.
I was thinking that it was run only when developing new features, not
for backpatch-able bug fixes, but that's a flawed assumption. I'll
remedy that soon along with the new symbols above, unless you beat me
to it.
Sounds good, thanks for dealing with it.
regards, tom lane
On Wed, Aug 6, 2025 at 7:16 PM John Naylor <johncnaylorls@gmail.com> wrote:
BTW, I see that you applied ed26c4e25 only to master, but don't
we want to back-patch? cpluspluscheck is not just an exercise in a
vacuum, it's to ensure that C++-coded extensions don't have trouble
with our headers.I was thinking that it was run only when developing new features, not
for backpatch-able bug fixes, but that's a flawed assumption. I'll
remedy that soon along with the new symbols above, unless you beat me
to it.
This is done.
--
John Naylor
Amazon Web Services