Enable -flto and -fsanitize=cfi in clang

Note that this flagged up a few issues, for which followup tickets were created: * oneapi-src#2323 * oneapi-src#2309 * oneapi-src#2324
RossBrunton · Nov 13, 2024 · 15aaebe · 15aaebe
1 parent 000ca0f
commit 15aaebe
Show file tree

Hide file tree

Showing 8 changed files with 64 additions and 20 deletions.
diff --git a/.github/workflows/build-fuzz-reusable.yml b/.github/workflows/build-fuzz-reusable.yml
@@ -47,6 +47,8 @@ jobs:
         cmake --build build -j $(nproc)
 
     - name: Configure CMake
+      # CFI sanitization (or flto?) seems to cause linking to fail
+      # https://github.com/oneapi-src/unified-runtime/issues/2323
       run: >
         cmake
         -B${{github.workspace}}/build
@@ -58,6 +60,7 @@ jobs:
         -DUR_USE_ASAN=ON
         -DUR_USE_UBSAN=ON
         -DUR_BUILD_ADAPTER_L0=ON
+        -DCXX_HAS_CFI_SANITIZE=OFF
         -DUR_LEVEL_ZERO_LOADER_LIBRARY=${{github.workspace}}/level-zero/build/lib/libze_loader.so
         -DUR_LEVEL_ZERO_INCLUDE_DIR=${{github.workspace}}/level-zero/include/
         -DUR_DPCXX=${{github.workspace}}/dpcpp_compiler/bin/clang++

diff --git a/.github/workflows/build-hw-reusable.yml b/.github/workflows/build-hw-reusable.yml
@@ -82,6 +82,8 @@ jobs:
         tar -xvf ${{github.workspace}}/dpcpp_compiler.tar.gz -C dpcpp_compiler
 
     - name: Configure CMake
+      # CFI sanitization seems to fail on our CUDA nodes
+      # https://github.com/oneapi-src/unified-runtime/issues/2309
       run: >
         cmake
         -B${{github.workspace}}/build
@@ -94,6 +96,7 @@ jobs:
         -DUR_BUILD_ADAPTER_${{matrix.adapter.name}}=ON
         -DUR_CONFORMANCE_TEST_LOADER=${{ matrix.adapter.other_name != '' && 'ON' || 'OFF' }}
         ${{ matrix.adapter.other_name != '' && format('-DUR_BUILD_ADAPTER_{0}=ON', matrix.adapter.other_name) || '' }}
+        ${{ matrix.adapter.name == 'CUDA' && '-DCXX_HAS_CFI_SANITIZE=OFF' || '' }}
         -DUR_STATIC_LOADER=${{matrix.adapter.static_Loader}}
         -DUR_STATIC_ADAPTER_${{matrix.adapter.name}}=${{matrix.adapter.static_adapter}}
         -DUR_DPCXX=${{github.workspace}}/dpcpp_compiler/bin/clang++

diff --git a/cmake/helpers.cmake b/cmake/helpers.cmake
@@ -63,6 +63,11 @@ if(CMAKE_SYSTEM_NAME STREQUAL Linux)
     check_cxx_compiler_flag("-fstack-clash-protection" CXX_HAS_FSTACK_CLASH_PROTECTION)
 endif()
 
+set(SAVED_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
+set(CMAKE_REQUIRED_FLAGS "-flto -fvisibility=hidden")
+check_cxx_compiler_flag("-fsanitize=cfi" CXX_HAS_CFI_SANITIZE)
+set(CMAKE_REQUIRED_FLAGS ${SAVED_CMAKE_REQUIRED_FLAGS})
+
 function(add_ur_target_compile_options name)
     if(NOT MSVC)
         target_compile_definitions(${name} PRIVATE -D_FORTIFY_SOURCE=2)
@@ -78,11 +83,10 @@ function(add_ur_target_compile_options name)
             # Hardening options
             -fPIC
             -fstack-protector-strong
-            -fvisibility=hidden # Required for -fsanitize=cfi
-            # -fsanitize=cfi requires -flto, which breaks a lot of things
-            # See: https://github.com/oneapi-src/unified-runtime/issues/2120
-            # -flto
-            # $<$<CXX_COMPILER_ID:Clang,AppleClang>:-fsanitize=cfi>
+            -fvisibility=hidden
+            # cfi-icall requires called functions in shared libraries to also be built with cfi-icall, which we can't
+            # guarantee. -fsanitize=cfi depends on -flto
+            $<$<BOOL:${CXX_HAS_CFI_SANITIZE}>:-flto -fsanitize=cfi -fno-sanitize=cfi-icall>
             $<$<BOOL:${CXX_HAS_FCF_PROTECTION_FULL}>:-fcf-protection=full>
             $<$<BOOL:${CXX_HAS_FSTACK_CLASH_PROTECTION}>:-fstack-clash-protection>
 
@@ -119,7 +123,10 @@ endfunction()
 function(add_ur_target_link_options name)
     if(NOT MSVC)
         if (NOT APPLE)
-            target_link_options(${name} PRIVATE "LINKER:-z,relro,-z,now,-z,noexecstack")
+            target_link_options(${name} PRIVATE
+                $<$<BOOL:${CXX_HAS_CFI_SANITIZE}>:-flto -fsanitize=cfi -fno-sanitize=cfi-icall>
+                "LINKER:-z,relro,-z,now,-z,noexecstack"
+            )
             if (UR_DEVELOPER_MODE)
                 target_link_options(${name} PRIVATE -Werror -Wextra)
             endif()

diff --git a/test/adapters/level_zero/v2/CMakeLists.txt b/test/adapters/level_zero/v2/CMakeLists.txt
@@ -33,19 +33,24 @@ add_unittest(level_zero_command_list_cache
         ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/command_list_cache.cpp
 )
 
-add_unittest(level_zero_event_pool
-        event_pool_test.cpp
-        ${PROJECT_SOURCE_DIR}/source/ur/ur.cpp
-        ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/adapter.cpp
-        ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/device.cpp
-        ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/platform.cpp
-        ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event_pool.cpp
-        ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event_pool_cache.cpp
-        ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event_provider_normal.cpp
-        ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event_provider_counter.cpp
-        ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event.cpp
-        ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/queue_api.cpp
-)
+if(CXX_HAS_CFI_SANITIZE)
+    message(WARNING "Level Zero V2 Event Pool tests are disabled when using CFI sanitizer")
+    message(NOTE "See https://github.com/oneapi-src/unified-runtime/issues/2324")
+else()
+    add_unittest(level_zero_event_pool
+            event_pool_test.cpp
+            ${PROJECT_SOURCE_DIR}/source/ur/ur.cpp
+            ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/adapter.cpp
+            ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/device.cpp
+            ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/platform.cpp
+            ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event_pool.cpp
+            ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event_pool_cache.cpp
+            ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event_provider_normal.cpp
+            ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event_provider_counter.cpp
+            ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/event.cpp
+            ${PROJECT_SOURCE_DIR}/source/adapters/level_zero/v2/queue_api.cpp
+    )
+endif()
 
 add_adapter_test(level_zero_memory_residency
     FIXTURE DEVICES

diff --git a/test/conformance/enqueue/enqueue_adapter_opencl.match b/test/conformance/enqueue/enqueue_adapter_opencl.match
@@ -0,0 +1,3 @@
+# Note: This file is only for use with cts_exe.py
+# Fails when -fsanitize=cfi
+{{OPT}}urEnqueueEventsWaitMultiDeviceMTTest.EnqueueWaitOnAllQueues/MultiThread
diff --git a/test/conformance/exp_command_buffer/exp_command_buffer_adapter_cuda.match b/test/conformance/exp_command_buffer/exp_command_buffer_adapter_cuda.match
@@ -0,0 +1,11 @@
+# Note: This file is only for use with cts_exe.py
+# These cause SIGILL when built with -fsanitize=cfi on Nvidia
+{{OPT}}urCommandBufferKernelHandleUpdateTest.Success/*
+{{OPT}}urCommandBufferKernelHandleUpdateTest.UpdateAgain/*
+{{OPT}}urCommandBufferKernelHandleUpdateTest.RestoreOriginalKernel/*
+{{OPT}}urCommandBufferKernelHandleUpdateTest.KernelAlternativeNotRegistered/*
+{{OPT}}urCommandBufferKernelHandleUpdateTest.RegisterInvalidKernelAlternative/*
+{{OPT}}urCommandBufferValidUpdateParametersTest.UpdateDimensionsWithoutUpdatingKernel/*
+{{OPT}}urCommandBufferValidUpdateParametersTest.UpdateOnlyLocalWorkSize/*
+{{OPT}}urCommandBufferValidUpdateParametersTest.SuccessNullptrHandle/*
+{{OPT}}KernelCommandEventSyncUpdateTest.TwoWaitEvents/*
diff --git a/test/conformance/exp_command_buffer/exp_command_buffer_adapter_hip.match b/test/conformance/exp_command_buffer/exp_command_buffer_adapter_hip.match
@@ -0,0 +1,10 @@
+# Note: This file is only for use with cts_exe.py
+# These cause SIGILL when built with -fsanitize=cfi on AMD
+{{OPT}}urCommandBufferKernelHandleUpdateTest.Success/*
+{{OPT}}urCommandBufferKernelHandleUpdateTest.UpdateAgain/*
+{{OPT}}urCommandBufferKernelHandleUpdateTest.RestoreOriginalKernel/*
+{{OPT}}urCommandBufferKernelHandleUpdateTest.KernelAlternativeNotRegistered/*
+{{OPT}}urCommandBufferKernelHandleUpdateTest.RegisterInvalidKernelAlternative/*
+{{OPT}}urCommandBufferValidUpdateParametersTest.UpdateDimensionsWithoutUpdatingKernel/*
+{{OPT}}urCommandBufferValidUpdateParametersTest.UpdateOnlyLocalWorkSize/*
+{{OPT}}urCommandBufferValidUpdateParametersTest.SuccessNullptrHandle/*
diff --git a/test/fuzz/CMakeLists.txt b/test/fuzz/CMakeLists.txt
@@ -51,7 +51,9 @@ target_link_libraries(fuzztest-base
     ${PROJECT_NAME}::headers
     ${PROJECT_NAME}::common
     -fsanitize=fuzzer)
-target_compile_options(fuzztest-base PRIVATE -g -fsanitize=fuzzer)
+# When built with -g and -flto (which is required by some hardening flags), this causes a segfault in (upstream)
+# LLVM 14-15 while linking when CMAKE_BUILD_TYPE is Release
+target_compile_options(fuzztest-base PRIVATE -fsanitize=fuzzer)
 target_compile_definitions(fuzztest-base PRIVATE -DKERNEL_IL_PATH="${UR_CONFORMANCE_DEVICE_BINARIES_DIR}/fill/spir64.bin.0")
 target_include_directories(fuzztest-base PRIVATE ${UR_CONFORMANCE_DEVICE_BINARIES_DIR})
 add_dependencies(fuzztest-base generate_device_binaries)