Horton

Thread

Commits

  1. Hide ICU C++ APIs from pg_locale.h

  2. ci: test headerscheck, cpluspluscheck as part of CompilerWarnings task.

  1. cpluspluscheck vs ICU

    Andres Freund <andres@anarazel.de> — 2022-03-23T00:20:24Z

    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

  2. Re: cpluspluscheck vs ICU

    Andres Freund <andres@anarazel.de> — 2022-03-23T02:23:23Z

    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

  3. Re: cpluspluscheck vs ICU

    Thomas Munro <thomas.munro@gmail.com> — 2022-03-23T02:52:34Z

    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#L0
>
> Unless somebody sees a reason not to, I'm planning to commit this soon.

LGTM.

  4. Re: cpluspluscheck vs ICU

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-23T12:19:38Z

    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#L0
>
> Unless 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

  5. Re: cpluspluscheck vs ICU

    Andres Freund <andres@anarazel.de> — 2022-03-23T15:56:17Z

    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#L0
> >
> > Unless 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

  6. Re: cpluspluscheck vs ICU

    Andres Freund <andres@anarazel.de> — 2023-03-11T03:37:27Z

    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

  7. Re: cpluspluscheck vs ICU

    Andres Freund <andres@anarazel.de> — 2023-03-11T04:10:30Z

    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

  8. Re: cpluspluscheck vs ICU

    Andres Freund <andres@anarazel.de> — 2023-08-07T23:35:57Z

    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

  9. Re: cpluspluscheck vs ICU

    Peter Eisentraut <peter@eisentraut.org> — 2023-08-08T07:20:03Z

    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-headers
> 
> So 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?