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

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

Merged
merged 2 commits into from Sep 27, 2022
Merged

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

merged 2 commits into from Sep 27, 2022

Conversation

relapids
Copy link
Contributor

@relapids relapids commented Aug 16, 2022

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.

@wtdcode
Copy link
Member

wtdcode commented Sep 25, 2022

I love the changes here. We once adopted a few PR on clang-cl and we are happy to support more compilers & platforms. However, for clang-cl, looks like we really need a CI (just a note, not for this PR).

@wtdcode wtdcode merged commit 5e06051 into unicorn-engine:dev Sep 27, 2022
@wtdcode
Copy link
Member

wtdcode commented Sep 27, 2022

Thanks!

@relapids relapids deleted the clang_cl_support branch September 30, 2022 22:58
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.

None yet

2 participants