Add IL compat flag to Windows builds#6658
Add IL compat flag to Windows builds#6658dwgebler wants to merge 3 commits intophp:masterphp/php-src:masterfrom dwgebler:allow-compat-il-flagdwgebler/php-src:allow-compat-il-flagCopy head branch name to clipboard
Conversation
|
Unrelated, I have another open PR #6650 just in case anyone fancies reviewing that too. |
cmb69
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Generally, I think this is a useful addition, since to my knowledge it is not possible to install a certain Visual Studio version so we could require this particular version (instead you only can update to the latest release for VS 2019), and otherwise C1047 can indeed happen. Rebuilding and deploying the dependencies for each VS release is not an option for bandwidth reasons. The only minor concern is regarding the official builds, where this option might slightly affect the optimization (/LTCG) if incompatible libs are used. @dwgebler, are you aware of a possibility to override this undocumented linker option for individual builds, or at least to get a prominent warning if an incompatible lib is found?
Regarding the current implementation: I don't think we should set flag globally. It should only be set if link.exe is actually used; that is not the case for the ICC_TOOLSET, though (I don't know, however, whether this is still supported). It probably makes sense to move the setting of the flag into toolset_setup_common_ldlags() [sic] in confutils.js, guarded by VS_TOOLSET (maybe apply it to CLANG_TOOLSET as well, but I think this is broken anyway since OPcache JIT has been added).
It's hard to be certain about the effects of this switch on LTCG optimization, since it is undocumented; I'm not clear it has any effect on /LTCG when the IL versions are actually compatible if different. Compatibility may mean there is no effect; the criteria here are unfortunately opaque. In respect of overriding, I think on the whole it's probably better to explicitly turn this on for the builds you want. Maybe add this flag to |
ed7c699 to
ca9747a
Compare
|
I don't see an advantage of introducing a new config option to explicitly enable Anyhow, I think this option is supported by older VS versions as well (explicitly setting |
9284db3 to
ca9747a
Compare
cmb69
left a comment
There was a problem hiding this comment.
Thanks, looks good me now! (nit: indentation should be done with tabs in this file; I'm going to change that right away)
See microsoft/php-sdk-binary-tools#72 for background.