Обсуждение: MSVC: Improve warning options set

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.

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

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?

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

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