Re: MSVC: Improve warning options set

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