Confusing `CMAKE_MSVC_RUNTIME_LIBRARY` checks

[es3n1n]

Hey!

I was working on a project that is being built by MSVC and it has a few more dependencies along with unicorn. One of the dependencies explicitly sets its MSVC_RUNTIME_LIBRARY to MT(d), and because of that, I need to set unicorn's runtime library type to the static one too. However, to my surprise, I can't just set MSVC_RUNTIME_LIBRARY because of the following checks:

https://github.com/unicorn-engine/unicorn/blob/5f5ef1546c4447fff3442682caa969ba075da329/CMakeLists.txt#L112-L124

Is there any reason why we can't set the MSVC_RUNTIME_LIBRARY by ourselves? Can't this code be changed to

if(NOT DEFINED CMAKE_MSVC_RUNTIME_LIBRARY)
    if(CMAKE_C_FLAGS MATCHES "[/-]MTd")
        set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreadedDebug")
    elseif(CMAKE_C_FLAGS MATCHES "[/-]MDd")
        set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreadedDebugDLL")
    elseif(CMAKE_C_FLAGS MATCHES "[/-]MT")
        set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded")
    elseif(CMAKE_C_FLAGS MATCHES "[/-]MD")
        set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreadedDLL")
    endif()
endif()

Since this code was added in 2912cd1e299456e71f9fc52b046d84cf1aff2144, I think the person who could explain me that would be @shuffle2.

I changed that part of the code to my suggestion and everything seemed to be working just fine, but I tested it only with my C++ project, so I might be missing something crucial for Rust there. Unfortunately, I don't have any Rust toolchains to check this.

[es3n1n]

If my proposed changes indeed still working with rust, then I would be happy to open a PR, but please let me know what was the reason for that behaviour in the first place, as it doesn't make much sense to me.

[recoil24]

It`s made for cmake lovers :) Normal people use .sln

[shuffle2]

To summarize the below, it's like that because when building for rust, the unicorn build is driven by rust crates which are commonly used to build non-rust dependencies (the cc and cmake crates). They do not use CMAKE_MSVC_RUNTIME_LIBRARY. Supporting multiple methods of controlling msvc runtime setting would just lead to confusing spaghetti in unicorn's cmake.

---

To build the rust library for unicorn, do cargo build --release from top level directory. You'll find something like this in target\release\build\unicorn-engine-93d2c980d1e8c9f7\out\build\CMakeCache.txt:

...
//C compiler
CMAKE_C_COMPILER:STRING=C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.40.33807/bin/HostX64/x64/cl.exe

//Flags used by the C compiler during all build types.
CMAKE_C_FLAGS:STRING= -nologo -MD -Brepro

//Flags used by the C compiler during DEBUG builds.
CMAKE_C_FLAGS_DEBUG:STRING=/Zi /Ob0 /Od /RTC1

//Flags used by the C compiler during MINSIZEREL builds.
CMAKE_C_FLAGS_MINSIZEREL:STRING=/O1 /Ob1 /DNDEBUG

//Flags used by the C compiler during RELEASE builds.
CMAKE_C_FLAGS_RELEASE:STRING=/O2 /Ob2 /DNDEBUG

//Flags used by the C compiler during RELWITHDEBINFO builds.
CMAKE_C_FLAGS_RELWITHDEBINFO:STRING=/Zi /O2 /Ob1 /DNDEBUG

//Libraries linked by default with all C applications.
CMAKE_C_STANDARD_LIBRARIES:STRING=kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib
...

The -MD flag is set here: https://github.com/rust-lang/cc-rs/blob/3ba23569a623074748a3030f382afd22483555df/src/lib.rs#L1846

Unicorn uses cc and cmake crates to facilitate building via https://github.com/unicorn-engine/unicorn/blob/master/bindings/rust/build.rs . Both of these crates wind up manipulating CMAKE_C_FLAGS in order to control msvc runtime link mode. They do not use CMAKE_MSVC_RUNTIME_LIBRARY. This matters because it is the rust build driving the unicorn build (not the reverse) - so just manually passing CMAKE_MSVC_RUNTIME_LIBRARY via the cmake crate would cause settings to go out of sync.

The problem for unicorn arises because both CMAKE_MSVC_RUNTIME_LIBRARY and CMAKE_C_FLAGS can be used to control msvc runtime link mode. Instead of trying to detect conflicting settings, we just require use of the raw C_FLAGS to be compatible with any external user (e.g. rust).

In general, it is not recommended to link against static msvc runtime. It is much worse to hardcode the setting, like your dependency is doing. I'd rather suggest you modify the other dependency to use DLL or at least settable at compile time.

[shuffle2]

Having thought about it more, if you want to implement the consistency check (to see if CMAKE_MSVC_RUNTIME_LIBRARY and CMAKE_C_FLAGS conflict), then set CMAKE_MSVC_RUNTIME_LIBRARY if not already set - I think that would be ok.

fwiw, you really don't need to change unicorn to get your dependent project to work - just set CMAKE_C_FLAGS for unicorn from your project instead of CMAKE_MSVC_RUNTIME_LIBRARY

[es3n1n]

Thanks for the explanation, it does make much more sense now.

In general, it is not recommended to link against static msvc runtime. It is much worse to hardcode the setting, like your dependency is doing. I'd rather suggest you modify the other dependency to use DLL or at least settable at compile time.

I agree, and I already notified the maintainer, but I still wanted to ask about that check since it looked weird.

Having thought about it more, if you want to implement the consistency check (to see if CMAKE_MSVC_RUNTIME_LIBRARY and CMAKE_C_FLAGS conflict), then set CMAKE_MSVC_RUNTIME_LIBRARY if not already set - I think that would be ok.

Yeah, I wanted it to throw an error if CMAKE_MSVC_RUNTIME_LIBRARY and CMAKE_C_FLAGS are conflicting. If this indeed does sound good, then I would be happy to implement that myself.

fwiw, you really don't need to change unicorn to get your dependent project to work - just set CMAKE_C_FLAGS for unicorn from your project instead of CMAKE_MSVC_RUNTIME_LIBRARY

Thanks, I am aware of that.

[wtdcode]

Yeah, I wanted it to throw an error if CMAKE_MSVC_RUNTIME_LIBRARY and CMAKE_C_FLAGS are conflicting. If this indeed does sound good, then I would be happy to implement that myself.

This option sounds good to me.

[wtdcode]

Close as PR gets through.