build: Add ClangCL profiles #791

alichraghi · 2024-11-21T14:13:21Z

Also fix some compilation errors catched by clang

Description

Testing

TODO list:

Also fix some compilation errors catched by clang Signed-off-by: Ali Cheraghi <alichraghi@proton.me>

3rdparty/CMakeLists.txt

3rdparty/dxc/CMakeLists.txt

CMakeLists.txt

cmake/adjust/template/vendor/CXX_MSVC.cmake

cmake/common.cmake

src/nbl/builtin/utils.cmake

CMakeLists.txt

Signed-off-by: Ali Cheraghi <alichraghi@proton.me>

cmake/adjust/flags.cmake

Signed-off-by: Ali Cheraghi <alichraghi@proton.me>

include/nbl/builtin/hlsl/cpp_compat/impl/intrinsics_impl.hlsl

devshgraphicsprogramming · 2025-04-08T14:15:26Z

3rdparty/CMakeLists.txt

-		else()
-			set_property(TARGET ${trgt} PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
-		endif()
+		set_property(TARGET ${trgt} PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>$<$<BOOL:${NBL_COMPILER_DYNAMIC_RUNTIME}>:DLL>")


AFAIK in Clang and GCC there are options about static linking of libstd-c++

yes there are and this property has effect only if MSVC ABI is the target

devshgraphicsprogramming · 2025-04-08T14:16:35Z

3rdparty/dxc/CMakeLists.txt

-else()
-	list(APPEND NBL_DXC_CMAKE_OPTIONS "-DCMAKE_MSVC_RUNTIME_LIBRARY:STATIC=MultiThreaded$<$<CONFIG:Debug>:Debug>")		
-endif()
+list(APPEND NBL_DXC_CMAKE_OPTIONS "-DCMAKE_MSVC_RUNTIME_LIBRARY:STATIC=MultiThreaded$<$<CONFIG:Debug>:Debug>$<$<BOOL:${NBL_COMPILER_DYNAMIC_RUNTIME}>:DLL>")


same as with the general nabla comment

#791 (comment)

devshgraphicsprogramming · 2025-04-08T14:17:45Z

CMakeLists.txt

+	if(NOT NBL_COMPILER_DYNAMIC_RUNTIME)
+		message(FATAL_ERROR "Turn NBL_COMPILER_DYNAMIC_RUNTIME on! For dynamic Nabla builds dynamic runtime is mandatory!")
 	endif()


aah ok so we want to ban compiling with static-libstdc++

yes we agreed it must match

devshgraphicsprogramming · 2025-04-08T14:19:20Z

cmake/adjust/template/vendor/CXX_Clang.cmake

+list(APPEND NBL_CXX_COMPILE_OPTIONS
+	-Wextra
+	-fno-strict-aliasing
+	-msse4.2


I think you might also need a flag for stripping debug symbols

mavx

resolved

devshgraphicsprogramming · 2025-04-08T14:19:41Z

cmake/adjust/template/vendor/CXX_Clang.cmake

+	-Wno-sequence-point
+	-Wno-unused-parameter
+	-Wno-unused-but-set-parameter
+	-Wno-c++98-compat
+	-Wno-c++98-compat-pedantic
+	-Wno-padded
+	-Wno-unsafe-buffer-usage
+	-Wno-switch-enum
+	-Wno-error=ignored-attributes
+	-Wno-error=unused-function
+	-Wno-error=unused-variable
+	-Wno-error=unused-parameter
+	-Wno-error=ignored-attributes
+	-Wno-error=non-pod-varargs


I take it that we'll remove this over time?

yes, @YasInvolved is going to kill all warnings

devshgraphicsprogramming · 2025-04-08T14:20:01Z

cmake/adjust/template/vendor/CXX_Clang.cmake

+	-Wno-error=unused-parameter
+	-Wno-error=ignored-attributes
+	-Wno-error=non-pod-varargs
+	-fno-exceptions


I dont think we can compile without exceptions, there are some try-catch block in our code IIRC ?

I dont think we can compile without exceptions, there are some try-catch block in our code IIRC ?

resolved, -fno-exceptions should not be there + its possible to compile with this flag but it changes runtime behaviour which we don't want (asserts)

devshgraphicsprogramming · 2025-04-08T14:20:43Z

cmake/adjust/template/vendor/CXX_Clang.cmake

+set(NBL_CXX_RELEASE_COMPILE_OPTIONS
+	-fexpensive-optimizations
+)


I think O3 enables this by default ?

yes however -fexpensive-optimizations is not Clang's option but GCC's

devshgraphicsprogramming · 2025-04-08T14:21:05Z

cmake/adjust/template/vendor/CXX_Clang.cmake

+# RelWithDebInfo
+set(NBL_CXX_RELWITHDEBINFO_COMPILE_OPTIONS "")


what about O1 or something ? Alos don't you need ggdb3 ?

what about O1 or something ? Alos don't you need ggdb3 ?

#791 (comment) + resolved

devshgraphicsprogramming · 2025-04-08T14:21:06Z

cmake/adjust/template/vendor/CXX_Clang.cmake

+
+# Debug
+set(NBL_CXX_DEBUG_COMPILE_OPTIONS
+	-ggdb3 -Wall -fno-omit-frame-pointer -fstack-protector-strong


is ggdb3 sufficient?

shouldn't -g also be used?

btw GCC/Clang have an equivalent of ffast math that we use in MSVC

actually ggdb3 doesn't exist on clang and it throws a warning about unknown arg

yeah I swear it was a GCC / Emscripten thing

also ggdb3 is for GDB, I think on Clang one is supposed to use LLDB

on windows ClangCL which is Clang frontend compatible with MSVC CLI uses lld-link linker

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\bin>lld.exe
lld is a generic driver.
Invoke ld.lld (Unix), ld64.lld (macOS), lld-link (Windows), wasm-ld (WebAssembly) instead

devshgraphicsprogramming · 2025-04-08T14:23:02Z

cmake/adjust/template/vendor/CXX_Clang.cmake

+# our pervious flags-set function called this, does not affect flags nor configs so I will keep it here temporary
+# TODO: move it out from the profile
+link_libraries(-fuse-ld=gold)


what about incremental linking and stuff?

resolved, we use incremental linking

devshgraphicsprogramming · 2025-04-08T14:23:50Z

cmake/adjust/template/vendor/CXX_Clang.cmake

+include_guard(GLOBAL)
+
+# Debug


what about macro definitions like NDEBUG ?

P.S. should MSVC even define should macros from commandline instead of configuring theminto the header?

what about macro definitions like NDEBUG ?

in theory we set them in MSVC frontend profile but we could skip doing so both for this profile & Clang since CMake sets NDEBUG at the very beginning with CMAKE_<LANG>_FLAGS_<CONFIG>_INIT within some defaults (ofc, it can be overwritten)

https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Platform/Windows-MSVC.cmake#L504

https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Platform/Windows-Clang.cmake#L117

instead of configuring theminto the header?

to be discussed but we already generate a set of defines which is included in nabla.h iirc, we could remove defines from CLI

a2a7e72 I stay at defining it myself though

devshgraphicsprogramming · 2025-04-08T14:24:48Z

cmake/adjust/template/vendor/C_Clang.cmake

+# Debug
+set(NBL_C_DEBUG_COMPILE_OPTIONS
+	-ggdb3 -Wall -fno-omit-frame-pointer -fstack-protector-strong
+)
+
+# Release
+set(NBL_C_RELEASE_COMPILE_OPTIONS
+	-fexpensive-optimizations
+)
+
+# RelWithDebInfo
+set(NBL_C_RELWITHDEBINFO_COMPILE_OPTIONS "")
+
+# Global
+list(APPEND NBL_C_COMPILE_OPTIONS
+	-Wextra
+	-fno-strict-aliasing
+	-msse4.2
+	-maes
+	-mfpmath=sse		
+	-Wextra
+	-Wno-sequence-point
+	-Wno-unused-parameter
+	-Wno-unused-but-set-parameter
+	-Wno-c++98-compat
+	-Wno-c++98-compat-pedantic
+	-Wno-padded
+	-Wno-unsafe-buffer-usage
+	-Wno-switch-enum
+	-Wno-error=ignored-attributes
+	-Wno-error=unused-function
+	-Wno-error=unused-variable
+	-Wno-error=unused-parameter
+	-Wno-error=ignored-attributes
+	-Wno-error=non-pod-varargs
+	-fno-exceptions
+)
+
+if(NBL_SANITIZE_ADDRESS)
+	list(APPEND NBL_C_COMPILE_OPTIONS -fsanitize=address)
+endif()
+
+if(NBL_SANITIZE_THREAD)
+	list(APPEND NBL_C_COMPILE_OPTIONS -fsanitize=thread)
+endif()
+
+# our pervious flags-set function called this, does not affect flags nor configs so I will keep it here temporary
+# TODO: move it out from the profile
+link_libraries(-fuse-ld=gold)


same comments as for C++, maybe unify a bit with a common cmake include ?

devshgraphicsprogramming · 2025-04-08T14:35:34Z

include/nbl/asset/IDescriptorSetLayout.h

@@ -330,7 +330,7 @@ class IDescriptorSetLayout : public IDescriptorSetLayoutBase
 				bindings[i].binding = i;
 				bindings[i].type = type;
 				bindings[i].createFlags = SBinding::E_CREATE_FLAGS::ECF_NONE;
-				bindings[i].stageFlags = stageAccessFlags ? stageAccessFlags[i]:asset::IShader::ESS_ALL_OR_LIBRARY;
+				bindings[i].stageFlags = stageAccessFlags ? stageAccessFlags[i]:asset::IShader::E_SHADER_STAGE::ESS_ALL_OR_LIBRARY;


hlsl::ShaderStage`

devshgraphicsprogramming · 2025-04-08T14:37:03Z

include/nbl/macros.h

 //! Workarounds for compiler specific bugs
 // MSVC 2019 is a special snowflake
-#if defined(_MSC_VER) && _MSC_VER>=1920
+#if defined(_MSC_VER) && !defined(__clang__) && _MSC_VER>=1920


can we just ban old MSC_VER and old GCC ? and not do weird macro tricks ( or less)

we can ban old compilers at header level, there are a few headers checking for compiler version and doing weird tricks so imo we should do it in another PR and delegate @YasInvolved

devshgraphicsprogramming · 2025-04-08T14:38:52Z

include/nbl/video/ISwapchain.h

+            SSharedCreationParams() {}
+


we'll lose designated initializers with a default ctor

resolved, though

25>D:\repositories\DevshGraphicsProgramming\Nabla\include\nbl/video/ISwapchain.h(457,90): error : default member initializer for 'imageUsage' needed within definition of enclosing class 'ISwapchain' outside of member functions

hence we cannot

inline core::smart_refctd_ptr<ISwapchain> recreate(SSharedCreationParams params={})

and I added an overload to use default initializers

inline core::smart_refctd_ptr<ISwapchain> recreate(SSharedCreationParams params) { /*body*/ } inline core::smart_refctd_ptr<ISwapchain> recreate() { return recreate({}); }

src/nbl/video/IGPUAccelerationStructure.cpp

…er new compile errors after upgrading VS and toolsets (coming from DXC source files)

… NBL_REQUEST_COMPILE_OPTION_SUPPORT, add required instruction set features for simdjson explicitly; now I hit GLI errors due to bad templates

AnastaZIuk · 2025-04-11T13:42:44Z

cmake/adjust/template/vendor/impl/Clang.cmake

+
+	# also instead of enabling single options maybe we could consider requesting an
+	# instruction implementation set instead, eg -march=haswel, though this approach
+	# could add a few more flags then we actually need while building - to rethink


@devshgraphicsprogramming

https://github.com/llvm/llvm-project/blob/b46f16c8d7f4a9e37271cdc4717dd858a4c813dc/llvm/lib/Target/X86/X86.td#L937

-march is a bad idea unless you're deploying on a particular CPU (i.e. renderfarm or amazon AWS)

-march is a bad idea unless you're deploying on a particular CPU (i.e. renderfarm or amazon AWS)

understood, also found it too much - resolved

… (to latest and our own fork not mine, this one was 6 years old) submodules

…ons which must be passed with proxy XClang arg (they were ignored before), use NBL_REQUEST_COMPILE_OPTION_SUPPORT for Clang profile hence enforce flags validation at configure time (TODO: do the same for MSVC). It still crashes at JIT loader's cpp with -1073741819 - windooze access violation, I need to attach diagnostic outputs for LLVM team

…ngstream for generated line, make it build with Clang(CL) 19.1.1

…stem + apply workaround for CLang(CL) 19.1.1 due to error : use of undeclared label "errhandler"; for some reason if in single translation unit we have identical label names (goto) in separate function bodies we hit this error

…errors with goto statements (8f454a9)

…QUEST_COMPILE_OPTION_SUPPORT & build system to correctly handle compile & link options, validate build options at configure time

…lly specify the flag

…CMAKE_<LANG>_COMPILER_FRONTEND_VARIANT, fix issues with /DELAYLOAD & debug info format for ClangCL, use MSVC-frontend checking logic, inherit default MSVC frontend options in Clang profile if using Windows' ClangCL; upgrade minimum CMake version to 3.31

…into ali_clang

…use hlsl::ShaderStage in IDescriptorSetLayout.h

…without need to specify debug format flags with ClangCL by hand, enforce ProgramDatabase regardless the case (https://gitlab.kitware.com/cmake/cmake/-/issues/26879#note_1649970)

build: Add ClangCL profiles

60e1e5b

Also fix some compilation errors catched by clang Signed-off-by: Ali Cheraghi <alichraghi@proton.me>

alichraghi requested a review from AnastaZIuk November 21, 2024 14:13

AnastaZIuk reviewed Nov 21, 2024

View reviewed changes

alichraghi added 2 commits November 21, 2024 20:21

build: one liner ifs and some fixes

114c549

Signed-off-by: Ali Cheraghi <alichraghi@proton.me>

update dxc submodule

ff5513b

Signed-off-by: Ali Cheraghi <alichraghi@proton.me>

AnastaZIuk reviewed Nov 21, 2024

View reviewed changes

cmake/adjust/flags.cmake Outdated Show resolved Hide resolved

alichraghi and others added 2 commits November 22, 2024 20:21

build: simplify if

44acfcf

Signed-off-by: Ali Cheraghi <alichraghi@proton.me>

Merge branch 'ali_clang' of github.com:Devsh-Graphics-Programming/Nabla

dbff78d

AnastaZIuk force-pushed the ali_clang branch from f44c0eb to dbff78d Compare April 8, 2025 12:38

AnastaZIuk added 2 commits April 8, 2025 14:39

fixing CLang build, save work

616f7d7

make Nabla Clang build work

aad8bb1

AnastaZIuk reviewed Apr 8, 2025

View reviewed changes

include/nbl/builtin/hlsl/cpp_compat/impl/intrinsics_impl.hlsl Outdated Show resolved Hide resolved

bad typo

7b8cb61

devshgraphicsprogramming reviewed Apr 8, 2025

View reviewed changes

src/nbl/video/IGPUAccelerationStructure.cpp Show resolved Hide resolved

update profiles & flags check handle, take care of Clang profile; ent…

062b5ba

…er new compile errors after upgrading VS and toolsets (coming from DXC source files)

update dxc submodule with fixed clang 19.1.1 build, upgrade & correct…

39bb3e1

… NBL_REQUEST_COMPILE_OPTION_SUPPORT, add required instruction set features for simdjson explicitly; now I hit GLI errors due to bad templates

AnastaZIuk reviewed Apr 11, 2025

View reviewed changes

AnastaZIuk added 15 commits April 11, 2025 18:42

update GLI (use custom location for GLM + fix with templates) and GLM…

cbb4db1

… (to latest and our own fork not mine, this one was 6 years old) submodules

Reduce device_capabilities_traits_jit.h instructions & use std::ostri…

16088b9

…ngstream for generated line, make it build with Clang(CL) 19.1.1

remove -Xclang=-fincremental-extensions which causes funny compile …

b4e722a

…errors with goto statements (8f454a9)

adjust MSVC profile + correct incremental link options, update NBL_RE…

eda05ee

…QUEST_COMPILE_OPTION_SUPPORT & build system to correctly handle compile & link options, validate build options at configure time

get rid of MSVC Release linker fallback with /LTCG due to /GL - manua…

25e0120

…lly specify the flag

Merge branch 'master' of github.com:Devsh-Graphics-Programming/Nabla …

d1bb5c6

…into ali_clang

ah DXC needs to point to devshFixes_clang_19_1_1

a1b9b99

keep designated initializers for ISwapchain's SSharedCreationParams, …

77ed416

…use hlsl::ShaderStage in IDescriptorSetLayout.h

for my sanity - add -DNDEBUG to Clang profile (Unix)

a2a7e72

correct CMAKE_MSVC_DEBUG_INFORMATION_FORMAT logic hence make it work …

cde9e79

…without need to specify debug format flags with ClangCL by hand, enforce ProgramDatabase regardless the case (https://gitlab.kitware.com/cmake/cmake/-/issues/26879#note_1649970)

pull master & resolve conflicts

8246891

add more NEW policies, leave TODO comment for cmake_policy

6e4392e

		# RelWithDebInfo
		set(NBL_CXX_RELWITHDEBINFO_COMPILE_OPTIONS "")

		include_guard(GLOBAL)

		# Debug

build: Add ClangCL profiles #791

Are you sure you want to change the base?

build: Add ClangCL profiles #791

Conversation

alichraghi commented Nov 21, 2024

Description

Testing

TODO list:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnastaZIuk Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnastaZIuk Apr 18, 2025 •

edited

Loading