Move JIT check to BigInteger

[danog]

This fixes static analysis with Psalm v5 (which enables JIT by default) of projects which require phpseclib as a dependency.

[terrafrost]

First, I apologize for the delay. I'm currently vacationing and am doing a bunch of outdoorsy stuff wherein I don't have access to my computer.

Anyway, I'm kinda confused by this. How does Psalm v5 enable JIT by default?

Using the install instructions at https://psalm.dev/docs/running_psalm/installation/ (either with Composer or the Phar) it'd use your existing PHP setup.

That said, I suppose that's somewhat immaterial to the merits of the proposed change. So in-so-far as those are concerned... I can see pros and cons of going with either approach.

Pros of the current approach
So I still stand by what I said in #1941 (comment) - if JIT on Windows is going to behave unpredictably in one scenario I can't be sure that there aren't other scenarios in which where it'll behave just as unpredictably. Maybe once php/php-src#11917 is fixed I could look at the fix and see if there specific conditions for the issue and then implement workarounds when those conditions are met but, as is, I don't have a good enough grasp on the underlying issue to be able to do that.

Pros of your approach
Thus far the only confirmed problem with JIT on Windows manifests itself in BigInteger when the PHP64 engine is used. It's possible that other issues that have defied diagnose (ie. maybe someone makes a bug report and then never follows up when I ask for more info) are attributable to this but I have no way of knowing that for sure. If I were able to identify other scenarios in which this problem occurs that might help the PHP devs hone in on and ultimately fix the underlying issue. Of course that does put more of the onus on me.

A Possible Middleground?
In lieu of knowing how Psalm v5 enables JIT by default and in recognition of your own contributions to open source I guess what I'm thinking is this: move the check over to Math/BigInteger/Engines/PHP64.php and PHP32.php. Make isValidEngine return false if JIT is enabled on Windows. Don't throw an exception, don't check for PHPSECLIB_ALLOW_JIT, just return false. Or better yet, in PHP.php (which both PHP64.php and PHP32.php extend), add a new method - testJITOnWindows() - and then call that method from isValidEngine() in both PHP32.php and PHP64.php.

Then, in initialize_static_variables(), in Math/BigInteger.php, make it so that if no valid engine is found that an exception is thrown. eg. maybe do $foundEngine = false; before the for loop and set that to true after self::setEngine($engine[0], $engine[1]);. Then, after the for loop, if !$foundEngine then do throw new UnexpectedValueException('No valid BigInteger found. This is only possible when JIT is enabled on Windows and neither the GMP or BCMath extensions are available so either disable JIT or install GMP / BCMath'); or some such.

The advantage of doing that vs what you're doing is that people can still use the GMP or BCMath engines on Windows even when JIT is enabled.

[danog]

Hi, thank you for the review!

Anyway, I'm kinda confused by this. How does Psalm v5 enable JIT by default?

Sorry, should've been more detailed in the description of the PR :)
Psalm v5 enables function JIT by default in order to improve performance, currently we have around a 20% improvement!

Using tracing JIT can probably raise that number even more but as you saw, tracing JIT is quite buggy right now, plus function JIT does not rely on runtime type information, so bugs are more easily reproducible than with tracing JIT.

A Possible Middleground?

Thank you, implemented the changes you suggested: I did it without the additional $foundEngine boolean variable since just returning from the initialize_static_variables function on success seemed like the simplest solution to me, can add it back if you want to.

Also, I've kept the PHPSECLIB_ALLOW_JIT environment variable, mostly because I intend to submit another PR to php-src to add a nightly test suite for Windows as well, which will test phpseclib (and other popular FOSS libraries) with JIT to find bugs and regressions, and that flag will really come in useful.

[richard67]

Will there be a release which includes this change? If I see right, the latest 3.0.23 doesn't include it.

[terrafrost]

I'll try to do a release this week.

[terrafrost]

3.0.33 has been released!

[richard67]

Yes, I saw. Thanks a lot.

[AltamashShaikh]

@terrafrost @danog The exception added with this PR seems to create issue for us, as we are using the bcmath polyfill