Subject: Re: Cannot find a working 64-bit integer type on Illumos

From: Thomas Munro <[email-masked]>
Date: Thu, 18 Apr 2024 12:31:05 +1200

On Sat, Mar 23, 2024 at 3:23 PM Tom Lane <[email-masked]> wrote:
> Thomas Munro <[email-masked]> 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?

---

From: Japin Li <[email-masked]>
Date: Thu, 18 Apr 2024 14:07:22 +0800

> 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 long unsigned 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

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 18 Apr 2024 10:46:57 +0200

> I played around with this a bit, but am not quite there yet.

Looks promising.

> 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"?

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

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

---

From: Thomas Munro <[email-masked]>
Date: Fri, 19 Apr 2024 08:29:27 +1200

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

---

From: Thomas Munro <[email-masked]>
Date: Fri, 19 Apr 2024 09:00:05 +1200

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

---

From: Thomas Munro <[email-masked]>
Date: Fri, 19 Apr 2024 09:22:10 +1200

>   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 'long long unsigned int', but argument 4 has type 'uint64' {aka 'long unsigned int'}…

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.

---

From: Japin Li <[email-masked]>
Date: Fri, 19 Apr 2024 13:25:05 +0800

> intercepting printf calls...).  I suspect that'll work better for you.

Yeah, the v2 patch fixed this problem.

--
Regards,
Japin Li

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 19 Apr 2024 10:34:38 +0200

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

---

From: Heikki Linnakangas <[email-masked]>
Date: Tue, 2 Jul 2024 16:34:57 +0300

> 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

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)