Add support for the clang compiler on Windows (v18) #138

Tractorou24 · 2024-08-10T08:04:16Z

Describe the pull request
This adds support for compiling the engine using the latest clang compiler (v18).
This is a good step for more adoption, since not all projects only use msvc.
It will also help porting to linux, since clang is stricter and more standard compliant, which will ease the compilation with GCC.
I was not able to test CI changes, so you may need to run it on the branch, and I will fix it without any problem!

Related issues
There are currently none, but I can create one if needed.

A word for you
First, I would like to thank you for this awesome project. It's just the best rendering engine I could find using modern C++!
I made some changes here I'm not a fan with (removing some constexprs). As much as I love those things, they are just not possible, and msvc just allows it wrongfully.
Thank you also for the time you take to read this, and review this if you are interested in it!

crud89 · 2024-08-13T12:43:29Z

Thank's a lot for the PR and the kind words! 🙂 Unfortunately I am a little bit busy at the moment, but I will go through the changes as soon as I find time for it.

With regards to Linux, I have a branch for WSL-support. Unfortunately I could not get GCC to work with stacktrace yet, which I (maybe overhasty) added to the exceptions. Also WSL has this annoying bug, where it does not work if the filesystem is split over multiple physical drives, which is my setup on both systems I have access to. So I put this aside for now with the intention of coming back to it, after GCC fully supports stacktrace.

crud89 · 2024-08-13T15:01:01Z

I've tested compiling this on my side for now and it works fine. I took the liberty to fix the target triplets and remove an obsolete warning from the build script. x64 builds run through successfully for me. Interestingly, it appears that issue #136 is resolved by this PR, though I am not sure if it's due to the drive-by changes or if it hints to some invalid code generation on the MSVC side yet.

However, I couldn't get x86 builds to compile, probably due to my lack of knowledge (I haven't touched clang much before). I have built everything from console, so vcvars must be setup properly (even when building with MSVC). However after this I am no longer able to configure the project. I will give this another try later. In the meantime, could you maybe see if you get it working? Is there anything we need to document with regards to cross-compiling using clang? And does this also affect CI?

Tractorou24 · 2024-08-13T15:15:59Z

No problem for the delay! We all have other things to do!
I will try the x86 builds as soon as possible, I don't remember manually testing them, thinking the CI would do the job... And I was wondering why you even support an outdated cpu architecture?
I think we should test clang builds on CI like MSVC, since one can build but not the other.

Regarding Linux, it seems gcc14 added full support for std::stacktrace
Il you push your branch as is, I can take a look at it (I don't promise anything about the delay)...

Tractorou24 · 2024-08-13T16:00:43Z

I found out why it was not building in x86. 2 main reasons so far:

size_t type is not the same as 0ull in 32bit mode, which leads to an ambiguous ctor
the implementation of the msvc generator (copied by hand) produces a sort of internal compiler error in 32bit.

The error looks like this:

E:\Dev\LiteFX\src\EXEC : fatal error : error in backend: failed to perform tail call elimination on a call site marked musttail
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.	Program arguments: C:\\PROGRA~1\\LLVM\\bin\\CLANG_~1.EXE -DLiteFX_Backends_Vulkan_EXPORTS -DSPDLOG_COMPILED_LIB -DSPDLOG_SHARED_LIB -DSPDLOG_USE_STD_FORMAT -IE:/Dev/LiteFX/src/Backends/Vulkan/src -IE:/Dev/LiteFX/src/Backends/Vulkan/include -IE:/Dev/LiteFX/src/Core/include -IE:/Dev/LiteFX/out/build/windows-clang-x86-debug/Core/include -IE:/Dev/LiteFX/src/Math/include -IE:/Dev/LiteFX/src/Rendering/include -IE:/Dev/LiteFX/src/Graphics/include -IE:/Dev/LiteFX/src/Logging/include -IE:/Dev/LiteFX/src/AppModel/include -IE:/Dev/LiteFX/src/Backends/DirectX12/include -isystem E:/Dev/LiteFX/out/build/windows-clang-x86-debug/vcpkg_installed/x86-windows/include -isystem E:/Dev/LiteFX/out/build/windows-clang-x86-debug/vcpkg_installed/x86-windows/include/directxmath -isystem E:/Dev/LiteFX/out/build/windows-clang-x86-debug/vcpkg_installed/x86-windows/include/vma -isystem E:/Dev/LiteFX/out/build/windows-clang-x86-debug/vcpkg_installed/x86-windows/include/spirv-reflect -isystem E:/Dev/LiteFX/out/build/windows-clang-x86-debug/vcpkg_installed/x86-windows/include/directx-dxc --target=i686-pc-windows-msvc -fdiagnostics-absolute-paths -O0 -g -Xclang -gcodeview -std=gnu++23 -D_DEBUG -D_DLL -D_MT -Xclang --dependent-lib=msvcrtd -MD -MT Backends/Vulkan/CMakeFiles/LiteFX.Backends.Vulkan.dir/src/queue.cpp.obj -MF Backends\\Vulkan\\CMakeFiles\\LiteFX.Backends.Vulkan.dir\\src\\queue.cpp.obj.d -o Backends/Vulkan/CMakeFiles/LiteFX.Backends.Vulkan.dir/src/queue.cpp.obj -c E:/Dev/LiteFX/src/Backends/Vulkan/src/queue.cpp
  1.	<eof> parser at end of file
  2.	Code generation
  3.	Running pass 'Function Pass Manager' on module 'E:/Dev/LiteFX/src/Backends/Vulkan/src/queue.cpp'.
  4.	Running pass 'X86 DAG->DAG Instruction Selection' on function '@"??R<lambda_4>@?0??submit@VulkanQueue@Backends@Rendering@LiteFX@@UBE_KABV?$Enumerable@V?$shared_ptr@$$CBVVulkanCommandBuffer@Backends@Rendering@LiteFX@@@std@@@5@@Z@QBE@XZ.resume"'
  Exception Code: 0xE0000046
   #0 0x00007fff7ad5f39c (C:\WINDOWS\System32\KERNELBASE.dll+0x5f39c)
   #1 0x00007ff612267efa (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x26c7efa)
   #2 0x00007ff612268059 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x26c8059)
   #3 0x00007ff612173473 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x25d3473)
   #4 0x00007ff612279209 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x26d9209)
   #5 0x00007ff6120dc904 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x253c904)
   #6 0x00007ff611eecf58 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x234cf58)
   #7 0x00007ff611d41342 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x21a1342)
   #8 0x00007ff611d319bb (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x21919bb)
   #9 0x00007ff611d30a5b (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x2190a5b)
  #10 0x00007ff611c82335 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x20e2335)
  #11 0x00007ff611c71f0f (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x20d1f0f)
  #12 0x00007ff611c70a2b (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x20d0a2b)
  #13 0x00007ff611db40c6 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x22140c6)
  #14 0x00007ff60ff5b6b9 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x3bb6b9)
  #15 0x00007ff611c4472e (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x20a472e)
  #16 0x00007ff611c65533 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x20c5533)
  #17 0x00007ff60fee6111 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x346111)
  #18 0x00007ff60fee5bdd (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x345bdd)
  #19 0x00007ff60fedfa69 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x33fa69)
  #20 0x00007ff611c60365 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x20c0365)
  #21 0x00007ff61201f1a8 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x247f1a8)
  #22 0x00007ff6101517e1 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x5b17e1)
  #23 0x00007ff610151282 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x5b1282)
  #24 0x00007ff61014dfb4 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x5adfb4)
  #25 0x00007ff61014bb30 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x5abb30)
  #26 0x00007ff61014a4fe (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x5aa4fe)
  #27 0x00007ff610f5e4cd (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x13be4cd)
  #28 0x00007ff6102dbc05 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x73bc05)
  #29 0x00007ff6102dba08 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x73ba08)
  #30 0x00007ff60ff8f6e8 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x3ef6e8)
  #31 0x00007ff60ff8f09e (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x3ef09e)
  #32 0x00007ff60ff8ebb7 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x3eebb7)
  #33 0x00007ff60ff88a47 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x3e8a47)
  #34 0x00007ff60ff860c3 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x3e60c3)
  #35 0x00007ff6120a4b70 (C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE+0x2504b70)
  #36 0x00007fff7bcb257d (C:\WINDOWS\System32\KERNEL32.DLL+0x1257d)
  #37 0x00007fff7d6caf28 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x5af28)
E:\Dev\LiteFX\src\clang++ : error : clang frontend command failed with exit code 70 (use -v to see invocation)
  clang version 18.1.8
  Target: i686-pc-windows-msvc
  Thread model: posix
  InstalledDir: C:\PROGRA~1\LLVM\bin
  clang++: note: diagnostic msg: 
  ********************
  
  PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
  
  Preprocessed source(s) and associated run script(s) are located at:
  clang++: note: diagnostic msg: C:\Users\DIRECT~1\AppData\Local\Temp\queue-95abd4.cpp
  clang++: note: diagnostic msg: C:\Users\DIRECT~1\AppData\Local\Temp\queue-95abd4.sh

So, here are the solutions I think about:

Fix the implementation of the generator (may be really hard)
Remove usage of generators (sad)
Not support 32bit for clang

I'm more and more curious why you are supporting 32bit despite all bugs and extra work it can cause, for not much benefit I think.
Your commit fixing the triplets is now merged in the one adding the presets in the first place.

crud89 · 2024-08-14T08:52:47Z

Thanks for checking. 🙂 I can repro the same issue and I think I've encountered this before. Somehow coroutines break codegen for x86 builds. This even happened on some occasions with MSVC, but I thought I fixed all of them. Still I am not sure if this is undefined behaviour or a compiler issue. It's a bit strange that this only affects x86 release builds, though.

Anyway, I believe this will fix itself in the future, as I plan to replace Enumerable with std::generator (in #119, which is currently a mess to merge). I really hope it will be available in STL in the no so far future, so I can get back to working on this change. In the meantime I am fine with having broken x86 support when it comes to clang, however we should document it in a follow-up issue (btw, size_t get's its own literal uz, which we can use to fix the overload issues. Meanwhile an explicit cast will do, I guess). Maybe we can drop support in the future, but I am undecided on this. I don't have any reasons not to support it yet, so I'd still prefer to have it.

Regarding stacktrace in GCC - there's partial support. You still have to link the libc++exp lib manually, which I wasn't able to get to work (at least in WSL using Ubuntu 24). It's not high on my priority list, but it's there.

I will see if I find time to review the rest of the changes later this week to get the PR to merge. I really appreciate your efforts, thanks again! 😊

Tractorou24 · 2024-08-14T18:02:25Z

I think I will remove clang x86 presets, so it can't be broken...
I didn't know about the sz suffix, feel free to add the comment during the review!

src/AppModel/include/litefx/app.hpp

src/Backends/DirectX12/src/descriptor_set.cpp

src/Backends/Vulkan/src/device.cpp

src/Backends/Vulkan/src/factory.cpp

src/Rendering/include/litefx/rendering.hpp

src/Backends/DirectX12/include/litefx/backends/dx12.hpp

crud89

I've got some minor nitpicks, but overall this looks good to me. If you resolve the changes, I am happy to merge the PR. As discussed, please also remove the x86 clang presets (we'll create a follow-up issue after the PR is merged) and document this in the readme (e.g. clang (x64 only)). Regarding the size_t literal, you don't need to change anything just yet. It isn't supported in MSVC yet and will only be required for working x86 builds in the future. I will test the CI changes in a private repo and update them later. Thanks again! ☺

Tractorou24 · 2024-08-16T19:11:32Z

I think l will make the changes next week when I have time to ! Thanks for your review !

This fixes the following errors: - error: 'nodiscard' attribute cannot be applied to types - warning: expected a qualified name after 'typename'

This fixes the following errors: - warning: 'dllexport' attribute only applies to functions, variables, classes, and Objective-C interfaces [-Wignored-attributes]

This fixes the following errors: - error: initialization of incomplete type 'AppBuilder' - warning: 'dllexport' attribute only applies to functions, variables, classes, and Objective-C interfaces [-Wignored-attributes]

The following errors were fixed: - warning: 'dllexport' attribute only applies to functions, variables, classes, and Objective-C interfaces[-Wignored-attributes]

This fixes the following errors: - error: 'nodiscard' attribute cannot be applied to types - error: constexpr function's 3rd parameter type 'RenderTarget' is not a literal type - warning: enumeration value 'Other' not handled in switch [-Wswitch] - warning: explicitly defaulted move assignment operator is implicitly deleted [-Wdefaulted-function-deleted] - warning: 'dllexport' attribute only applies to functions, variables, classes, and Objective-C interfaces [-Wignored-attributes] The assignments operators are implictly deleted since the class has a non copy assignable or a non move assignable member. This can be a const& or an Enumerable.

Fixes the following errors: - error: expected value in expression - error: template specialization requires 'template<>' - error: too few argument sprovided to function-like macro invocation For the last error (__cpuid), see: nothings/stb#978

Problem: When compiling any backend, calls to the constructor of ArgumentOutOfRangeException fails due to an ambiguous constructor. One error message: error: call to constructor of 'ArgumentOutOfRangeException' is ambiguous 166 | throw ArgumentOutOfRangeException("buffer", 0ull, buffer->size(), offset + requiredMemory, "The buffer does not contain enough memory after offset {0} to fully contain the acceleration structure.", offset); note: candidate constructor [with T = unsigned long long, TArgs = <unsigned long long &>] 143 | explicit ArgumentOutOfRangeException(std::string_view argument, T fromIncluse, T toExclusive, T value, std::string_view format, TArgs&&... args) noexcept note: candidate constructor [with TArgs = <unsigned long long, unsigned long long, const char (&)[104], unsigned long long &>] 129 | explicit ArgumentOutOfRangeException(std::string_view argument, std::string_view format, TArgs&&... args) noexcept Solution: Use a std::pair fo the value range, it allows to desambiguous the ctors since it's now an explicit type.

The following errors were fixed: - error: virtual function 'XX' has more than one final overrider in 'XX' - error: constraints not satisfied for class template 'XX' - error: enumeration value 'Other' not handled in switch [-Wswitch] - error: alias template 'Tuple' requires template arguments; argument deduction only allowed for class templates - warning: 'EnumeratePushConstants' is deprecated: This symbol is deprecated. Details: Renamed to EnumeratePushConstantBlocks [-Wdeprecated-declarations] - warning: brace elision for designated initializer is a C99 extension [-Wc99-designator] - warning: X enumeration values not handled in switch

We declare vulkan symbols as extern everywhere but nevver define it in a compilation unit. So, the linker complains about undefined extern symbols.

- error: virtual function 'XX' has more than one final overrider in 'XX' - error: constraints not satisfied for class template 'XX' - error: alias template 'Tuple' requires template arguments; argument deduction only allowed for class templates - warning: brace elision for designated initializer is a C99 extension [-Wc99-designator] - warning: X enumeration values not handled in switch - warning: using the result of an assignment as a condition without parentheses [-Wparentheses] - warning: extra tokens at end of #endif directive [-Wextra-tokens] - warning: function previously declared with an implicit exception specification redeclared with an explicit exception specification [-Wimplicit-exception-spec-mismatch]

When compiling the any backend, we have the following error: error: member access into incomplete type 'LiteFX::Rendering::Backends::XBuilder' So, in order to fix it, we reorder builder classes so the parent builders are below the child ones.

Builder holding a pointer to a type with a non constexpr constructor cannot be constexpr.

Applies C++ core guidelines C1.28: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Rh-override

Tractorou24 · 2024-08-23T19:36:39Z

I squashed your doc commit and fixed all review comments.
If you have anything else, you can add a comment and I will take care of it!

crud89 · 2024-08-26T08:24:18Z

LGTM! I've fixed some places where indentation got too deep (probably caused by this issue). I am going to merge the PR and check if the builds are successful. If so I will include clang to build in weekly builds as well as (future) test and release builds (#129).

Thank you! 🙂

Tractorou24 added 2 commits August 11, 2024 15:07

CMakePresets.json: Rename presets to include compiler name

85fcdce

CMakePresets.json: Force msvc presets to use cl.exe

c0a708e

Tractorou24 force-pushed the windows-clang-support branch from 7702939 to e97489c Compare August 12, 2024 17:17

crud89 self-assigned this Aug 13, 2024

crud89 added the Build 🛠 Issues that involve the build process. label Aug 13, 2024

Tractorou24 force-pushed the windows-clang-support branch from 55eb38b to 9cfb138 Compare August 13, 2024 16:02