unicorn icon indicating copy to clipboard operation
unicorn copied to clipboard

Allow building with clang-cl (using MSVC config) on Windows.

Open relapids opened this issue 1 year ago • 0 comments

Not sure if this is a change you're interested in or not, but figured I'd send it and let you decide. If you don't want this it's fine, it's easy enough for me to maintain my own patchset (though of course I'd prefer not to).

Build example (using latest LLVM + VS2022 targeting x64):

PS E:\Dev\unicorn\build> cmake -S ..\repo\ -B . -G Ninja -D CMAKE_C_COMPILER=clang-cl -D CMAKE_BUILD_TYPE=Debug
PS E:\Dev\unicorn\build> cmake --build .
PS E:\Dev\unicorn\build> cmake --build . --target test

There are 4 changes here.

  1. The typeof extension is only available in GNU mode, whereas __typeof__ is always available in all modes.

  2. __int128_t is already defined with Clang, and since it shares the MSVC headers (hence doesn't define CONFIG_INT128) we need to ifdef it out. Alternatively it looks like it would be possible to drop the __int128_t altogether in that code, but I wasn't sure which would be preferable. Dropping it altogether seems potentially 'safer' to me in terms of not accidentally using the 'real' __int128_t when you meant to be using Int128, but I'm not sure if the typedef was there for some reason I wasn't aware of, since it seems to have been added intentionally. However it appears unused to me (all uses of __int128_t I see are guarded by CONFIG_INT128 - e.g. the MSVC build passed locally for me with that typedef removed).

  3. For helper_pminsn the DEF_HELPER_2 macro declares the function to take uint32_t as the second arg, but it's defined to take powerpc_pm_insn_t. Not sure why it's only clang-cl which really doesn't like this. Probably it could also be done without the special-casing by just changing it unconditionally to use uint32_t in the definition (I'd need to test that works everywhere of course), but again this seemed like the less invasive change. Happy to test changing it unconditionally if you think that's better (I can test on Linux, FreeBSD, MacOS and Windows - a quick test with Clang and GCC on Linux showed that it seemed to work so if that's the preferred route I'd expect it to work elsewhere too).

  4. The change around __ATOMIC_RELAXED to exclude clang-cl was due to a segfault in tb_remove_from_jmp_list as a result of the atomic_or_fetch operation only returning a 32-bit value, and truncating the high half of the 64-bit pointer. Forcing clang-cl to use the same code-path as MSVC for atomic operations resolved the issue.

relapids avatar Aug 16 '22 02:08 relapids