| From: | Peter Eisentraut <peter(at)eisentraut(dot)org> | 
|---|---|
| To: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> | 
| Subject: | MSVC: Improve warning options set | 
| Date: | 2025-10-29 07:51:00 | 
| Message-ID: | bf060644-47ff-441b-97cf-c685d0827757@eisentraut.org | 
| Views: | Whole Thread | Raw Message | Download mbox | Resend email | 
| Thread: | |
| Lists: | pgsql-hackers | 
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.
| Attachment | Content-Type | Size | 
|---|---|---|
| 0001-MSVC-Improve-warning-options-set.patch | text/plain | 2.1 KB | 
| 0002-Fix-inconsistent-DLL-linkage-warning-on-Windows-MSVC.patch | text/plain | 3.1 KB | 
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chao Li | 2025-10-29 08:25:03 | Re: tuple radix sort | 
| Previous Message | Chao Li | 2025-10-29 07:05:42 | Re: Optimize LISTEN/NOTIFY |