Обсуждение: Cannot find a working 64-bit integer type on Illumos
Hi, hackers, When I try to configure PostgreSQL 16.2 on Illumos using the following command, it complains $subject. ./configure --enable-cassert --enable-debug --enable-nls --with-perl \ --with-python --without-tcl --without-gssapi --with-openssl \ --with-ldap --with-libxml --with-libxslt --without-systemd \ --with-readline --enable-thread-safety --enable-dtrace \ DTRACEFLAGS=-64 CFLAGS=-Werror However, if I remove the `CFLAGS=-Werror`, it works fine. I'm not sure what happened here. $ uname -a SunOS db_build 5.11 hunghu-20231216T132436Z i86pc i386 i86pc illumos $ gcc --version gcc (GCC) 10.4.0 Copyright (C) 2020 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Hi, On 2024-03-23 00:48:05 +0800, Japin Li wrote: > When I try to configure PostgreSQL 16.2 on Illumos using the following command, > it complains $subject. > > ./configure --enable-cassert --enable-debug --enable-nls --with-perl \ > --with-python --without-tcl --without-gssapi --with-openssl \ > --with-ldap --with-libxml --with-libxslt --without-systemd \ > --with-readline --enable-thread-safety --enable-dtrace \ > DTRACEFLAGS=-64 CFLAGS=-Werror > > However, if I remove the `CFLAGS=-Werror`, it works fine. Likely there's an unrelated warning triggering the configure test to fail. We'd need to see config.log to see what that is. Greetings, Andres Freund
Japin Li <japinli@hotmail.com> writes: > When I try to configure PostgreSQL 16.2 on Illumos using the following command, > it complains $subject. > ./configure --enable-cassert --enable-debug --enable-nls --with-perl \ > --with-python --without-tcl --without-gssapi --with-openssl \ > --with-ldap --with-libxml --with-libxslt --without-systemd \ > --with-readline --enable-thread-safety --enable-dtrace \ > DTRACEFLAGS=-64 CFLAGS=-Werror > However, if I remove the `CFLAGS=-Werror`, it works fine. > I'm not sure what happened here. CFLAGS=-Werror breaks a whole lot of configure's tests, not only that one. (We even have this documented, see [1].) So you can't inject -Werror that way. What I do on my buildfarm animals is the equivalent of export COPT='-Werror' after configure and before build. I think configure pays no attention to COPT, so it'd likely be safe to keep that set all the time, but in the buildfarm client it's just as easy to be conservative. regards, tom lane [1] https://www.postgresql.org/docs/devel/install-make.html#CONFIGURE-ENVVARS
On Sat, 23 Mar 2024 at 00:53, Andres Freund <andres@anarazel.de> wrote: > Hi, > > On 2024-03-23 00:48:05 +0800, Japin Li wrote: >> When I try to configure PostgreSQL 16.2 on Illumos using the following command, >> it complains $subject. >> >> ./configure --enable-cassert --enable-debug --enable-nls --with-perl \ >> --with-python --without-tcl --without-gssapi --with-openssl \ >> --with-ldap --with-libxml --with-libxslt --without-systemd \ >> --with-readline --enable-thread-safety --enable-dtrace \ >> DTRACEFLAGS=-64 CFLAGS=-Werror >> >> However, if I remove the `CFLAGS=-Werror`, it works fine. > > Likely there's an unrelated warning triggering the configure test to > fail. We'd need to see config.log to see what that is. > Thanks for your quick reply. Attach the config.log.
Вложения
On Sat, 23 Mar 2024 at 01:04, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Japin Li <japinli@hotmail.com> writes: >> When I try to configure PostgreSQL 16.2 on Illumos using the following command, >> it complains $subject. > >> ./configure --enable-cassert --enable-debug --enable-nls --with-perl \ >> --with-python --without-tcl --without-gssapi --with-openssl \ >> --with-ldap --with-libxml --with-libxslt --without-systemd \ >> --with-readline --enable-thread-safety --enable-dtrace \ >> DTRACEFLAGS=-64 CFLAGS=-Werror > >> However, if I remove the `CFLAGS=-Werror`, it works fine. >> I'm not sure what happened here. > > CFLAGS=-Werror breaks a whole lot of configure's tests, not only that > one. (We even have this documented, see [1].) So you can't inject > -Werror that way. What I do on my buildfarm animals is the equivalent > of > > export COPT='-Werror' > > after configure and before build. I think configure pays no attention > to COPT, so it'd likely be safe to keep that set all the time, but in > the buildfarm client it's just as easy to be conservative. > > regards, tom lane > > [1] https://www.postgresql.org/docs/devel/install-make.html#CONFIGURE-ENVVARS Thank you very much! I didn't notice this part before.
Japin Li <japinli@hotmail.com> writes: > On Sat, 23 Mar 2024 at 00:53, Andres Freund <andres@anarazel.de> wrote: >> Likely there's an unrelated warning triggering the configure test to >> fail. We'd need to see config.log to see what that is. > Thanks for your quick reply. Attach the config.log. Yup: conftest.c:139:5: error: no previous prototype for 'does_int64_work' [-Werror=missing-prototypes] 139 | int does_int64_work() | ^~~~~~~~~~~~~~~ cc1: all warnings being treated as errors configure:17003: $? = 1 configure: program exited with status 1 This warning is harmless normally, but breaks the configure probe if you enable -Werror. No doubt we could improve that test snippet so that it does not trigger that warning. But trying to make configure safe for -Werror seems like a fool's errand, for these reasons: * Do you really want to try to make all of configure's probes proof against every compiler warning everywhere? * Many of the test snippets aren't readily under our control, as they are supplied by Autoconf. * In the majority of cases, any such failures would be silent, as configure would just conclude that the feature it is probing for isn't there. So even finding there's a problem would be difficult. The short answer is that Autoconf is not designed to support -Werror and it's not worth it to try to make it do so. regards, tom lane
Hi, On 2024-03-22 13:25:56 -0400, Tom Lane wrote: > The short answer is that Autoconf is not designed to support -Werror > and it's not worth it to try to make it do so. I wonder if we ought to make configure warn if it sees -Werror in CFLAGS - this is far from the first time somebody stumbling with -Werror. Including a few quite senior hackers, if I recall correctly. We could also just filter it temporarily and put it back at the end of configure. I don't think there's great way of making the autoconf buildsystem use -Werror continually, today. IIRC the best way is to use Makefile.custom. Greetings, Andres Freund
On Fri, Mar 22, 2024 at 2:38 PM Andres Freund <andres@anarazel.de> wrote: > I wonder if we ought to make configure warn if it sees -Werror in CFLAGS - > this is far from the first time somebody stumbling with -Werror. Including a > few quite senior hackers, if I recall correctly. We could also just filter it > temporarily and put it back at the end of configure. I think I made this mistake at some point, but I just looked at config.log and corrected my mistake. I'm not strongly against having an explicit check for -Werror, but I think the main problem here is that the original poster didn't have a look at config.log to see what the actual problem was, and at least IME that's necessary in pretty much 100% of cases where configure fails for whatever reason. Perhaps autotools could be better-designed in that regard, but we don't necessarily want to work around every problem that can stem from that design choice in our code, either. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On 2024-03-22 15:02:45 -0400, Robert Haas wrote: > On Fri, Mar 22, 2024 at 2:38 PM Andres Freund <andres@anarazel.de> wrote: > > I wonder if we ought to make configure warn if it sees -Werror in CFLAGS - > > this is far from the first time somebody stumbling with -Werror. Including a > > few quite senior hackers, if I recall correctly. We could also just filter it > > temporarily and put it back at the end of configure. > > I think I made this mistake at some point, but I just looked at > config.log and corrected my mistake. IME the bigger issue is that sometimes it doesn't lead to outright failures, just to lots of stuff not being detected as supported / present. Greetings, Andres Freund
On Fri, Mar 22, 2024 at 3:31 PM Andres Freund <andres@anarazel.de> wrote: > IME the bigger issue is that sometimes it doesn't lead to outright failures, > just to lots of stuff not being detected as supported / present. Ugh. That does, indeed, sound not very nice. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > On Fri, Mar 22, 2024 at 3:31 PM Andres Freund <andres@anarazel.de> wrote: >> IME the bigger issue is that sometimes it doesn't lead to outright failures, >> just to lots of stuff not being detected as supported / present. > Ugh. That does, indeed, sound not very nice. I could get behind throwing an error if -Werror is spotted. I think trying to pull it out and put it back is too much work and a bit too much magic. regards, tom lane
On Sat, Mar 23, 2024 at 6:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > conftest.c:139:5: error: no previous prototype for 'does_int64_work' [-Werror=missing-prototypes] > 139 | int does_int64_work() > | ^~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > configure:17003: $? = 1 > configure: program exited with status 1 . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago )
Thomas Munro <thomas.munro@gmail.com> writes: > . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago ) Yeah. Now that we require C99 it's probably reasonable to assume that those things exist. I wouldn't be in favor of ripping out our existing notations like UINT64CONST, because the code churn would be substantial and the gain minimal. But we could imagine reimplementing that stuff atop <stdint.h> and then getting rid of the configure-time probes. regards, tom lane
On Sat, 23 Mar 2024 at 01:22, Japin Li <japinli@hotmail.com> wrote: > On Sat, 23 Mar 2024 at 01:04, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Japin Li <japinli@hotmail.com> writes: >>> When I try to configure PostgreSQL 16.2 on Illumos using the following command, >>> it complains $subject. >> >>> ./configure --enable-cassert --enable-debug --enable-nls --with-perl \ >>> --with-python --without-tcl --without-gssapi --with-openssl \ >>> --with-ldap --with-libxml --with-libxslt --without-systemd \ >>> --with-readline --enable-thread-safety --enable-dtrace \ >>> DTRACEFLAGS=-64 CFLAGS=-Werror >> >>> However, if I remove the `CFLAGS=-Werror`, it works fine. >>> I'm not sure what happened here. >> >> CFLAGS=-Werror breaks a whole lot of configure's tests, not only that >> one. (We even have this documented, see [1].) So you can't inject >> -Werror that way. What I do on my buildfarm animals is the equivalent >> of >> >> export COPT='-Werror' >> >> after configure and before build. I think configure pays no attention >> to COPT, so it'd likely be safe to keep that set all the time, but in >> the buildfarm client it's just as easy to be conservative. >> >> regards, tom lane >> >> [1] https://www.postgresql.org/docs/devel/install-make.html#CONFIGURE-ENVVARS > > Thank you very much! I didn't notice this part before. I try to use the following to compile it, however, it cannot compile it. $ ../configure --enable-cassert --enable-debug --enable-nls --with-perl --with-python --without-tcl --without-gssapi --with-openssl--with-ldap --with-libxml --with-libxslt --without-systemd --with-readline --enable-thread-safety --enable-dtraceDTRACEFLAGS=-64 $ make COPT='-Werror' -s /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c: In function 'repairDependencyLoop': /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c:1276:3: error: format not a string literal and no format arguments[-Werror=format-security] 1276 | pg_log_warning(ngettext("there are circular foreign-key constraints on this table:", | ^~~~~~~~~~~~~~ cc1: all warnings being treated as errors make[3]: *** [<builtin>: pg_dump_sort.o] Error 1 make[2]: *** [Makefile:43: all-pg_dump-recurse] Error 2 make[1]: *** [Makefile:42: all-bin-recurse] Error 2 make: *** [GNUmakefile:11: all-src-recurse] Error 2
Japin Li <japinli@hotmail.com> writes: > /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c: In function 'repairDependencyLoop': > /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c:1276:3: error: format not a string literal and no format arguments[-Werror=format-security] > 1276 | pg_log_warning(ngettext("there are circular foreign-key constraints on this table:", > | ^~~~~~~~~~~~~~ Yeah, some of the older buildfarm animals issue that warning too. AFAICS it's a bogus compiler heuristic: there is not anything wrong with the code as given. regards, tom lane
On Mon, 25 Mar 2024 at 09:32, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Japin Li <japinli@hotmail.com> writes: >> /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c: In function 'repairDependencyLoop': >> /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c:1276:3: error: format not a string literal and no formatarguments [-Werror=format-security] >> 1276 | pg_log_warning(ngettext("there are circular foreign-key constraints on this table:", >> | ^~~~~~~~~~~~~~ > > Yeah, some of the older buildfarm animals issue that warning too. > AFAICS it's a bogus compiler heuristic: there is not anything > wrong with the code as given. > Thanks! It seems I should remove -Werror option on Illumos.
On Sat, Mar 23, 2024 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@gmail.com> writes: > > . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago ) > > Yeah. Now that we require C99 it's probably reasonable to assume > that those things exist. I wouldn't be in favor of ripping out our > existing notations like UINT64CONST, because the code churn would be > substantial and the gain minimal. But we could imagine reimplementing > that stuff atop <stdint.h> and then getting rid of the configure-time > probes. I played around with this a bit, but am not quite there yet. printf() is a little tricky. The standard wants us to use <inttypes.h>'s PRId64 etc, but that might confuse our snprintf.c (in theory, probably not in practice). "ll" should have the right size on all systems, but gets warnings from the printf format string checker on systems where "l" is the right type. So I think we still need to probe for INT64_MODIFIER at configure-time. Here's one way, but I can see it's not working on Clang/Linux... perhaps instead of that dubious incantation I should try compiling some actual printfs and check for warnings/errors. I think INT64CONST should just point to standard INT64_C(). For limits, why do we have this: - * stdint.h limits aren't guaranteed to have compatible types with our fixed - * width types. So just define our own. ? I mean, how could they not have compatible types? I noticed that configure.ac checks if int64 (no "_t") might be defined already by system header pollution, but meson.build doesn't. That's an inconsistency that should be fixed, but which way? Hmm, commit 15abc7788e6 said that was done for BeOS, which we de-supported. So maybe we should get rid of that?
Вложения
On Thu, 18 Apr 2024 at 08:31, Thomas Munro <thomas.munro@gmail.com> wrote: > On Sat, Mar 23, 2024 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Thomas Munro <thomas.munro@gmail.com> writes: >> > . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago ) >> >> Yeah. Now that we require C99 it's probably reasonable to assume >> that those things exist. I wouldn't be in favor of ripping out our >> existing notations like UINT64CONST, because the code churn would be >> substantial and the gain minimal. But we could imagine reimplementing >> that stuff atop <stdint.h> and then getting rid of the configure-time >> probes. > > I played around with this a bit, but am not quite there yet. > > printf() is a little tricky. The standard wants us to use > <inttypes.h>'s PRId64 etc, but that might confuse our snprintf.c (in > theory, probably not in practice). "ll" should have the right size on > all systems, but gets warnings from the printf format string checker > on systems where "l" is the right type. So I think we still need to > probe for INT64_MODIFIER at configure-time. Here's one way, but I can > see it's not working on Clang/Linux... perhaps instead of that dubious > incantation I should try compiling some actual printfs and check for > warnings/errors. > > I think INT64CONST should just point to standard INT64_C(). > > For limits, why do we have this: > > - * stdint.h limits aren't guaranteed to have compatible types with our fixed > - * width types. So just define our own. > > ? I mean, how could they not have compatible types? > > I noticed that configure.ac checks if int64 (no "_t") might be defined > already by system header pollution, but meson.build doesn't. That's > an inconsistency that should be fixed, but which way? Hmm, commit > 15abc7788e6 said that was done for BeOS, which we de-supported. So > maybe we should get rid of that? Thanks for working on this! I test the patch and it now works on Illumos when configure with -Werror option. However, there are some errors when compiling. In file included from /home/japin/postgres/build/../src/include/c.h:834, from /home/japin/postgres/build/../src/include/postgres_fe.h:25, from /home/japin/postgres/build/../src/common/config_info.c:20: /home/japin/postgres/build/../src/common/config_info.c: In function 'get_configdata': /home/japin/postgres/build/../src/common/config_info.c:198:11: error: comparison of integer expressions of different signedness:'int' and 'size_t' {aka 'long unsigned int'} [-Werror=sign-compare] 198 | Assert(i == *configdata_len); | ^~ /home/japin/postgres/build/../src/common/config_info.c:198:2: note: in expansion of macro 'Assert' 198 | Assert(i == *configdata_len); | ^~~~~~ In file included from /home/japin/postgres/build/../src/common/blkreftable.c:36: /home/japin/postgres/build/../src/include/lib/simplehash.h: In function 'blockreftable_stat': /home/japin/postgres/build/../src/include/lib/simplehash.h:1138:9: error: format '%llu' expects argument of type 'long longunsigned int', but argument 4 has type 'uint64' {aka 'long unsigned int'} [-Werror=format=] 1138 | sh_log("size: " UINT64_FORMAT ", members: %u, filled: %f, total chain: %u, max chain: %u, avg chain: %f, total_collisions:%u, max_collisions: %u, avg_collisions: %f", | ^~~~~~~~ 1139 | tb->size, tb->members, fillfactor, total_chain_length, max_chain_length, avg_chain_length, | ~~~~~~~~ | | | uint64 {aka long unsigned int} /home/japin/postgres/build/../src/include/common/logging.h:125:46: note: in definition of macro 'pg_log_info' 125 | pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__) | ^~~~~~~~~~~ /home/japin/postgres/build/../src/include/lib/simplehash.h:1138:2: note: in expansion of macro 'sh_log' 1138 | sh_log("size: " UINT64_FORMAT ", members: %u, filled: %f, total chain: %u, max chain: %u, avg chain: %f, total_collisions:%u, max_collisions: %u, avg_collisions: %f", | ^~~~~~ In file included from /home/japin/postgres/build/../src/include/access/xlogrecord.h:14, from /home/japin/postgres/build/../src/include/access/xlogreader.h:41, from /home/japin/postgres/build/../src/include/access/xlog_internal.h:23, from /home/japin/postgres/build/../src/common/controldata_utils.c:28: /home/japin/postgres/build/../src/include/access/rmgr.h: In function 'RmgrIdIsCustom': /home/japin/postgres/build/../src/include/access/rmgr.h:50:42: error: comparison of integer expressions of different signedness:'int' and 'unsigned int' [-Werror=sign-compare] 50 | return rmid >= RM_MIN_CUSTOM_ID && rmid <= RM_MAX_CUSTOM_ID; | ^~ /home/japin/postgres/build/../src/common/blkreftable.c: In function 'BlockRefTableReaderGetBlocks': /home/japin/postgres/build/../src/common/blkreftable.c:716:22: error: comparison of integer expressions of different signedness:'unsigned int' and 'int' [-Werror=sign-compare] 716 | blocks_found < nblocks) | ^ /home/japin/postgres/build/../src/common/blkreftable.c:732:22: error: comparison of integer expressions of different signedness:'unsigned int' and 'int' [-Werror=sign-compare] 732 | blocks_found < nblocks) | ^ /home/japin/postgres/build/../src/common/blkreftable.c:742:20: error: comparison of integer expressions of different signedness:'unsigned int' and 'int' [-Werror=sign-compare] 742 | if (blocks_found >= nblocks) | ^~ cc1: all warnings being treated as errors cc1: all warnings being treated as errors make[2]: *** [../../src/Makefile.global:947: config_info.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[2]: *** [../../src/Makefile.global:947: controldata_utils.o] Error 1 In file included from /home/japin/postgres/build/../src/include/postgres_fe.h:25, from /home/japin/postgres/build/../src/common/logging.c:15: /home/japin/postgres/build/../src/common/logging.c: In function 'pg_log_generic_v': /home/japin/postgres/build/../src/include/c.h:523:23: error: format '%llu' expects argument of type 'long long unsigned int',but argument 3 has type 'uint64' {aka 'long unsigned int'} [-Werror=format=] 523 | #define UINT64_FORMAT "%" INT64_MODIFIER "u" | ^~~ /home/japin/postgres/build/../src/common/logging.c:259:21: note: in expansion of macro 'UINT64_FORMAT' 259 | fprintf(stderr, UINT64_FORMAT ":", lineno); | ^~~~~~~~~~~~~ /home/japin/postgres/build/../src/include/c.h:523:43: note: format string is defined here 523 | #define UINT64_FORMAT "%" INT64_MODIFIER "u" | ~~~~~~~~~~~~~~~~~~~^ | | | long long unsigned int /home/japin/postgres/build/../src/common/unicode_norm.c: In function 'recompose_code': /home/japin/postgres/build/../src/common/unicode_norm.c:290:17: error: comparison of integer expressions of different signedness:'int' and 'long unsigned int' [-Werror=sign-compare] 290 | for (i = 0; i < lengthof(UnicodeDecompMain); i++) | ^ In file included from /home/japin/postgres/build/../src/include/c.h:834, from /home/japin/postgres/build/../src/common/encnames.c:13: /home/japin/postgres/build/../src/common/encnames.c: In function 'pg_encoding_to_char_private': /home/japin/postgres/build/../src/common/encnames.c:593:19: error: comparison of integer expressions of different signedness:'int' and 'pg_enc' [-Werror=sign-compare] 593 | Assert(encoding == p->encoding); | ^~ /home/japin/postgres/build/../src/common/encnames.c:593:3: note: in expansion of macro 'Assert' 593 | Assert(encoding == p->encoding); | ^~~~~~ /home/japin/postgres/build/../src/common/jsonapi.c: In function 'pg_parse_json_incremental': /home/japin/postgres/build/../src/common/jsonapi.c:693:11: error: comparison of integer expressions of different signedness:'char' and 'JsonTokenType' [-Werror=sign-compare] 693 | if (top == tok) | ^~ cc1: all warnings being treated as errors cc1: all warnings being treated as errors make[2]: *** [../../src/Makefile.global:947: logging.o] Error 1 make[2]: *** [../../src/Makefile.global:947: blkreftable.o] Error 1 /home/japin/postgres/build/../src/common/kwlookup.c: In function 'ScanKeywordLookup': /home/japin/postgres/build/../src/common/kwlookup.c:50:10: error: comparison of integer expressions of different signedness:'size_t' {aka 'long unsigned int'} and 'int' [-Werror=sign-compare] 50 | if (len > keywords->max_kw_len) | ^ cc1: all warnings being treated as errors make[2]: *** [../../src/Makefile.global:947: encnames.o] Error 1 cc1: all warnings being treated as errors cc1: all warnings being treated as errors make[2]: *** [../../src/Makefile.global:947: kwlookup.o] Error 1 In file included from /home/japin/postgres/build/../src/include/c.h:834, from /home/japin/postgres/build/../src/include/postgres_fe.h:25, from /home/japin/postgres/build/../src/common/file_utils.c:19: /home/japin/postgres/build/../src/common/file_utils.c: In function 'pg_pwrite_zeros': /home/japin/postgres/build/../src/common/file_utils.c:725:23: error: comparison of integer expressions of different signedness:'ssize_t' {aka 'long int'} and 'size_t' {aka 'long unsigned int'} [-Werror=sign-compare] 725 | Assert(total_written == size); | ^~ /home/japin/postgres/build/../src/common/file_utils.c:725:2: note: in expansion of macro 'Assert' 725 | Assert(total_written == size); | ^~~~~~ make[2]: *** [../../src/Makefile.global:947: unicode_norm.o] Error 1 cc1: all warnings being treated as errors make[2]: *** [../../src/Makefile.global:947: file_utils.o] Error 1 /home/japin/postgres/build/../src/common/unicode_case.c: In function 'convert_case': /home/japin/postgres/build/../src/common/unicode_case.c:155:31: error: comparison of integer expressions of different signedness:'size_t' {aka 'long unsigned int'} and 'ssize_t' {aka 'long int'} [-Werror=sign-compare] 155 | while ((srclen < 0 || srcoff < srclen) && src[srcoff] != '\0') | ^ /home/japin/postgres/build/../src/common/wchar.c: In function 'pg_utf8_verifystr': /home/japin/postgres/build/../src/common/wchar.c:1868:10: error: comparison of integer expressions of different signedness:'int' and 'long unsigned int' [-Werror=sign-compare] 1868 | if (len >= STRIDE_LENGTH) | ^~ /home/japin/postgres/build/../src/common/wchar.c:1870:14: error: comparison of integer expressions of different signedness:'int' and 'long unsigned int' [-Werror=sign-compare] 1870 | while (len >= STRIDE_LENGTH) | ^~ cc1: all warnings being treated as errors make[2]: *** [../../src/Makefile.global:947: unicode_case.o] Error 1 cc1: all warnings being treated as errors make[2]: *** [../../src/Makefile.global:947: jsonapi.o] Error 1 cc1: all warnings being treated as errors make[2]: *** [../../src/Makefile.global:947: wchar.o] Error 1 make[1]: *** [Makefile:42: all-common-recurse] Error 2 make: *** [GNUmakefile:11: all-src-recurse] Error 2 For rmid >= RM_MIN_CUSTOM_ID && rmid <= RM_MAX_CUSTOM_ID comparison error, I found that UINT8_MAX is defined as '255U' on Illumos, however, Linux glibc uses '255' for UINT8_MAX, which is signed. [1] https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/sys/int_limits.h#L92 [2] https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/stdint.h;h=bb3e8b5cc61fb3df8842225d2286de67e6f2ffe2;hb=refs/heads/master#l116 -- Regards, Japin Li
On 18.04.24 02:31, Thomas Munro wrote: > On Sat, Mar 23, 2024 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Thomas Munro <thomas.munro@gmail.com> writes: >>> . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago ) >> >> Yeah. Now that we require C99 it's probably reasonable to assume >> that those things exist. I wouldn't be in favor of ripping out our >> existing notations like UINT64CONST, because the code churn would be >> substantial and the gain minimal. But we could imagine reimplementing >> that stuff atop <stdint.h> and then getting rid of the configure-time >> probes. > > I played around with this a bit, but am not quite there yet. Looks promising. > printf() is a little tricky. The standard wants us to use > <inttypes.h>'s PRId64 etc, but that might confuse our snprintf.c (in > theory, probably not in practice). "ll" should have the right size on > all systems, but gets warnings from the printf format string checker > on systems where "l" is the right type. I'm not sure I understand the problem here. Do you mean that in theory a platform's PRId64 could be something other than "l" or "ll"? > For limits, why do we have this: > > - * stdint.h limits aren't guaranteed to have compatible types with our fixed > - * width types. So just define our own. > > ? I mean, how could they not have compatible types? Maybe this means something like our int64 is long long int but the system's int64_t is long int underneath, but I don't see how that would matter for the limit macros. > I noticed that configure.ac checks if int64 (no "_t") might be defined > already by system header pollution, but meson.build doesn't. That's > an inconsistency that should be fixed, but which way? Hmm, commit > 15abc7788e6 said that was done for BeOS, which we de-supported. So > maybe we should get rid of that? I had a vague recollection that it was for AIX, but the commit indeed mentions BeOS. Could be removed in either case.
On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut <peter@eisentraut.org> wrote: > I'm not sure I understand the problem here. Do you mean that in theory > a platform's PRId64 could be something other than "l" or "ll"? Yes. I don't know why anyone would do that, and the systems I checked all have the obvious definitions, eg "ld", "lld" etc. Perhaps it's an acceptable risk? It certainly gives us a tidier result. For discussion, here is a variant that fully embraces <inttypes.h> and the PRI*64 macros.
Вложения
On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut <peter@eisentraut.org> wrote: > Maybe this means something like our int64 is long long int but the > system's int64_t is long int underneath, but I don't see how that would > matter for the limit macros. Agreed, so I don't think it's long vs long long (when they have the same width). I wonder if this comment is a clue: static char * inet_net_ntop_ipv6(const u_char *src, int bits, char *dst, size_t size) { /* * Note that int32_t and int16_t need only be "at least" large enough to * contain a value of the specified size. On some systems, like Crays, * there is no such thing as an integer variable with 16 bits. Keep this * in mind if you think this function should have been coded to use * pointer overlays. All the world's not a VAX. */ I'd seen that claim before somewhere else but I can't recall where. So there were systems using those names in an ad hoc unspecified way before C99 nailed this stuff down? In modern C, int32_t is definitely an exact width type (but there are other standardised variants like int_fast32_t to allow for Cray-like systems that would prefer to use a wider type, ie "at least", 32 bits wide, so I guess that's what happened to that idea?). Or perhaps it's referring to worries about the width of char, short, int or the assumption of two's-complement. I think if any of that stuff weren't as assumed we'd have many problems in many places, so I'm not seeing a problem. (FTR C23 finally nailed down two's-complement as a requirement, and although C might not say so, POSIX says that char is a byte, and our assumption that int = int32_t is pretty deeply baked into PostgreSQL, so it's almost impossible to imagine that short has a size other than 16 bits; but these are all assumptions made by the OLD coding, not by the patch I posted). In short, I guess that isn't what was meant.
On Thu, Apr 18, 2024 at 6:09 PM Japin Li <japinli@hotmail.com> wrote: > /home/japin/postgres/build/../src/common/config_info.c:198:11: error: comparison of integer expressions of different signedness:'int' and 'size_t' {aka 'long unsigned int'} [-Werror=sign-compare] > 198 | Assert(i == *configdata_len); Right, PostgreSQL doesn't compile cleanly with the "sign-compare" warning. There have been a few threads about that, and someone might want to think harder about it, but it's a different topic unrelated to <stdint.h>. > /home/japin/postgres/build/../src/include/lib/simplehash.h:1138:9: error: format '%llu' expects argument of type 'longlong unsigned int', but argument 4 has type 'uint64' {aka 'long unsigned int'} [-Werror=format=] It seems my v1 patch's configure probe for INT64_FORMAT was broken. In the v2 patch I tried not doing that probe at all, and instead inviting <inttypes.h> into our world (that's the standardised way to produce format strings, which has the slight complication that we are intercepting printf calls...). I suspect that'll work better for you.
On Fri, 19 Apr 2024 at 05:22, Thomas Munro <thomas.munro@gmail.com> wrote: > On Thu, Apr 18, 2024 at 6:09 PM Japin Li <japinli@hotmail.com> wrote: >> /home/japin/postgres/build/../src/include/lib/simplehash.h:1138:9: error: format '%llu' expects argument of type 'longlong unsigned int', but argument 4 has type 'uint64' {aka 'long unsigned int'} [-Werror=format=] > > It seems my v1 patch's configure probe for INT64_FORMAT was broken. > In the v2 patch I tried not doing that probe at all, and instead > inviting <inttypes.h> into our world (that's the standardised way to > produce format strings, which has the slight complication that we are > intercepting printf calls...). I suspect that'll work better for you. Yeah, the v2 patch fixed this problem. -- Regards, Japin Li
On 18.04.24 02:31, Thomas Munro wrote: > For limits, why do we have this: > > - * stdint.h limits aren't guaranteed to have compatible types with our fixed > - * width types. So just define our own. > > ? I mean, how could they not have compatible types? The commit for this was 62e2a8dc2c7 and the thread was <https://www.postgresql.org/message-id/flat/E1YatAv-0007cu-KW%40gemulon.postgresql.org>. The problem was that something like snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE); could issue a warning if, say, INT64_FORMAT, which refers to our own int64, is based on long int, but SEQ_MINVALUE, which was then INT64_MIN, which refers to int64_t, which could be long long int. So this is correct. If we introduce the use of int64_t, then you need to be consistent still: int64, PG_INT64_MIN, PG_INT64_MAX, INT64_FORMAT int64_t, INT64_MIN, INT64_MAX, PRId64
On 18/04/2024 23:29, Thomas Munro wrote: > On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut <peter@eisentraut.org> wrote: >> I'm not sure I understand the problem here. Do you mean that in theory >> a platform's PRId64 could be something other than "l" or "ll"? > > Yes. I don't know why anyone would do that, and the systems I checked > all have the obvious definitions, eg "ld", "lld" etc. Perhaps it's an > acceptable risk? It certainly gives us a tidier result. Could we have a configure check or static assertion for that? > For discussion, here is a variant that fully embraces <inttypes.h> and > the PRI*64 macros. Looks good to me. Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't nice either, though, and following standards is good, so I'm sure I'll get used to it. They're both less readable than INT64_FORMAT and "%lld", which we use in most places, though. Perhaps "%lld" and casting the arguments to "long long" would be more readable in the places where this patch replaces INT64_MODIFIER with PRI*64, too. -- Heikki Linnakangas Neon (https://neon.tech)
On Wed, Jul 3, 2024 at 1:34 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 18/04/2024 23:29, Thomas Munro wrote: > > On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut <peter@eisentraut.org> wrote: > >> I'm not sure I understand the problem here. Do you mean that in theory > >> a platform's PRId64 could be something other than "l" or "ll"? > > > > Yes. I don't know why anyone would do that, and the systems I checked > > all have the obvious definitions, eg "ld", "lld" etc. Perhaps it's an > > acceptable risk? It certainly gives us a tidier result. > > Could we have a configure check or static assertion for that? Unfortunately, that theory turned out to be wrong. The usual suspect, Windows, uses something else: "I64" or something like that. We could teach our snprintf to grok that, but I don't like the idea anymore. So let's go back to INT64_MODIFIER, with just a small amount of configure time work to pick the right value. I couldn't figure out any header-only way to do that. > Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't > nice either, though, and following standards is good, so I'm sure I'll > get used to it. Yeah, I like standards a lot but we've painted ourselves into a corner here... New version attached. This time I was brave enough to try to tackle src/timezone too, which had comments planning to drop a lot of small differences against the upstream tzcode once all supported branches required C99. I suppose that should make future maintenance easier, and C89 disappeared from our window of interest with PostgreSQL 11. It's a little hard to understand what changed, but to try to show it better I diff'd master against upstream (after filtering through sed and pgindent as recommended by README), and then diff'd patched against upstream, and then ... ehm.. diff'd the two diffs, so that you can see there are some hunks that go away. IMHO it's a rather scary choice on tzcode's part to use int_fastN_t, and hard for us to verify that it works correctly especially when combined with our changes, but on the other hand I don't really expect any system that PostgreSQL can run on to have "fast" types that really differ from the "exact width" types. My understanding is that this is something of interest to historical supercomputers and microcontrollers, but I can't find any evidence of general purpose/commodity systems that we target using different sizes (anyone know better?).
Вложения
Thomas Munro <thomas.munro@gmail.com> writes: > New version attached. This time I was brave enough to try to tackle > src/timezone too, which had comments planning to drop a lot of small > differences against the upstream tzcode once all supported branches > required C99. Unless you've specifically checked that this reduces diffs against upstream tzcode, I'd really prefer not to touch that code right now. I know I'm overdue for a round of syncing src/timezone/ with upstream, but I can't see how drive-by changes will make that easier. > IMHO it's a rather scary choice on tzcode's part to use int_fastN_t, Yeah, I was never pleased with that choice of theirs. OTOH, I've seen darn few portability complaints on their mailing list, so it seems like they've got it right in isolation. The problem from our standpoint is that I don't think we want int_fastN_t to leak into APIs visible to the rest of Postgres, because then we risk issues related to their configuration methods being totally unlike ours. regards, tom lane
On Thu, Jul 4, 2024 at 3:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Unless you've specifically checked that this reduces diffs against > upstream tzcode, I'd really prefer not to touch that code right now. > I know I'm overdue for a round of syncing src/timezone/ with upstream, > but I can't see how drive-by changes will make that easier. Sure, I'll wait until you say it's a good time. It does remove a dozen or so hunks of difference, which should hopefully make that job easier eventually but I don't want to get in your way. I can see there are a few more trivialities we could synchronise on, like const keywords, to kill useless diffs (either dropping local improvements or sending patches upstream). > > IMHO it's a rather scary choice on tzcode's part to use int_fastN_t, > > Yeah, I was never pleased with that choice of theirs. OTOH, I've > seen darn few portability complaints on their mailing list, so > it seems like they've got it right in isolation. The problem > from our standpoint is that I don't think we want int_fastN_t > to leak into APIs visible to the rest of Postgres, because then > we risk issues related to their configuration methods being > totally unlike ours. Yeah. My first swing at this touched only .c files, no .h files, with that in mind.
On 04.07.24 03:55, Thomas Munro wrote: > Unfortunately, that theory turned out to be wrong. The usual suspect, > Windows, uses something else: "I64" or something like that. We could > teach our snprintf to grok that, but I don't like the idea anymore. > So let's go back to INT64_MODIFIER, with just a small amount of > configure time work to pick the right value. I couldn't figure out > any header-only way to do that. src/port/snprintf.c used to support I64 in the past, but it was later removed. We could probably put it back. >> Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't >> nice either, though, and following standards is good, so I'm sure I'll >> get used to it. Using PRId64 would be very beneficial because gettext understands it, and so we would no longer need the various workarounds for not putting INT64_FORMAT into the middle of a translated string. But this could be a separate patch. What you have works for now. Here are some other comments on this patch set: * v3-0001-Use-int64_t-support-from-stdint.h.patch - src/include/c.h: Maybe add a comment that above all the int8_t -> int8 etc. typedefs that these are for backward compatibility or something like that. Actually, just move the comment +/* Historical names for limits in stdint.h. */ up a bit to it covers the types as well. Also, these /* == 8 bits */ comments could be removed, I think. - src/include/port/pg_bitutils.h: - src/port/pg_bitutils.c: - src/port/snprintf.c: These changes look functionally correct, but I think I like the old code layout better, like #if (using long) ... #elif (using long long) ... #else #error #endif rather than #if (using long) ... #else static assertion ... // long long #endif which seems a bit more complicated. I think you could leave the code mostly alone and just change defined(HAVE_LONG_INT_64) to SIZEOF_LONG == 8 defined(HAVE_LONG_LONG_INT_64) to SIZEOF_LONG_LONG == 8 in each case. - src/include/postgres_ext.h: -#define OID_MAX UINT_MAX -/* you will need to include <limits.h> to use the above #define */ +#define OID_MAX UINT32_MAX If the type Oid is unsigned int, then the OID_MAX should be UINT_MAX. So this should not be changed. Also, is the comment about <limits.h> no longer applicable? - src/interfaces/ecpg/ecpglib/typename.c: - src/interfaces/ecpg/include/sqltypes.h: - .../test/expected/compat_informix-sqlda.c: -#ifdef HAVE_LONG_LONG_INT_64 +#if SIZEOF_LONG < 8 These changes alter the library behavior unnecessarily. The old code would always prefer to report back long long (ECPGt_long_long etc.), but the new code will report back long (ECPGt_long etc.) if it is 64-bit. I don't know the impact of these changes, but it seems preferable to keep the existing behavior. - src/interfaces/ecpg/include/ecpg_config.h.in: - src/interfaces/ecpg/include/meson.build: In the past, we have kept obsolete symbols as always defined in ecpg_config.h. We ought to do the same here. * v3-0002-Remove-traces-of-BeOS.patch This looks ok. This could also be committed before 0001. * v3-0003-Allow-tzcode-to-use-stdint.h-and-inttypes.h.patch - src/timezone/localtime.c: Addition of #include <stdint.h> is unnecessary, since it's already included in c.h, and it's also not in the upstream code. This looks like a typo: - * UTC months are at least 28 days long (minus 1 second for a + * UTC months are at least 2 days long (minus 1 second for a -getsecs(const char *strp, int32 *const secsp) +getsecs(const char *strp, int_fast32_t * const secsp) Need to add int_fast32_t (and maybe the other types) to typedefs.list? - src/timezone/zic.c: +#include <inttypes.h> +#include <stdint.h> We don't need both of these. Also, this is not in the upstream code AFAICT.
Peter Eisentraut <peter@eisentraut.org> writes: > On 04.07.24 03:55, Thomas Munro wrote: >>> Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't >>> nice either, though, and following standards is good, so I'm sure I'll >>> get used to it. > Using PRId64 would be very beneficial because gettext understands it, > and so we would no longer need the various workarounds for not putting > INT64_FORMAT into the middle of a translated string. Uh, really? The translated strings live in /usr/share, which is expected to be architecture-independent, so how would they make that work? regards, tom lane
On 14.07.24 16:51, Tom Lane wrote: > Peter Eisentraut <peter@eisentraut.org> writes: >> On 04.07.24 03:55, Thomas Munro wrote: >>>> Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't >>>> nice either, though, and following standards is good, so I'm sure I'll >>>> get used to it. > >> Using PRId64 would be very beneficial because gettext understands it, >> and so we would no longer need the various workarounds for not putting >> INT64_FORMAT into the middle of a translated string. > > Uh, really? The translated strings live in /usr/share, which is > expected to be architecture-independent, so how would they make > that work? Gettext has some special run-time support for this. See here: <https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html#No-string-concatenation>