Fix compile for Windows x86 with /W1 /WX options (warnings are fatal)

[mvorisek]

Thankfully there are only a few issues to fix to remove all x86 warnings failing the compile with /WX option, so we fix them.

[mvorisek]

@cmb69 any idea how to overcome this:

c:\build-cache\deps-8.0-vs16-x86\include\net-snmp/net-snmp-config.h(1370): warning C4142: 'SSIZE_T': benign redefinition of type

warning on Windows 32 build?

the file is from https://windows.php.net/downloads/php-sdk/deps/vs16/x86/net-snmp-5.7.3-1-vs16-x86.zip

do you think we can avoid the type redefinition and patch the net-snmp-config.h easily?

[cmb69]

do you think we can avoid the type redefinition and patch the net-snmp-config.h easily?

I wouldn't want to monkey patch this, but it should be fixed in winlibs/net-snmp. And actually, net-snmp needs to be updated anyway; net-snmp 5.9.1 is available for almost a year, while our old 5.7.3 has been released more than 9 years ago.

Anyhow, I strongly suggest to only target the "master" branch for /WX.

PS: to avoid the need to look that up again: from basetsd.h:

//
// SIZE_T used for counts or ranges which need to span the range of
// of a pointer.  SSIZE_T is the signed variation.
//

typedef ULONG_PTR SIZE_T, *PSIZE_T;
typedef LONG_PTR SSIZE_T, *PSSIZE_T;

[Girgias]

IMHO this should only target master

[mvorisek]

IMHO this should only target master

why? If there are production/compiler issues, why the bugfixes should be be put into the lowest supported branch?

again, draft with #8392, once I am done, I will keep here only the relevant changes

[cmb69]

If there are real bugs, those should be fixed for PHP-8.0+, but I assume that most (if not all) of these warnings do not indicate real bugs.

[mvorisek]

@cmb69 do you understand the cause of

ext\gmp\gmp.c(373): error C2220: the following warning is treated as an error
ext\gmp\gmp.c(373): warning C4028: formal parameter 3 different from declaration
ext\gmp\gmp.c(375): warning C4028: formal parameter 3 different from declaration
ext\gmp\gmp.c(377): warning C4028: formal parameter 3 different from declaration
ext\gmp\gmp.c(379): warning C4028: formal parameter 3 different from declaration
ext\gmp\gmp.c(386): warning C4028: formal parameter 3 different from declaration
ext\gmp\gmp.c(389): warning C4028: formal parameter 3 different from declaration
ext\gmp\gmp.c(1046): warning C4028: formal parameter 3 different from declaration
ext\gmp\gmp.c(1053): warning C4028: formal parameter 3 different from declaration
ext\gmp\gmp.c(1060): warning C4028: formal parameter 3 different from declaration
ext\gmp\gmp.c(1076): warning C4028: formal parameter 4 different from declaration
ext\gmp\gmp.c(1079): warning C4028: formal parameter 4 different from declaration
ext\gmp\gmp.c(1082): warning C4028: formal parameter 4 different from declaration
ext\gmp\gmp.c(1521): warning C4028: formal parameter 3 different from declaration

warnings?

[cmb69]

The GMP issue should be fixed with

 ext/gmp/php_gmp_int.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/gmp/php_gmp_int.h b/ext/gmp/php_gmp_int.h
index d4ef5f0157..c37fdb136d 100644
--- a/ext/gmp/php_gmp_int.h
+++ b/ext/gmp/php_gmp_int.h
@@ -28,7 +28,7 @@ static inline gmp_object *php_gmp_object_from_zend_object(zend_object *zobj) {
 PHP_GMP_API zend_class_entry *php_gmp_class_entry(void);
 
 /* GMP and MPIR use different datatypes on different platforms */
-#ifdef PHP_WIN32
+#ifdef _WIN64
 typedef zend_long gmp_long;
 typedef zend_ulong gmp_ulong;
 #else

However, I don't understand why MSVC yields the warning. int32_t should be the same as long, and uint32_t should be the same as unsigned long on x86.

[mvorisek]

@cmb69 thank you a lot for helping me with the fixes!

The proposed changes fixes all MSVC warnings so php can be build with /W1 /WX switches.

Patching the one include for CI is legit in my opinion, as it highly improves the CI quality assurance (any newly introduced warning will fail CI) and the one removed typedef via patch is safe as if would be needed, it would fail CI as well. However, I would definitely not object patching the source https://windows.php.net/downloads/php-sdk/deps/vs16/x86/net-snmp-5.7.3-1-vs16-x86.zip.

[cmb69]

Thanks for the PR! I don't see any actual bug (the compiler warnings don't exhibit any), but I think this is a good improvement for the master branch. Before we merge this, I like to figure out improvements to the net-snmp dependency, so the monkey patching might not be needed. Locally, I have 29 snmp test passing, but 4 are failing; I need to investigate this first.

[mvorisek]

Targetting based on bugfix/feature mantra is clear to me. However, I do not understand why CI/quality assurance should not target the lowest active branch. This not only helps to develop the actual bug fixes, but also asserts the bugfix branches code quality is not unintentionally degraded.

[Girgias]

Please rebase this onto master, compiler warnings don't imply bugs and are QA changes which should only target master.

[mvorisek]

I do not agreee here, these fixes should target/fix the lowest branch as the warnings are correctly detected issues. If someone with the write access wants to change the target, feel free to do it, but I will not sign under that change.

[cmb69]

as the warnings are correctly detected issues

I still don't see that these warnings actually hint at bugs, though.

Anyhow, the changes to JIT and streams are uncontroversial, so I'm going to apply these to master. I still don't understand the GMP warning, and I think this needs closer investigation. Monkey patching the net-snmp header looks wrong to me, especially for QA, since we won't test the real thing.

[mvorisek]

Finally, all x86 problems for Windows platform are fixed a we can assert zero compiler warnings on any Windows!

The small patch should not hurt as it does not change anything, it only prevents ssize_t (re)declared.

[nielsdos]

Hmm I agree with @cmb69 's message that the snmp issue should be fixed upstream in the winlibs repo.
Also I see rem Some undefined behavior is reported on 32-bit, this should be fixed, do you know what this was talking about; and was this actually undefined behaviour or was this referring to the warnings?

[mvorisek]

Also I see rem Some undefined behavior is reported on 32-bit, this should be fixed, do you know what this was talking about; and was this actually undefined behaviour or was this referring to the warnings?

Where do you see it?

Hmm I agree with @cmb69 's message that the snmp issue should be fixed upstream in the winlibs repo

The upstream was (probably already) fixed, but php-src for Windows uses very old version, see discussion above, do you have any access to the Windows libs to update it there?

I propose the patch as it will assert quality for the whole repo and once the upstream is updated, the patching will fail, thus CI will remind itself the patch should be removed then...

[nielsdos]

Also I see rem Some undefined behavior is reported on 32-bit, this should be fixed, do you know what this was talking about; and was this actually undefined behaviour or was this referring to the warnings?

Where do you see it?

In the commit diff of this PR.

Hmm I agree with @cmb69 's message that the snmp issue should be fixed upstream in the winlibs repo

The upstream was (probably already) fixed, but php-src for Windows uses very old version, see discussion above, do you have any access to the Windows libs to update it there?

That would be ideal, unfortunately I don't have access and idk anyone active who has access...

I propose the patch as it will assert quality for the whole repo and once the upstream is updated, the patching will fail, thus CI will remind itself the patch should be removed then...

If we include the patch in a version, and that version goes out of bugfix support (but e.g. still security support), then there's a risk of silent breakage when the winlibs repo updates because CI isn't regularly run anymore, right?

[mvorisek]

In the commit diff of this PR.

I see, the comment is removed as the warnings were fixed - and this PR is to assert no warning is emit for x86 (for x64 already done sone).

If we include the patch in a version, and that version goes out of bugfix support (but e.g. still security support), then there's a risk of silent breakage when the winlibs repo updates because CI isn't regularly run anymore, right?

The GH Actions CI files are not distributed officially anywhere... I belive this should not be a problem and some breakage in longer time period is expected as not everything is locked... So if patching is the only option now, IMHO it should not be a problem and once the lib is updated, we simply remove the patch.

[mvorisek]

@nielsdos as long as we cannot patch/upgrade the source immediatelly, are you ok to merge this PR or do you have any other idea how to assert no warnings in the CI for x86?

[nielsdos]

I'm not very against the idea. But I'd like input from other contributors about this before making a decision on this...

[Girgias]

do you think we can avoid the type redefinition and patch the net-snmp-config.h easily?

I wouldn't want to monkey patch this, but it should be fixed in winlibs/net-snmp. And actually, net-snmp needs to be updated anyway; net-snmp 5.9.1 is available for almost a year, while our old 5.7.3 has been released more than 9 years ago.

Has this been done or attempted? Because I agree that monkey patching is something, I don't feel comfortable with.

[mvorisek]

Has this been done or attempted? Because I agree that monkey patching is something, I don't feel comfortable with.

Who can do it? Or is some repo somewhere for it?

[nielsdos]

Has this been done or attempted? Because I agree that monkey patching is something, I don't feel comfortable with.

Who can do it? Or is some repo somewhere for it?

https://github.com/winlibs/net-snmp

This one should be updated to the latest net-snmp, and the warning should be fixed there too (if after the upgrade to a more modern net-snmp the problem persists).

[mvorisek]

@weltling I haven't seen @cmb69 online for a while, is he still alive? I am tagging you here is you are the 2nd author of the mentioned upstream repo - can you land the patch there from this PR?

[weltling]

@mvorisek please file a PR there, I think i should be able to merge it.

After the patch is applied, the new dependency needs to be pulled into the build.

Thanks

[weltling]

Hmm, after looking at how that header is patched - it wouldn't seem to work out like that. The header is also used to build the dependency itself, that's where ssize_t define will be needed. Probably an additional check is needed there in the lib to tell if the header is included in PHP or ssize_t is already defined somehow. Does PHP define such availability macro nowadays, like HAVE_SSIZE_T, etc.?

Thanks

[mvorisek]

IDK, @nielsdos can you please look at it, you are more skilled in C/configure scripts than me.

[weltling]

In other words - please try to compile the dependency lib with your patch applied first. Once that works (plus it still behaves as expected with PHP), all is good.

Thansk

[cmb69]

Finally some progress: winlibs/net-snmp#3

I still had to apply a patch regarding this issue, since net-snmp developers are apparently not aware of LLP64 architectures. If anybody wants to provide an upstream patch, feel free to do. I won't, since it took me way to long to do this update.

Anyhow, as soon as the new version has been rolled-out, we can apply /WX for x86 builds, too.

[cmb69]

I'm closing this PR in favor of #16170. Patching net-snmp is no longer necessary. Running an x86 Windows job would be possible, but given that Windows runners are expensive (compared to Linux), that x86 Windows builds are likely used only rarely nowadays, and that we already have a nightly x86 Windows job, I think we don't need x86 Windows on pulls.

[mvorisek]

Technically, you are using my 1st commit, but thank you for all the work done behind!

[cmb69]

Oh, right! Will credit you when merging!