-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[mono/win] Build native runtime as Debug again #60263
[mono/win] Build native runtime as Debug again #60263
Conversation
Context: dotnet#57320 Setting `CMAKE_MSVC_RUNTIME_LIBRARY` to `MultiThreaded` in `Debug` configuration allows us to link with llvm built in `Release` config without symbols. That results in `-MT` option used for compilation and getting rid of errors like: error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MT_StaticRelease' doesn't match value 'MTd_StaticDebug' in monosgen-2.0.lib(...) Also sets `_ITERATOR_DEBUG_LEVEL` to get rid of errors like: error LNK2038: mismatch detected for '_ITERATOR_DEBUG_LEVEL': value '0' doesn't match value '2' in monosgen-2.0.lib(...) And finally removing _DEBUG define. Without removing this define the `-MT` option doesn't have an effect on the runtime selection and we still end up with `MTd_StaticDebug`. I am not sure about all implications of removing `_DEBUG` define. It works fine so far and produces executables with debug information, like `mono-aot-cross.exe` with `mono-aot-cross.pdb`. That allows us to debug bugs specific to windows, like dotnet#57141
@lateralusX Can you give this a review when you have a moment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note sure about this, tweaking the debug build to use release version of c-runtime and removing _DEBUG that includes diagnostic checks and features when running debug build. I added #33282 1.5 years ago for this specific scenario, maybe an alternative could be to get debug builds of LLVM that will be used for debug builds with runtime as proposed in that issue (that would be how I would have done it locally and how we done it in mono/mono)? If not, I think we should put this change to only be applied when building a debug runtime linking a release LLVM package and maybe put out some warning around what's been disabled.
@steveisok, @directhex, possible that we could look at #33282, it will be needed by other projects going forward as well and would be the solution for this issue as well without affecting the underlying runtime debug build. It would also open up ability to debug inside LLVM libraries, that is needed from time to time as well. |
I will try to make it apply only for builds with LLVM libs, or if it will be too complicated, make it only browser specific change. |
@steveisok @lateralusX what's the plan here? |
@directhex did some experimentation to build Windows debug LLVM that would be needed and pull that version when building a debug LLVM enabled runtime (as described here, #33282), but the LLVM debug package turned out to be to big to be upload as a nuget artifact, @directhex have some more details around that actual size of the Windows debug LLVM package and what issues he hit when trying to build and publish LLVM debug packages. @directhex, maybe you could publish your findings in this PR (or link against the work you previously did in this area)? Initially this is mainly an issue when building the cross compilers since runtime build doesn't link LLVM libraries (Windows doesn't currently support LLVM JIT, just LLVM AOT), but as soon as we would like to use LLVM JIT on Windows, we will hit this issue when building regular Mono runtime as well. Issue with mixing debug/release static libs on Windows boils down to several different things, all from linking different versions of C runtime, causing potential memory corruption and resource leaks as well as C++ ABI dependencies between debug/release builds as well as potential issues due to different compiler settings between debug/release builds and compiler versions that needs to match. This is mainly solved by isolating components into separate DLL's with clear rules around what types that can cross between DLL boundaries as well as allocation and resource ownership rules between the components or if static libs is used, make sure all static libraries linked together into same binary are build using similar build settings. In the past Mono had an option to use LLVM as a loadable component, but that option was eliminated a couple of years ago, so we only support static libs build. So that takes us back to alternatives: First alternative would be to download and use a debug build Windows version of LLVM when building Mono debug configurations using LLVM. Main issue with this approach seems to be size of produced nuget package. Second alternative would be to see if we could shrink size of debug LLVM package by excluding things that might not be needed for the actual runtime build. Compressed release build LLVM nuget package seems to be ~65 MB for SDK package, installed on disk size, ~290 MB. @directhex, what was the compressed nuget package size for debug build LLVM? Third alternative would be to do local debug build of LLVM when building debug LLVM enabled runtime. Drawback here is increased local build times and changes in current LLVM init logic. Last alternative, do some hack similar to this PR, but it will be sensitive to future changes and difference between build configurations and could cause unpredicted runtime issues. It will also reduce the usage of debug build (since part of the debug build have been disabled) compared to above options. If we are going to do it this way, we should probably base it out of release build configuration (tweak release build based on parameter when requesting a debugable LLVM enabled runtime build) and turn off optimizations and generate debug info, that way you will still be able to debug the Mono runtime code and inspect variables, callstack etc, but there won't be any need to tweak other defines that will already be inline with the release build LLVM libraries (so will be more future proof solution). We should only apply changes when we build runtime and need to link to LLVM libraries, if that is not needed, we shouldn't do any adjustments to the build config. If we will do tweaks to the build configuration, we should also add a cmake warning informing user about changes made to the build configuration due to building a "debugable" LLVM runtime build. |
Having proper debug package (or packages if we need to split it) would be great. The changes in this PR helped to fix windows specific bug and we can use them again if needed. They don't need to be applied to the main for that. |
Context: #57320
Setting
CMAKE_MSVC_RUNTIME_LIBRARY
toMultiThreaded
inDebug
configuration allows us to link with llvm built in
Release
configwithout symbols. That results in
-MT
option used for compilation andgetting rid of errors like:
Also set/define
_ITERATOR_DEBUG_LEVEL
to get rid of errors like:And finally removing _DEBUG define. Without removing this define
the
-MT
option doesn't have an effect on the runtime selection and westill end up with
MTd_StaticDebug
.Note: I am not sure about all implications of removing
_DEBUG
define. Itworks fine so far and produces executables with debug information,
like
mono-aot-cross.exe
withmono-aot-cross.pdb
. That allows usto debug bugs specific to windows, like
#57141