[PATCH v3 0/1] MR6377: winebuild: Use know 14.29 version for the linker version of dll files

[Lorenzo Ferrillo (＠llde)]

PHP 8.2.7 check that vcruntime140.dll is have the linker version equal to the version of the linker of the toolchain used to build itself, and refuse to start otherwise, with this error message:

`PHP Warning:  'C:\windows\system32\VCRUNTIME140.dll' 2.39 is not compatible with this PHP build linked with 14.29 in Unknown on line 0`

Setting the linker version in winebuild to 14.29 allow php to pass this check.

If there are concerns to having this change global, I could try to make this as argument to winebuild with the value setted somewhere into the build system

Signed-off-by: Lorenzo Ferrillo

--
  v3: winebuild: Use know 14.29 version for the linker version of dll files

https://gitlab.winehq.org/wine/wine/-/merge_requests/6377

[PATCH v3 1/1] winebuild: Use know 14.29 version for the linker version of dll files

[Lorenzo Ferrillo]

From: Lorenzo Ferrillo

v2: fix mixed declarations and code
Signed-off-by: Lorenzo Ferrillo 
---
 tools/winebuild/spec32.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/winebuild/spec32.c b/tools/winebuild/spec32.c
index f06c40ca9b1..8c05151267e 100644
--- a/tools/winebuild/spec32.c
+++ b/tools/winebuild/spec32.c
@@ -45,6 +45,8 @@
 #define IMAGE_NT_OPTIONAL_HDR64_MAGIC 0x20b
 #define IMAGE_ROM_OPTIONAL_HDR_MAGIC  0x107

+#define LINKER_VERSION_MAJOR 14
+#define LINKER_VERSION_MINOR 29    /*14.29 vs2019 64bit redist*/
 int needs_get_pc_thunk = 0;

static const char builtin_signature[32] = "Wine builtin DLL";
@@ -798,8 +800,8 @@ void output_module( DLLSPEC *spec )
              spec->characteristics );
     output( "\t.short 0x%04x\n",          /* Magic */
              get_ptr_size() == 8 ? IMAGE_NT_OPTIONAL_HDR64_MAGIC : IMAGE_NT_OPTIONAL_HDR32_MAGIC );
-    output( "\t.byte 7\n" );              /* MajorLinkerVersion */
-    output( "\t.byte 10\n" );             /* MinorLinkerVersion */
+    output( "\t.byte %u\n", LINKER_VERSION_MAJOR);              /* MajorLinkerVersion */
+    output( "\t.byte %u\n", LINKER_VERSION_MINOR );             /* MinorLinkerVersion */
     output( "\t.long 0\n" );              /* SizeOfCode */
     output( "\t.long 0\n" );              /* SizeOfInitializedData */
     output( "\t.long 0\n" );              /* SizeOfUninitializedData */
@@ -1217,8 +1219,8 @@ static void output_pe_file( DLLSPEC *spec, const char signature[32] )
     put_word( get_ptr_size() == 8 ?
               IMAGE_NT_OPTIONAL_HDR64_MAGIC :
               IMAGE_NT_OPTIONAL_HDR32_MAGIC );       /* Magic */
-    put_byte(  7 );                                  /* MajorLinkerVersion */
-    put_byte(  10 );                                 /* MinorLinkerVersion */
+    put_byte(  LINKER_VERSION_MAJOR );                                  /* MajorLinkerVersion */
+    put_byte(  LINKER_VERSION_MINOR );                                 /* MinorLinkerVersion */
     put_dword( code_size );                          /* SizeOfCode */
     put_dword( data_size );                          /* SizeOfInitializedData */
     put_dword( 0 );                                  /* SizeOfUninitializedData */
@@ -1473,12 +1475,14 @@ void output_def_file( DLLSPEC *spec, struct exports *exports, int import_only )
 void make_builtin_files( struct strarray files )
 {
     int i, fd;
+    char linker_version[2] = {LINKER_VERSION_MAJOR, LINKER_VERSION_MINOR};
     struct
     {
         unsigned short e_magic;
         unsigned short unused[29];
         unsigned int   e_lfanew;
     } header;
+    unsigned int pos_linker;

for (i = 0; i < files.count; i++)
     {
@@ -1490,6 +1494,10 @@ void make_builtin_files( struct strarray files )
                 fatal_error( "%s: Not enough space (%x) for Wine signature\n", files.str[i], header.e_lfanew );
             write( fd, builtin_signature, sizeof(builtin_signature) );

+            pos_linker = header.e_lfanew + 0x1a;
+            lseek( fd, pos_linker, SEEK_SET );
+            write(fd,linker_version, sizeof(linker_version));
+
             if (prefer_native)
             {
                 unsigned int pos = header.e_lfanew + 0x5e;  /* OptionalHeader.DllCharacteristics */

[Nikolay Sivov (＠nsivov)]

The claim [1] is that using newer VS tools creates binaries incompatible with vcruntime140.dll built with older VS. I think it makes sense to contact php dev who made this change, how they reached this conclusion.

[1] https://github.com/php/php-src/blob/9b73d591c6b00ef962229ee565a85bd9b1e9305c...

[Lorenzo Ferrillo (＠llde)]

hi @nsivov. 
These checks were added in https://github.com/php/php-src/commit/ddce7ada4c319dafa5cead0c0fb560a659f076...
and modified in https://github.com/php/php-src/commit/dd0aca0c11f2572e37fd9583866f37d895a80e...

According to the second commit they implemented it to follow a recommendation from a devblog regarding binary compatibility.

I tried to search for examples of this kind of breaking  and I found something like this https://github.com/microsoft/STL/issues/4730  (related to adding a constexpr to a mutex constructor)
They are however caused by specific changes in the core libraries code and headers (and only if the code used that feature) and not by the only "virtue" of being compiled with a newer toolchain. (otherwise wine libraries, being compiled with a totally different toolchain won't work at all)
(However I have to say, how that modification is compatible with code compiled before that changes eludes me, considering the virtual table changes)

[Nikolay Sivov (＠nsivov)]

If it's really a c++ feature issue, it would make sense if they checked msvcp140.dll instead. Setting arbitrary version without understanding what's the problem was is not great, do we have this issue in our msvcp140? Does it correspond to newer or older behavior?

And if this is in fact a problem that's not fixable at application side, e.g. by never using constructs that supposedly cause the issue, "correct" way seems to me is to bundle crt modules of a matching version. Or asking user to install updated redist package.

[Lorenzo Ferrillo (＠llde)]

> If it's really a c++ feature issue, it would make sense if they checked msvcp140.dll instead. Setting arbitrary version without understanding what's the problem was is not great, do we have this issue in our msvcp140? Does it correspond to newer or older behavior?

I mean everything I found is for C++, this doesn't necessarily mean C is unaffected. Regarding our lib, it seems to me that is using the previous abi, but I'm not sure, as I don't know what kind of breakage that change does.
Note that this is a tangential matter, as this change was introduced way after 14.29
> And if this is in fact a problem that's not fixable at application side, e.g. by never using constructs that supposedly cause the issue

An application that doesn't use the problematic code (in this case mutexes) won't be affected by the change.
> "correct" way seems to me is to bundle crt modules of a matching version. Or asking user to install updated redist package.

We could do that, but wouldn't this loose the point of shipping it inside wine? 
And for what I can see PHP works with this change, barren a problem with traversing the mount points (and symlinks with the junction patchset), without the need to actually using a native version of the runtime. It seems also it's one of the few applications making this kind of check.
And imho, if the windows runtime get updated, it should be responsability of the wine project to make the wine implementation conformant again (excpecially if we have now a guarantee of forward compatibility).