Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle /notify_update in mt wrapper #63

Merged
merged 2 commits into from
May 8, 2023

Conversation

huangqinjin
Copy link
Contributor

@huangqinjin huangqinjin commented Apr 14, 2023

CMake invokes mt.exe with the undocumented option, and mt.exe may exit with 0x41020001.

The exit code is truncated to 8 bits on POSIX hosts.

Translate exit code of mt.exe in bat file.

https://gitlab.kitware.com/cmake/cmake/-/blob/0991023c30ed5b83bcb1446b5bcc9c1eae028835/Source/cmcmd.cxx#L2533
https://gitlab.kitware.com/cmake/cmake/-/blob/0991023c30ed5b83bcb1446b5bcc9c1eae028835/Source/cmcmd.cxx#L2388

Fixes #45

@mstorsjo
Copy link
Owner

If this patch is tested to work (I haven't tried the syntax ${var#pattern} on something like $@), then this looks reasonable to me!

Do you happen to know in which cases cmake does try to execute mt.exe, i.e. if we'd add a minimal testcase that triggers both this issue and the one relating to CMAKE_NINJA_CMCLDEPS_RC, are we sure that we're triggering this? If we have cmake working without any custom modifications, we should add some CI coverage for it (see https://github.com/mstorsjo/msvc-wine/blob/master/.github/workflows/build.yml - currently I'm only compiling the bundled hello.c with cl.exe and clang-cl).

On the other hand, it looks like mt.exe is executed much less since https://gitlab.kitware.com/cmake/cmake/-/commit/0b552eb877b887638e8130bb6c982106a76827d8 anyway, so at some point we're probably losing the test coverage for it (unless we explicitly enable incremental linking, but that's probably not necessary).

@huangqinjin
Copy link
Contributor Author

mt.exe is always executed when cmake try to link final binary (incremental in cmake term).
https://gitlab.kitware.com/cmake/cmake/-/blob/0b552eb877b887638e8130bb6c982106a76827d8/Source/cmcmd.cxx#L2395

Testing a dummy cmake project on Windows with Ninja, we can see the output for the first time generating the binary by setting env var VERBOSE to 1.

Visual Studio Incremental Link with embedded manifests
Create test\CMakeFiles\naive_echo.dir/manifest.rc
Create empty: test\CMakeFiles\naive_echo.dir/embed.manifest
RC Pass 1:
C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe /fo test\CMakeFiles\naive_echo.dir/manifest.res test\CMakeFiles\naive_echo.dir/manifest.rc
LINK Pass 1:
C:\PROGRA~1\MIB055~1\2022\ENTERP~1\VC\Tools\MSVC\1435~1.322\bin\Hostx64\x64\link.exe /nologo test\CMakeFiles\naive_echo.dir\echo.c.obj /out:bin\naive_echo.exe /implib:test\naive_echo.lib /pdb:bin\naive_echo.pdb /version:0.0 /machine:x64 /debug /INCREMENTAL /subsystem:console kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:test\CMakeFiles\naive_echo.dir/intermediate.manifest test\CMakeFiles\naive_echo.dir/manifest.res
MT:
C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe /nologo /manifest test\CMakeFiles\naive_echo.dir/intermediate.manifest /out:test\CMakeFiles\naive_echo.dir/embed.manifest /notify_update
RC Pass 2:
C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe /fo test\CMakeFiles\naive_echo.dir/manifest.res test\CMakeFiles\naive_echo.dir/manifest.rc
FINAL LINK:
C:\PROGRA~1\MIB055~1\2022\ENTERP~1\VC\Tools\MSVC\1435~1.322\bin\Hostx64\x64\link.exe /nologo test\CMakeFiles\naive_echo.dir\echo.c.obj /out:bin\naive_echo.exe /implib:test\naive_echo.lib /pdb:bin\naive_echo.pdb /version:0.0 /machine:x64 /debug /INCREMENTAL /subsystem:console kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:test\CMakeFiles\naive_echo.dir/intermediate.manifest test\CMakeFiles\naive_echo.dir/manifest.res

LINK Pass 1: embeds the empty embed.manifest into binary, and generates intermediate.manifest with content

<?xml version='1.0' encoding='UTF-8' standalone='yes'?>
<assembly xmlns='urn:schemas-microsoft-com:asm.v1' manifestVersion='1.0'>
  <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
    <security>
      <requestedPrivileges>
        <requestedExecutionLevel level='asInvoker' uiAccess='false' />
      </requestedPrivileges>
    </security>
  </trustInfo>
</assembly>

MT: merges input manifests, intermediate.manifest in this case, and updates embed.manifest. Since output udpated, mt.exe exits with 0x41020001.

FINAL LINK: embeds the udpated embed.manifest into binary.

Using linux utility strings, we can find the following contents in the final binary:

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
  <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
    <security>
      <requestedPrivileges>
        <requestedExecutionLevel level="asInvoker" uiAccess="false"></requestedExecutionLevel>
      </requestedPrivileges>
    </security>
  </trustInfo>
</assembly>

@huangqinjin huangqinjin changed the title Strip out /notify_update in mt wrapper Handle /notify_update in mt wrapper Apr 14, 2023
@huangqinjin
Copy link
Contributor Author

huangqinjin commented Apr 14, 2023

If we silently ignore /notify_update, cmake does not execute RC Pass 2: and FINAL LINK:. So the final binary does not contain the manifest contents.

I reworked the PR to return 0xbb in case /notify_update is present. Such that cmake can continue to execute RC Pass 2: and FINAL LINK:. Though sometimes these two steps are unneeded, but correctness first.

I think we can add a CI test for this case. Checking requestedExecutionLevel literal in the final binary is enough.

@mstorsjo
Copy link
Owner

If we silently ignore /notify_update, cmake does not execute RC Pass 2: and FINAL LINK:. So the final binary does not contain the manifest contents.

Oh, I see - so in builds with the patched cmake, it has actually never merged the manifest in the end?

I reworked the PR to return 0xbb in case /notify_update is present. Such that cmake can continue to execute RC Pass 2: and FINAL LINK:. Though sometimes these two steps are unneeded, but correctness first.

Interesting; good that this can be done with a return code within 0-255 and not requiring a full 32 bit windows style return code too.

I think we can add a CI test for this case. Checking requestedExecutionLevel literal in the final binary is enough.

Ok, good. FWIW, for CI I don't think we strictly need to check that the manifest actually is included - I would settle for the level of smoke testing to see that building doesn't fail - we'd just need to build a test project that actually trigger including a manifest (do all cmake builds include that)?

@huangqinjin
Copy link
Contributor Author

huangqinjin commented Apr 15, 2023

in builds with the patched cmake, it has actually never merged the manifest in the end?

No, merging is always done, but the output embed.manifest (may be updated!) is not embeded into binary this time linking. And when next time linking, embed.manifest is not deleted, and cmake directly embeds this file (already updated due to last LINK Pass 1: and MT:!).
https://gitlab.kitware.com/cmake/cmake/-/blob/0991023c30ed5b83bcb1446b5bcc9c1eae028835/Source/cmcmd.cxx#L2441

That means if we don't handle /notify_update, cmake always embeds an old version manifest (empty for the first time linking).

Worse, if there is a user provided manifest, and it is modified, cmake will re-link, but the changes is not included into final binary. (cmake supports manifests as source files, simply use add_executable(exe main.cpp a.manifest)).

we'd just need to build a test project that actually trigger including a manifest (do all cmake builds include that)?

Yes, incremental link (default for debug build) always invokes mt.exe.

@huangqinjin huangqinjin marked this pull request as draft April 15, 2023 04:23
@huangqinjin huangqinjin force-pushed the cmake-mt branch 2 times, most recently from 8497edf to b562527 Compare April 15, 2023 05:31
@huangqinjin
Copy link
Contributor Author

After further testing, /notify_update is supported but the exit code 0x41020001 is truncated to 1. So I added a batch file to translate 0x41020001 to 0xbb.

Testing:

> rm -f embed.manifest
> /opt/msvc/bin/x64/mt /nologo /manifest wmain.manifest /out:embed.manifest /notify_update
> echo $?
187
> /opt/msvc/bin/x64/mt /nologo /manifest wmain.manifest /out:embed.manifest /notify_update
> echo $?
0

wmain.manifest.

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly manifestVersion="1.0" xmlns="urn:schemas-microsoft-com:asm.v1">
  <application>
    <windowsSettings>
      <!-- https://docs.microsoft.com/en-us/windows/uwp/design/globalizing/use-utf8-code-page -->
      <activeCodePage xmlns="http://schemas.microsoft.com/SMI/2019/WindowsSettings">UTF-8</activeCodePage>
    </windowsSettings>
  </application>
</assembly>

@huangqinjin huangqinjin marked this pull request as ready for review April 22, 2023 13:13
@huangqinjin huangqinjin marked this pull request as draft April 29, 2023 12:31
@huangqinjin
Copy link
Contributor Author

mt.bat should be merged into wine-msvc.bat introduced in PR #65 .

@mstorsjo mstorsjo mentioned this pull request Apr 29, 2023
@huangqinjin huangqinjin marked this pull request as ready for review May 8, 2023 08:03
@mstorsjo
Copy link
Owner

mstorsjo commented May 8, 2023

LGTM, thanks!

@mstorsjo mstorsjo merged commit cd0fab2 into mstorsjo:master May 8, 2023
@huangqinjin huangqinjin deleted the cmake-mt branch May 8, 2023 09:58
@huangqinjin huangqinjin mentioned this pull request Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mt.exe issue with CMake
2 participants