implicit casts from void*

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

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_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.

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

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>
 #endif

This 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>
#endif

This 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