Thread
Commits
-
Fix "inconsistent DLL linkage" warning on Windows MSVC
- a3ea5330fcf4 19 (unreleased) landed
-
MSVC: Improve warning options set
- 2307cfe31621 19 (unreleased) landed
-
Enable MSVC conforming preprocessor
- 8fd9bb1d9654 19 (unreleased) cited
-
Fix warnings about declaration of environ on MinGW.
- 7bc9a8bdd2d6 18.0 cited
-
Require ucrt if using MinGW.
- 1758d4244616 18.0 cited
-
MSVC: Improve warning options set
Peter Eisentraut <peter@eisentraut.org> — 2025-10-29T07:51:00Z
meson.build has a list of warnings to disable on MSVC: cflags_warn += [ '/wd4018', # signed/unsigned mismatch '/wd4244', # conversion from 'type1' to 'type2', possible loss of data '/wd4273', # inconsistent DLL linkage '/wd4101', # unreferenced local variable '/wd4102', # unreferenced label '/wd4090', # different 'modifier' qualifiers '/wd4267', # conversion from 'size_t' to 'type', possible loss of data ] First, these appear to be in some random order, so I wanted to sort them. But then it also appeared that for some of these, if you remove the disablement, nothing changes, so it seemed some of these entries are not needed. Some of these warnings are assigned to higher warning levels, so if someone wanted to compile PostgreSQL with a higher warning level on MSVC (similar to -Wextra), then disabling some from the higher levels is useful. But I figured this could be explained better. So what I did is sort these and group them by the warning level assigned by MSVC. Actually, one of them (the "unreferenced label" one) was not enabled by default on any level, and conversely I think we do actually want that one enabled, since we use -Wunused-label on other compilers, so I changed the disablement to an enablement. Then I also found a few more warnings that would be useful to enable. So I'm proposing to add warnings that are similar to -Wformat and -Wswitch. Here are some documentation links: https://learn.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level?view=msvc-170 https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warnings-c4000-c5999?view=msvc-170 https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warnings-by-compiler-version?view=msvc-170 With that done, I also looked at the disabled warnings to see what could be done to fix the underlying issues. In particular, I looked at '/wd4273', # inconsistent DLL linkage If you enable that one, you get this: ../src/backend/utils/misc/ps_status.c(27): warning C4273: '__p__environ': inconsistent dll linkage C:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\ucrt\stdlib.h(1158): note: see previous definition of '__p__environ' The declaration in ps_status.c was: #if !defined(WIN32) || defined(_MSC_VER) extern char **environ; #endif The declaration in the OS header file is: _DCRTIMP char*** __cdecl __p__environ (void); #define _environ (*__p__environ()) So it is clear why a linker might be upset about this. To fix this, we can just remove the || defined(_MSC_VER). Note that these conditionals around the environ declarations were added only somewhat recently in commit 7bc9a8bdd2d ("Fix warnings about declaration of environ on MinGW."). Maybe there are older versions of Windows where the declaration is needed, maybe this is a ucrt vs. msvcrt thing, don't know, testing is welcome. The business in ps_status.c is kind of weird anyway, because we only need the environ variable in the PS_USE_CLOBBER_ARGV case, which is not Windows, so maybe we should move some things around to avoid the problem in this file. But there are also a few other files where environ is declared for use by Windows as well. -
Re: MSVC: Improve warning options set
Bryan Green <dbryan.green@gmail.com> — 2025-10-31T13:31:20Z
On 10/29/2025 1:51 AM, Peter Eisentraut wrote: > meson.build has a list of warnings to disable on MSVC: > > cflags_warn += [ > '/wd4018', # signed/unsigned mismatch > '/wd4244', # conversion from 'type1' to 'type2', possible loss of data > '/wd4273', # inconsistent DLL linkage > '/wd4101', # unreferenced local variable > '/wd4102', # unreferenced label > '/wd4090', # different 'modifier' qualifiers > '/wd4267', # conversion from 'size_t' to 'type', possible loss of data > ] > > First, these appear to be in some random order, so I wanted to sort > them. But then it also appeared that for some of these, if you remove > the disablement, nothing changes, so it seemed some of these entries are > not needed. > > Some of these warnings are assigned to higher warning levels, so if > someone wanted to compile PostgreSQL with a higher warning level on MSVC > (similar to -Wextra), then disabling some from the higher levels is > useful. But I figured this could be explained better. > > So what I did is sort these and group them by the warning level assigned > by MSVC. > > Actually, one of them (the "unreferenced label" one) was not enabled by > default on any level, and conversely I think we do actually want that > one enabled, since we use -Wunused-label on other compilers, so I > changed the disablement to an enablement. > > Then I also found a few more warnings that would be useful to enable. So > I'm proposing to add warnings that are similar to -Wformat and -Wswitch. > > Here are some documentation links: > > https://learn.microsoft.com/en-us/cpp/build/reference/compiler-option- > warning-level?view=msvc-170 > https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/ > compiler-warnings-c4000-c5999?view=msvc-170 > https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/ > compiler-warnings-by-compiler-version?view=msvc-170 > > > With that done, I also looked at the disabled warnings to see what could > be done to fix the underlying issues. In particular, I looked at > > '/wd4273', # inconsistent DLL linkage > > If you enable that one, you get this: > > ../src/backend/utils/misc/ps_status.c(27): warning C4273: > '__p__environ': inconsistent dll linkage > C:\Program Files (x86)\Windows > Kits\10\include\10.0.22621.0\ucrt\stdlib.h(1158): note: see previous > definition of '__p__environ' > > The declaration in ps_status.c was: > > #if !defined(WIN32) || defined(_MSC_VER) > extern char **environ; > #endif > > The declaration in the OS header file is: > > _DCRTIMP char*** __cdecl __p__environ (void); > #define _environ (*__p__environ()) > > So it is clear why a linker might be upset about this. > > To fix this, we can just remove the || defined(_MSC_VER). > > Note that these conditionals around the environ declarations were added > only somewhat recently in commit 7bc9a8bdd2d ("Fix warnings about > declaration of environ on MinGW."). > > Maybe there are older versions of Windows where the declaration is > needed, maybe this is a ucrt vs. msvcrt thing, don't know, testing is > welcome. > > > The business in ps_status.c is kind of weird anyway, because we only > need the environ variable in the PS_USE_CLOBBER_ARGV case, which is not > Windows, so maybe we should move some things around to avoid the problem > in this file. But there are also a few other files where environ is > declared for use by Windows as well. Good cleanup. The warning level organization makes the file much more maintainable, and switching C4102 to enabled is clearly correct. Regarding the environ declaration-- it comes down to which C runtime is being targeted. The old MSVCRT (msvcrt.dll) actually exported environ as a data symbol, so declaring "extern char **environ;" worked fine. MinGW traditionally targeted this runtime, and older MSVC versions used it too. The Universal CRT (UCRT), introduced with VS2015, changed the ABI. It doesn't export environ directly—instead it exports __p__environ() as a function and provides the _environ macro. That's why modern MSVC complains about the declaration. So when commit 7bc9a8bdd2d added || defined(_MSC_VER), it was probably correct for whatever toolchains were supported at that time. But if we now require VS2015+ (which I think we do), then removing that condition makes sense. The real question is MinGW. If we still support MinGW builds targeting the old MSVCRT, those need the environ declaration. If we require MinGW with UCRT, we don't. You'd need something like "#if defined(MINGW32) && !defined(_UCRT)" to distinguish them, if it matters. But yeah, you're right that the whole thing is weird for ps_status.c specifically, since Windows never uses PS_USE_CLOBBER_ARGV anyway. Might as well just guard it with that directly. What's our minimum supported MSVC version these days? I am partially interested in this answer because it would be aesthetically pleasing to get rid of the unneeded ones listed in win32env.c-- static const char *const modulenames[] = { "msvcrt", /* Visual Studio 6.0 / MinGW */ "msvcrtd", "msvcr70", /* Visual Studio 2002 */ "msvcr70d", "msvcr71", /* Visual Studio 2003 */ "msvcr71d", "msvcr80", /* Visual Studio 2005 */ "msvcr80d", "msvcr90", /* Visual Studio 2008 */ "msvcr90d", "msvcr100", /* Visual Studio 2010 */ "msvcr100d", "msvcr110", /* Visual Studio 2012 */ "msvcr110d", "msvcr120", /* Visual Studio 2013 */ "msvcr120d", "ucrtbase", /* Visual Studio 2015 and later */ "ucrtbased", NULL }; And do we care about MinGW with old MSVCRT? Bryan -
Re: MSVC: Improve warning options set
Peter Eisentraut <peter@eisentraut.org> — 2025-11-03T18:56:16Z
On 31.10.25 14:31, Bryan Green wrote: > Regarding the environ declaration-- it comes down to which C runtime is > being targeted. > > The old MSVCRT (msvcrt.dll) actually exported environ as a data symbol, > so declaring "extern char **environ;" worked fine. MinGW traditionally > targeted this runtime, and older MSVC versions used it too. > > The Universal CRT (UCRT), introduced with VS2015, changed the ABI. It > doesn't export environ directly—instead it exports __p__environ() as a > function and provides the _environ macro. That's why modern MSVC > complains about the declaration. > > So when commit 7bc9a8bdd2d added || defined(_MSC_VER), it was probably > correct for whatever toolchains were supported at that time. But if we > now require VS2015+ (which I think we do), then removing that condition > makes sense. > > The real question is MinGW. If we still support MinGW builds targeting > the old MSVCRT, those need the environ declaration. If we require MinGW > with UCRT, we don't. You'd need something like "#if defined(MINGW32) > && !defined(_UCRT)" to distinguish them, if it matters. As of commit 1758d424461 (PG18) we require UCRT if using MinGW. I don't know if we still support MSVCRT if using MSVC, or how long we still need to support it. (Or, for example, how to tell which variant CI or the buildfarm uses.) > What's our minimum supported MSVC version these days? I am partially > interested in this answer because it would be aesthetically pleasing to > get rid of the unneeded ones listed in win32env.c-- As of commit 8fd9bb1d965 (PG19) we require Visual Studio 2019. > static const char *const modulenames[] = { > "msvcrt", /* Visual Studio 6.0 / MinGW */ > "msvcrtd", > "msvcr70", /* Visual Studio 2002 */ > "msvcr70d", > "msvcr71", /* Visual Studio 2003 */ > "msvcr71d", > "msvcr80", /* Visual Studio 2005 */ > "msvcr80d", > "msvcr90", /* Visual Studio 2008 */ > "msvcr90d", > "msvcr100", /* Visual Studio 2010 */ > "msvcr100d", > "msvcr110", /* Visual Studio 2012 */ > "msvcr110d", > "msvcr120", /* Visual Studio 2013 */ > "msvcr120d", > "ucrtbase", /* Visual Studio 2015 and later */ > "ucrtbased", > NULL > }; So that would mean we can remove all but the two ucrt* entries? Is there no more msvcr* after VS 2015? -
Re: MSVC: Improve warning options set
Andres Freund <andres@anarazel.de> — 2025-11-03T19:15:59Z
Hi, On 2025-10-29 08:51:00 +0100, Peter Eisentraut wrote: > meson.build has a list of warnings to disable on MSVC: > > cflags_warn += [ > '/wd4018', # signed/unsigned mismatch > '/wd4244', # conversion from 'type1' to 'type2', possible loss of data > '/wd4273', # inconsistent DLL linkage > '/wd4101', # unreferenced local variable > '/wd4102', # unreferenced label > '/wd4090', # different 'modifier' qualifiers > '/wd4267', # conversion from 'size_t' to 'type', possible loss of data > ] > > First, these appear to be in some random order, so I wanted to sort them. FWIW, I just transported them 1:1 from src/tools/msvc, at the time it seemed more important to keep things consistent than to clean up. Greetings, Andres Freund
-
Re: MSVC: Improve warning options set
Andres Freund <andres@anarazel.de> — 2025-11-03T19:26:25Z
Hi, On 2025-11-03 19:56:16 +0100, Peter Eisentraut wrote: > I don't know if we still support MSVCRT if using MSVC, or how long we still > need to support it. (Or, for example, how to tell which variant CI or the > buildfarm uses.) To my knowledge anything close to a recent version visual studio / msvc don't use msvcrt anymore. Starting at least with VS 2015. I don't think our code would really work when building against msvcrt anyway (mingw worked for longer, because they added additional functionality in wrapper libraries, IIRC). > > static const char *const modulenames[] = { > > "msvcrt", /* Visual Studio 6.0 / MinGW */ > > "msvcrtd", > > "msvcr70", /* Visual Studio 2002 */ > > "msvcr70d", > > "msvcr71", /* Visual Studio 2003 */ > > "msvcr71d", > > "msvcr80", /* Visual Studio 2005 */ > > "msvcr80d", > > "msvcr90", /* Visual Studio 2008 */ > > "msvcr90d", > > "msvcr100", /* Visual Studio 2010 */ > > "msvcr100d", > > "msvcr110", /* Visual Studio 2012 */ > > "msvcr110d", > > "msvcr120", /* Visual Studio 2013 */ > > "msvcr120d", > > "ucrtbase", /* Visual Studio 2015 and later */ > > "ucrtbased", > > NULL > > }; > > So that would mean we can remove all but the two ucrt* entries? Is there no > more msvcr* after VS 2015? Correct, there's no msvcrt*dll anymore for anything recent. I think there's still link libraries named like similarly, but that shouldn't matter here. However - that doesn't necessarily mean that no msvcrt could be loaded into the same process, e.g. when linking against an older library that's built against msvcrt. On windows multiple runtime libraries can exist in the same process (which is why it's not safe to allocate/deallocate memory or open/close files across library boundaries). I'm not sure the gain from pruning the above list is worth the potential subtle breakage that could result. Greetings, Andres Freund -
Re: MSVC: Improve warning options set
Peter Eisentraut <peter@eisentraut.org> — 2025-11-08T16:33:29Z
On 03.11.25 20:26, Andres Freund wrote: > On 2025-11-03 19:56:16 +0100, Peter Eisentraut wrote: >> I don't know if we still support MSVCRT if using MSVC, or how long we still >> need to support it. (Or, for example, how to tell which variant CI or the >> buildfarm uses.) > > To my knowledge anything close to a recent version visual studio / msvc don't > use msvcrt anymore. Starting at least with VS 2015. I don't think our code > would really work when building against msvcrt anyway (mingw worked for > longer, because they added additional functionality in wrapper libraries, > IIRC). I have committed both of the patches as proposed, and both relevant buildfarm members have had no complaints.
-
Re: MSVC: Improve warning options set
Thomas Munro <thomas.munro@gmail.com> — 2025-11-08T21:40:26Z
On Tue, Nov 4, 2025 at 7:56 AM Peter Eisentraut <peter@eisentraut.org> wrote: > On 31.10.25 14:31, Bryan Green wrote: > > The real question is MinGW. If we still support MinGW builds targeting > > the old MSVCRT, those need the environ declaration. If we require MinGW > > with UCRT, we don't. You'd need something like "#if defined(MINGW32) > > && !defined(_UCRT)" to distinguish them, if it matters. > > As of commit 1758d424461 (PG18) we require UCRT if using MinGW. > > I don't know if we still support MSVCRT if using MSVC, or how long we > still need to support it. (Or, for example, how to tell which variant > CI or the buildfarm uses.) The main CI task and build farm animal fairywren use UCRT, so there is no automated test coverage for the old runtime and hasn't been for some time, and that's also the default with that toolchain these day. There is still one place that builds (without running) against MSVCRT in CI: the CompilerWarnings mingw cross-compilation task that runs on Debian. In commit 1758d424461, we officially stopped thinking about the implications for MSVCRT builds and allowed ourselves to begin ripping out the long tail of weird distortions in our code due to that historical train wreck, but we were unsure when exactly PostgreSQL would become actually unbuildable with MSVCRT and I for one was not aware that the cross-build was doing that at the time. Unfortunately it didn't ever seem to become unbuildable, but apparently things break in undiagnosed ways at runtime (at a guess it might have some API calls that are stubbed out with empty implementations or something like that, but there is zero reason to investigate that, it's dead). What we should do to make this clearer and avoid spurious problem reports is error out unless you're on UCRT, but a patch for that got stuck waiting for the Debian images used on CI to be upgraded to Debian trixie, because that shipped the necessary newer MinGW/headers/etc in its cross-build packages. That has now happened, so we should probably go ahead with something like the patch I posted here: https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BQJv-g7C2APVV7O_jEJkxH1AmvgAe8X2vDR8mRdSKn3A%40mail.gmail.com#e6d0c91e2f59e6e39eb61095da4cc598 In theory we could even back-patch that to 18, since we already know it won't fully work and we already declared that we don't support it. Or we could just let sleeping dogs lie and do that for 19.
-
Re: MSVC: Improve warning options set
Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-08T21:58:23Z
Thomas Munro <thomas.munro@gmail.com> writes: > What we should do to make this clearer and avoid spurious problem > reports is error out unless you're on UCRT, but a patch for that got > stuck waiting for the Debian images used on CI to be upgraded to > Debian trixie, because that shipped the necessary newer > MinGW/headers/etc in its cross-build packages. That has now happened, > so we should probably go ahead with something like the patch I posted > here: > https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BQJv-g7C2APVV7O_jEJkxH1AmvgAe8X2vDR8mRdSKn3A%40mail.gmail.com#e6d0c91e2f59e6e39eb61095da4cc598 +1 > In theory we could even back-patch that to 18, since we already know > it won't fully work and we already declared that we don't support it. > Or we could just let sleeping dogs lie and do that for 19. I don't quite understand how 1758d4244 didn't break building with MSVCRT? But if it builds yet doesn't in fact work, that's likely to draw complaints from people who didn't spot the documentation change. So I'd lean towards back-patching. regards, tom lane
-
Re: MSVC: Improve warning options set
Thomas Munro <thomas.munro@gmail.com> — 2025-11-09T00:29:36Z
On Sun, Nov 9, 2025 at 10:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I don't quite understand how 1758d4244 didn't break building with > MSVCRT? But if it builds yet doesn't in fact work, that's likely to > draw complaints from people who didn't spot the documentation change. I think it links against MinGW shims that at least in some cases can be replaced at runtime with something looked up by dlsym(), but are otherwise no-op/fail implementations. I expect it was a moving target over the decades, because the shims were added with potentially long lag, ie when someone cared enough to write the patch because their program was affected. In that problem report we had 'The operating system could not find any locale data for the locale name "en-US"', but it beats me whether that was "dummy _create_locale() always returns NULL[1]" or "found MSVCRT's version of _create_locale() but it expects locale data files installed at a different location than UCRT's and they aren't there because MSVCRT is dead and we're chasing ghosts" :-) > So I'd lean towards back-patching. Cool. Will do that after the freeze, unless someone show up with a concrete reason not to. [1] https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-crt/misc/_create_locale.c#L13
-
Re: MSVC: Improve warning options set
Andres Freund <andres@anarazel.de> — 2025-11-09T03:21:20Z
Hi, On November 8, 2025 4:40:26 PM EST, Thomas Munro <thomas.munro@gmail.com> wrote: >On Tue, Nov 4, 2025 at 7:56 AM Peter Eisentraut <peter@eisentraut.org> wrote: >> On 31.10.25 14:31, Bryan Green wrote: >> > The real question is MinGW. If we still support MinGW builds targeting >> > the old MSVCRT, those need the environ declaration. If we require MinGW >> > with UCRT, we don't. You'd need something like "#if defined(MINGW32) >> > && !defined(_UCRT)" to distinguish them, if it matters. >> >> As of commit 1758d424461 (PG18) we require UCRT if using MinGW. >> >> I don't know if we still support MSVCRT if using MSVC, or how long we >> still need to support it. (Or, for example, how to tell which variant >> CI or the buildfarm uses.) > >The main CI task and build farm animal fairywren use UCRT, so there is >no automated test coverage for the old runtime and hasn't been for >some time, and that's also the default with that toolchain these day. >There is still one place that builds (without running) against MSVCRT >in CI: the CompilerWarnings mingw cross-compilation task that runs on >Debian. Pretty sure it's now using ucrt too, that had to be changed as part of the update to Trixie, otherwise the builds would fail. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
-
Re: MSVC: Improve warning options set
Peter Eisentraut <peter@eisentraut.org> — 2025-11-10T15:34:10Z
On 08.11.25 22:40, Thomas Munro wrote: > Unfortunately > it didn't ever seem to become unbuildable, but apparently things break > in undiagnosed ways at runtime (at a guess it might have some API > calls that are stubbed out with empty implementations or something > like that, but there is zero reason to investigate that, it's dead). > What we should do to make this clearer and avoid spurious problem > reports is error out unless you're on UCRT, but a patch for that got > stuck waiting for the Debian images used on CI to be upgraded to > Debian trixie, because that shipped the necessary newer > MinGW/headers/etc in its cross-build packages. That has now happened, > so we should probably go ahead with something like the patch I posted > here: > > https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BQJv- > g7C2APVV7O_jEJkxH1AmvgAe8X2vDR8mRdSKn3A%40mail.gmail.com#e6d0c91e2f59e6e39eb61095da4cc598 > > In theory we could even back-patch that to 18, since we already know > it won't fully work and we already declared that we don't support it. > Or we could just let sleeping dogs lie and do that for 19. When you build master under the msys2 msvcrt environment now, various regression tests fail, related to floating point differences and locales not being found. So I think this regulates itself and we don't really need to do anything further.