CI CompilerWarnings test fails on 15 in mingw_cross_warning
Hi,
See https://cirrus-ci.com/task/5880116075560960
[18:14:04.821] time make -s -j${BUILD_JOBS} world-bin
[18:15:49.090] pg_locale.c: In function ‘get_collation_actual_version’:
[18:15:49.090] pg_locale.c:1763:42: error: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘long unsigned int’ [-Werror=format=]
[18:15:49.090] 1763 | collversion = psprintf("%d.%d,%d.%d",
[18:15:49.090] | ~^
[18:15:49.090] | |
[18:15:49.090] | int
[18:15:49.090] | %ld
[18:15:49.090] 1764 | (version.dwNLSVersion >> 8) & 0xFFFF,
[18:15:49.090] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[18:15:49.090] | |
[18:15:49.090] | long unsigned int
I have no idea why we are seeing this error now when we didn't in the past -
there don't seem to have been any relevant changes?
It does reproduce on my debian sid machine, so it's something we ought to fix,
I think?
We did fix it in newer versions:
Author: Peter Eisentraut <peter@eisentraut.org>
Branch: master Release: REL_16_BR [a9bc04b21] 2023-03-24 07:21:40 +0100
Fix incorrect format placeholders
The fields of NLSVERSIONINFOEX are of type DWORD, which is unsigned
long, so the results of the computations being printed are also of
type unsigned long.
Peter, any reason you didn't backpatch that?
Greetings,
Andres Freund
On 17.11.24 02:59, Andres Freund wrote:
Hi,
See https://cirrus-ci.com/task/5880116075560960
[18:14:04.821] time make -s -j${BUILD_JOBS} world-bin
[18:15:49.090] pg_locale.c: In function ‘get_collation_actual_version’:
[18:15:49.090] pg_locale.c:1763:42: error: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘long unsigned int’ [-Werror=format=]
[18:15:49.090] 1763 | collversion = psprintf("%d.%d,%d.%d",
[18:15:49.090] | ~^
[18:15:49.090] | |
[18:15:49.090] | int
[18:15:49.090] | %ld
[18:15:49.090] 1764 | (version.dwNLSVersion >> 8) & 0xFFFF,
[18:15:49.090] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[18:15:49.090] | |
[18:15:49.090] | long unsigned intI have no idea why we are seeing this error now when we didn't in the past -
there don't seem to have been any relevant changes?It does reproduce on my debian sid machine, so it's something we ought to fix,
I think?We did fix it in newer versions:
Author: Peter Eisentraut <peter@eisentraut.org>
Branch: master Release: REL_16_BR [a9bc04b21] 2023-03-24 07:21:40 +0100Fix incorrect format placeholders
The fields of NLSVERSIONINFOEX are of type DWORD, which is unsigned
long, so the results of the computations being printed are also of
type unsigned long.Peter, any reason you didn't backpatch that?
The more interesting patch is 495ed0ef2d7, which did
- collversion = psprintf("%d.%d,%d.%d",
+ collversion = psprintf("%ld.%ld,%ld.%ld",whereas my patch just did
- collversion = psprintf("%ld.%ld,%ld.%ld",
+ collversion = psprintf("%lu.%lu,%lu.%lu",The former change was part of a larger patch, so it was not a candidate
for backpatching.
As to why it's happening now, the code in question is guarded by
#elif defined(WIN32) && _WIN32_WINNT >= 0x0600
so if it didn't happen before, maybe the _WIN32_WINNT value changed.
On Tue, Nov 19, 2024 at 12:09 AM Peter Eisentraut <peter@eisentraut.org> wrote:
The more interesting patch is 495ed0ef2d7, which did
- collversion = psprintf("%d.%d,%d.%d", + collversion = psprintf("%ld.%ld,%ld.%ld",
This platform has sizeof(int) == sizeof(long), so it doesn't matter.
whereas my patch just did
- collversion = psprintf("%ld.%ld,%ld.%ld", + collversion = psprintf("%lu.%lu,%lu.%lu",
I was worried the numbers might have been negative, but those
expressions don't appear to allow that. Strings observed in real life
are like "1539.5,1539.5" (which bears a passing resemblance to an ICU
version, it has a few fairly low-valued human-originated numbers like
a Unicode version swizzled around in a fairly small number of bits,
but it doesn't matter too match how it works, it's enough to see that
we shift down and mask).
So I think it's OK to back-patch that change. See attached.
As for why now, I think both fairywren and CI started warning/failing
about 2 months ago[1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2024-10-02%2003%3A57%3A44[2]https://github.com/postgres/postgres/runs/30890390821, and that's when fairywren got a uname -r
upgrade, so I guess msys was upgraded; as for CI I guess you'd have to
check for the relevant ming-cross packages moving in Debian.
Unfortunately there were a lot of random CI failures around that time
and I didn't notice this particular circus at the time.
[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2024-10-02%2003%3A57%3A44
[2]: https://github.com/postgres/postgres/runs/30890390821
Attachments:
0001-Fix-MinGW-d-vs-lu-warnings-in-back-branches.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-MinGW-d-vs-lu-warnings-in-back-branches.patchDownload
From dce109fcd0da3c1379cd64389d03fdef18b52b54 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Thu, 28 Nov 2024 16:38:03 +1300
Subject: [PATCH] Fix MinGW %d vs %lu warnings in back branches.
Commit 352f6f2d used %d to format DWORD values (unsigned long) with
psprintf(). It has format-string checking under gcc, and gcc doesn't
like it. It seems that MinGW has started compiling this code in recent
versions, perhaps due to a _WINNT value change, so now we see the
warnings.
Commits 495ed0ef and a9bc04b2 already changed it to %lu in 16+. In
13-15, build farm animal fairywren began warning a couple of months ago,
and in 15 (the first branch with CI) the CompilerWarnings
mingw_cross_warning step began failing around the same time.
This fixes the warning without any change in behavior, because
sizeof(int) == sizeof(long) on this platform and the values are computed
with DWORD expressions that cannot exceed INT_MAX or LONG_MAX.
Back-patch the formatting change from those commits into 13-15.
Reported-by: Andres Freund <andres@anarazel.de>
Reviewed-by:
Discussion: https://postgr.es/m/t2vjrcb3bloxf5qqvxjst6r7lvrefqyecxgt2koy5ho5b5glr2%40yuupmm6whgob
---
src/backend/utils/adt/pg_locale.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index 8f841c1d192..3646c979498 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1760,7 +1760,7 @@ get_collation_actual_version(char collprovider, const char *collcollate)
collcollate,
GetLastError())));
}
- collversion = psprintf("%d.%d,%d.%d",
+ collversion = psprintf("%lu.%lu,%lu.%lu",
(version.dwNLSVersion >> 8) & 0xFFFF,
version.dwNLSVersion & 0xFF,
(version.dwDefinedVersion >> 8) & 0xFFFF,
--
2.47.0
On Thu, Nov 28, 2024 at 6:09 PM Thomas Munro <thomas.munro@gmail.com> wrote:
As for why now, I think both fairywren and CI started warning/failing
about 2 months ago[1][2]
Ohhh... it's because:
commit d700e8d75bc3844d866bf15c8cadbd72d759422d
Author: Andrew Dunstan <andrew@dunslane.net>
Date: Mon Sep 30 11:32:32 2024 -0400
Bump MIN_WINNT for MINGW to clear a build error
That was 15 only. It was in that thread too, I just forgot. Sorry,
it took noticing that fairywren doesn't warn on 14 to trigger the
memory :-)