cpluspluscheck vs ICU
Hi,
I was about to propose adding headerscheck / cpluspluscheck to the CI file so
that cfbot can catch future issues. Unfortunately running cpluspluscheck with
ICU enabled is, um, not fun: There's 30k lines of error output.
/home/andres/src/postgresql/src/tools/pginclude/cpluspluscheck /home/andres/src/postgresql /home/andres/build/postgres/dev-assert/vpath
In file included from /usr/include/c++/12/bits/stl_algobase.h:60,
from /usr/include/c++/12/memory:63,
from /usr/include/unicode/localpointer.h:45,
from /usr/include/unicode/unorm2.h:34,
from /usr/include/unicode/unorm.h:25,
from /usr/include/unicode/ucol.h:17,
from /home/andres/src/postgresql/src/include/utils/pg_locale.h:19,
from /home/andres/src/postgresql/src/include/tsearch/ts_locale.h:20,
from /tmp/cpluspluscheck.H59Y6V/test.cpp:3:
/usr/include/c++/12/bits/functexcept.h:101:3: error: conflicting declaration of C function ‘void std::__throw_ios_failure(const char*, int)’
101 | __throw_ios_failure(const char*, int) __attribute__((__noreturn__));
| ^~~~~~~~~~~~~~~~~~~
/usr/include/c++/12/bits/functexcept.h:98:3: note: previous declaration ‘void std::__throw_ios_failure(const char*)’
98 | __throw_ios_failure(const char*) __attribute__((__noreturn__));
| ^~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/12/bits/stl_algobase.h:63:
/usr/include/c++/12/ext/numeric_traits.h:50:3: error: template with C linkage
50 | template<typename _Tp>
| ^~~~~~~~
/tmp/cpluspluscheck.H59Y6V/test.cpp:1:1: note: ‘extern "C"’ linkage started here
1 | extern "C" {
| ^~~~~~~~~~
...
with one warning for each declaration in numeric_traits.h, I think.
So, there's two questions:
1) How can we prevent this problem when ICU support is enabled?
2) Can we prevent such absurdly long error output?
For 2), perhaps we should just specify EXTRAFLAGS=-fmax-errors=10 in the
cpluspluscheck invocation, or add it in cpluspluscheck itself?
For 1), I don't immediately see a minimal solution other than ignoring it in
cpluspluscheck, similar to pg_trace.h/probes.h.
A different / complimentary approach could be to add -Wc++-compat to the
headerscheck invocation. Both gcc and clang understand that.
But neither of these really gets to the heart of the problem. There's still no
way for C++ code to include pg_locale.h correctly. And in contrast to
pg_trace.h/probes.h pg_locale.h is somewhat important.
This isn't a new problem, afaics.
Perhaps we should strive to remove the use of ICU headers from within our
headers? The members of pg_locale are just pointers and could thus be void *,
and HAVE_UCOL_STRCOLLUTF8 could be computed at configure time or such.
Greetings,
Andres Freund
Hi,
On 2022-03-22 17:20:24 -0700, Andres Freund wrote:
I was about to propose adding headerscheck / cpluspluscheck to the CI file so
that cfbot can catch future issues.
The attached patch does so, with ICU disabled to avoid the problems discussed
in the thread. Example run:
https://cirrus-ci.com/task/6326161696358400?logs=headers_headerscheck#L0
Unless somebody sees a reason not to, I'm planning to commit this soon.
Greetings,
Andres Freund
Attachments:
v1-0001-ci-test-headerscheck-cpluspluscheck-as-part-of-Co.patchtext/x-diff; charset=us-asciiDownload
From fbc2c30f2420706bc2db64fa193b809b88ddff0b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 22 Mar 2022 16:52:03 -0700
Subject: [PATCH v1] ci: test headerscheck, cpluspluscheck as part of
CompilerWarnings task.
---
.cirrus.yml | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/.cirrus.yml b/.cirrus.yml
index e5335fede76..171bd29cf03 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -576,5 +576,28 @@ task:
make -s -j${BUILD_JOBS} clean
time make -s -j${BUILD_JOBS} -C doc
+ ###
+ # Verify headerscheck / cpluspluscheck succeed
+ #
+ # - 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
+ # - XXX: the -Wno-register avoids verbose warnings:
+ # https://postgr.es/m/20220308181837.aun3tdtdvao4vb7o%40alap3.anarazel.de
+ ###
+ always:
+ headers_headerscheck_script: |
+ time ./configure \
+ ${LINUX_CONFIGURE_FEATURES} \
+ --without-icu \
+ --quiet \
+ CC="gcc" CXX"=g++" CLANG="clang"
+ make -s -j${BUILD_JOBS} clean
+ time make -s headerscheck EXTRAFLAGS='-fmax-errors=10'
+ headers_cpluspluscheck_script: |
+ time make -s cpluspluscheck EXTRAFLAGS='-Wno-register -fmax-errors=10'
+
always:
upload_caches: ccache
--
2.35.1.354.g715d08a9e5
On Wed, Mar 23, 2022 at 3:23 PM Andres Freund <andres@anarazel.de> wrote:
On 2022-03-22 17:20:24 -0700, Andres Freund wrote:
I was about to propose adding headerscheck / cpluspluscheck to the CI file so
that cfbot can catch future issues.The attached patch does so, with ICU disabled to avoid the problems discussed
in the thread. Example run:
https://cirrus-ci.com/task/6326161696358400?logs=headers_headerscheck#L0Unless somebody sees a reason not to, I'm planning to commit this soon.
LGTM.
On 3/22/22 22:23, Andres Freund wrote:
Hi,
On 2022-03-22 17:20:24 -0700, Andres Freund wrote:
I was about to propose adding headerscheck / cpluspluscheck to the CI file so
that cfbot can catch future issues.The attached patch does so, with ICU disabled to avoid the problems discussed
in the thread. Example run:
https://cirrus-ci.com/task/6326161696358400?logs=headers_headerscheck#L0Unless somebody sees a reason not to, I'm planning to commit this soon.
That only helps when running the CI/cfbot setup. Fixing it for other
(manual or buildfarm) users would be nice. Luckily crake isn't building
with ICU.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Hi,
On 2022-03-23 08:19:38 -0400, Andrew Dunstan wrote:
On 3/22/22 22:23, Andres Freund wrote:
On 2022-03-22 17:20:24 -0700, Andres Freund wrote:
I was about to propose adding headerscheck / cpluspluscheck to the CI file so
that cfbot can catch future issues.The attached patch does so, with ICU disabled to avoid the problems discussed
in the thread. Example run:
https://cirrus-ci.com/task/6326161696358400?logs=headers_headerscheck#L0Unless somebody sees a reason not to, I'm planning to commit this soon.
That only helps when running the CI/cfbot setup. Fixing it for other
(manual or buildfarm) users would be nice. Luckily crake isn't building
with ICU.
Oh, I agree we need to fix it properly. I just don't yet know how to - see the
list of alternatives upthread. Seems no reason to hold up preventing further
problems via CI / cfbot though.
Greetings,
Andres Freund
Hi,
On 2022-03-23 08:56:17 -0700, Andres Freund wrote:
On 2022-03-23 08:19:38 -0400, Andrew Dunstan wrote:
On 3/22/22 22:23, Andres Freund wrote:
That only helps when running the CI/cfbot setup. Fixing it for other
(manual or buildfarm) users would be nice. Luckily crake isn't building
with ICU.Oh, I agree we need to fix it properly. I just don't yet know how to - see the
list of alternatives upthread. Seems no reason to hold up preventing further
problems via CI / cfbot though.
I just hit this once more - and I figured out a fairly easy fix:
We just need a
#ifndef U_DEFAULT_SHOW_DRAFT
#define U_DEFAULT_SHOW_DRAFT 0
#endif
before including unicode/ucol.h.
At first I was looking at
#define U_SHOW_CPLUSPLUS_API 0
and
#define U_HIDE_INTERNAL_API 1
which both work, but they are documented to be internal.
The reason for the #ifndef is that pg_locale.h might be included by .c files
that already included ICU headers, which then otherwise would cause macro
redefinition warnings. E.g. in formatting.c.
Alternatively we could emit U_DEFAULT_SHOW_DRAFT 0 into pg_config.h to avoid
that issue.
The only other thing I see is to do something like:
#ifdef USE_ICU
#ifdef __cplusplus
/* close extern "C", otherwise we'll get errors from within ICU */
}
#endif /* __cplusplus */
#include <unicode/ucol.h>
#ifdef __cplusplus
extern "C" {
#endif /* __cplusplus */
#endif /* USE_ICU */
which seems mighty ugly.
Regards,
Andres
Hi,
On 2023-03-10 19:37:27 -0800, Andres Freund wrote:
On 2022-03-23 08:56:17 -0700, Andres Freund wrote:
On 2022-03-23 08:19:38 -0400, Andrew Dunstan wrote:
On 3/22/22 22:23, Andres Freund wrote:
That only helps when running the CI/cfbot setup. Fixing it for other
(manual or buildfarm) users would be nice. Luckily crake isn't building
with ICU.Oh, I agree we need to fix it properly. I just don't yet know how to - see the
list of alternatives upthread. Seems no reason to hold up preventing further
problems via CI / cfbot though.I just hit this once more - and I figured out a fairly easy fix:
We just need a
#ifndef U_DEFAULT_SHOW_DRAFT
#define U_DEFAULT_SHOW_DRAFT 0
#endif
before including unicode/ucol.h.At first I was looking at
#define U_SHOW_CPLUSPLUS_API 0
and
#define U_HIDE_INTERNAL_API 1
which both work, but they are documented to be internal.
Err. Unfortunately only the U_SHOW_CPLUSPLUS_API approach actually works. The
others don't, not quite sure what I was doing earlier.
So it's either relying on a define marked as internal, or the below:
Alternatively we could emit U_DEFAULT_SHOW_DRAFT 0 into pg_config.h to avoid
that issue.The only other thing I see is to do something like:
#ifdef USE_ICU
#ifdef __cplusplus
/* close extern "C", otherwise we'll get errors from within ICU */
}
#endif /* __cplusplus */#include <unicode/ucol.h>
#ifdef __cplusplus
extern "C" {
#endif /* __cplusplus */#endif /* USE_ICU */
which seems mighty ugly.
Greetings,
Andres Freund
Hi,
On 2023-03-10 20:10:30 -0800, Andres Freund wrote:
On 2023-03-10 19:37:27 -0800, Andres Freund wrote:
I just hit this once more - and I figured out a fairly easy fix:
We just need a
#ifndef U_DEFAULT_SHOW_DRAFT
#define U_DEFAULT_SHOW_DRAFT 0
#endif
before including unicode/ucol.h.At first I was looking at
#define U_SHOW_CPLUSPLUS_API 0
and
#define U_HIDE_INTERNAL_API 1
which both work, but they are documented to be internal.Err. Unfortunately only the U_SHOW_CPLUSPLUS_API approach actually works. The
others don't, not quite sure what I was doing earlier.So it's either relying on a define marked as internal, or the below:
Alternatively we could emit U_DEFAULT_SHOW_DRAFT 0 into pg_config.h to avoid
that issue.The only other thing I see is to do something like:
[ugly]
which seems mighty ugly.
The ICU docs talk about it like it's not really internal:
https://github.com/unicode-org/icu/blob/720e5741ccaa112c4faafffdedeb7459b66c5673/docs/processes/release/tasks/healthy-code.md#test-icu4c-headers
So I'm inclined to go with that solution.
Any comments? Arguments against?
Greetings,
Andres Freund
On 08.08.23 01:35, Andres Freund wrote:
Hi,
On 2023-03-10 20:10:30 -0800, Andres Freund wrote:
On 2023-03-10 19:37:27 -0800, Andres Freund wrote:
I just hit this once more - and I figured out a fairly easy fix:
We just need a
#ifndef U_DEFAULT_SHOW_DRAFT
#define U_DEFAULT_SHOW_DRAFT 0
#endif
before including unicode/ucol.h.At first I was looking at
#define U_SHOW_CPLUSPLUS_API 0
and
#define U_HIDE_INTERNAL_API 1
which both work, but they are documented to be internal.Err. Unfortunately only the U_SHOW_CPLUSPLUS_API approach actually works. The
others don't, not quite sure what I was doing earlier.So it's either relying on a define marked as internal, or the below:
Alternatively we could emit U_DEFAULT_SHOW_DRAFT 0 into pg_config.h to avoid
that issue.The only other thing I see is to do something like:
[ugly]
which seems mighty ugly.The ICU docs talk about it like it's not really internal:
https://github.com/unicode-org/icu/blob/720e5741ccaa112c4faafffdedeb7459b66c5673/docs/processes/release/tasks/healthy-code.md#test-icu4c-headersSo I'm inclined to go with that solution.
This looks sensible to me.
Perhaps undef U_SHOW_CPLUSPLUS_API after including the headers, so that
if extensions want to use the ICU C++ APIs, they are not tripped up by this?