Skip to content

Commit

Permalink
Remove faulty fallback for unknown functions
Browse files Browse the repository at this point in the history 
This code was only used when the required assembly code for unknown function
was missing or intentionally disabled. Because it relied on the implementation
of the compiler to optimize the code in such a way as to not disturb passed in
parameters, the fallback code could not be relied on to work. Removing the
fallback path simplifies the support matrix by making unknown functions
definitely not supported (and will return NULL when queried), whereas with the
fallback the code may work or may crash.
  • Loading branch information
@charles-lunarg
charles-lunarg committed May 16, 2024
1 parent 1e8781e commit ffb58e2
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 868 deletions.
1 change: 0 additions & 1 deletion BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ if (!is_android) {
"loader/vk_loader_layer.h",

# TODO(jmadill): Use assembler where available.
"loader/unknown_ext_chain.c",
"loader/vk_loader_platform.h",
"loader/wsi.c",
"loader/wsi.h",
Expand Down
68 changes: 21 additions & 47 deletions loader/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,6 @@ if(WIN32)
set(CMAKE_C_STANDARD_LIBRARIES " ") # space is intentional
endif()

# ~~~
# Build dev_ext_trampoline.c and unknown_ext_chain.c with /O2 to allow tail-call optimization.
# Setup two CMake targets (loader-norm and loader-opt) for the different compilation flags.
# ~~~
set(MODIFIED_C_FLAGS_DEBUG ${CMAKE_C_FLAGS_DEBUG})

string(REPLACE "/Od" "/O2" MODIFIED_C_FLAGS_DEBUG ${MODIFIED_C_FLAGS_DEBUG})
string(REPLACE "/Ob0" "/Ob2" MODIFIED_C_FLAGS_DEBUG ${MODIFIED_C_FLAGS_DEBUG})
string(REGEX REPLACE "/RTC." "" MODIFIED_C_FLAGS_DEBUG ${MODIFIED_C_FLAGS_DEBUG}) #remove run-time error checks

separate_arguments(MODIFIED_C_FLAGS_DEBUG WINDOWS_COMMAND ${MODIFIED_C_FLAGS_DEBUG})

# ~~~
# Only generate the loader.rc file with CMake if BUILD_DLL_VERSIONINFO was set.
# This feature is for the Vulkan Runtime build
Expand Down Expand Up @@ -121,15 +109,11 @@ endif()

set(OPT_LOADER_SRCS dev_ext_trampoline.c phys_dev_ext.c)

set(ASM_FAILURE_MSG "Support for unknown physical device and device functions is disabled due to missing the required assembly support code. \
To support unknown functions, assembly must be added for the platform.\n")

# Check for assembler support
set(ASM_FAILURE_MSG "The build will fall back on building with C code\n")
set(ASM_FAILURE_MSG "${ASM_FAILURE_MSG}Note that this may be unsafe, as the C code requires tail-call optimizations to remove")
set(ASM_FAILURE_MSG "${ASM_FAILURE_MSG} the stack frame for certain calls. If the compiler does not do this, then unknown device")
set(ASM_FAILURE_MSG "${ASM_FAILURE_MSG} extensions will suffer from a corrupted stack.")

if (APPLE_UNIVERSAL_BINARY)
set(USE_ASSEMBLY_FALLBACK ON)
elseif(WIN32 AND NOT USE_GAS)
if(WIN32 AND NOT USE_GAS)
option(USE_MASM "Use MASM" ON)
if(USE_MASM AND MINGW)
find_program(JWASM_FOUND NAMES jwasm uasm)
Expand Down Expand Up @@ -211,8 +195,9 @@ end
target_link_libraries(loader-unknown-chain Vulkan::Headers)
target_include_directories(loader-unknown-chain PUBLIC ${CMAKE_CURRENT_BINARY_DIR})
add_dependencies(loader-unknown-chain loader_asm_gen_files)
set(LOADER_UNKNOWN_CHAIN_LIBRARY $<TARGET_OBJECTS:loader-unknown-chain>)
set(UNKNOWN_FUNCTIONS_SUPPORTED ON)
else()
set(USE_ASSEMBLY_FALLBACK ON)
message(WARNING "Could not find working MASM assembler\n${ASM_FAILURE_MSG}")
endif()
elseif(UNIX OR MINGW OR (WIN32 AND USE_GAS)) # i.e.: Linux & Apple & MinGW & Windows using Clang-CL
Expand Down Expand Up @@ -312,8 +297,8 @@ elseif(UNIX OR MINGW OR (WIN32 AND USE_GAS)) # i.e.: Linux & Apple & MinGW & Win
if (APPLE)
set(MODIFY_UNKNOWN_FUNCTION_DECLS ON)
endif()
set(UNKNOWN_FUNCTIONS_SUPPORTED ON)
else()
set(USE_ASSEMBLY_FALLBACK ON)
if(USE_GAS)
message(WARNING "Could not find working ${ASM_OFFSET_SYSTEM_PROCESSOR} GAS assembler\n${ASM_FAILURE_MSG}")
else()
Expand All @@ -322,40 +307,25 @@ elseif(UNIX OR MINGW OR (WIN32 AND USE_GAS)) # i.e.: Linux & Apple & MinGW & Win
endif()
endif()

if(USE_ASSEMBLY_FALLBACK)
add_custom_target(loader_asm_gen_files)
if (MSVC)
add_library(loader-unknown-chain OBJECT unknown_ext_chain.c)
target_link_libraries(loader-unknown-chain loader_specific_options)
set_target_properties(loader-unknown-chain PROPERTIES CMAKE_C_FLAGS_DEBUG "${MODIFIED_C_FLAGS_DEBUG}")
else()
set(OPT_LOADER_SRCS ${OPT_LOADER_SRCS} unknown_ext_chain.c)
set_source_files_properties(${OPT_LOADER_SRCS} PROPERTIES COMPILE_FLAGS -O)
endif()
if(UNKNOWN_FUNCTIONS_SUPPORTED)
list(APPEND NORMAL_LOADER_SRCS ${OPT_LOADER_SRCS})
endif()

if(WIN32)
add_library(loader-opt STATIC ${OPT_LOADER_SRCS})
target_link_libraries(loader-opt PUBLIC loader_specific_options)
add_dependencies(loader-opt loader_asm_gen_files)
set_target_properties(loader-opt PROPERTIES CMAKE_C_FLAGS_DEBUG "${MODIFIED_C_FLAGS_DEBUG}")

# If BUILD_DLL_VERSIONINFO was set, use the loader.rc in the build dir, otherwise use the checked in file
set(RC_FILE_LOCATION ${CMAKE_CURRENT_SOURCE_DIR}/loader.rc)
if (NOT "$CACHE{BUILD_DLL_VERSIONINFO}" STREQUAL "")
set(RC_FILE_LOCATION ${CMAKE_CURRENT_BINARY_DIR}/loader.rc)
endif()

set(LOADER_UNKNOWN_CHAIN_LIBRARY $<$<TARGET_EXISTS:loader-unknown-chain>:$<TARGET_OBJECTS:loader-unknown-chain>>)

add_library(vulkan
SHARED
${NORMAL_LOADER_SRCS}
${LOADER_UNKNOWN_CHAIN_LIBRARY}
${CMAKE_CURRENT_SOURCE_DIR}/${API_TYPE}-1.def
${RC_FILE_LOCATION})

target_link_libraries(vulkan PRIVATE loader_specific_options loader-opt)
target_link_libraries(vulkan PRIVATE loader_specific_options)

# when adding the suffix the import and runtime library names must be consistent
# mingw: libvulkan-1.dll.a / vulkan-1.dll
Expand All @@ -377,8 +347,6 @@ if(WIN32)
target_link_libraries(vulkan PRIVATE cfgmgr32)
endif()

add_dependencies(vulkan loader_asm_gen_files)

else()
if(APPLE)
option(APPLE_STATIC_LOADER "Build a loader that can be statically linked. Intended for Chromium usage/testing.")
Expand All @@ -395,9 +363,7 @@ else()
add_library(vulkan SHARED)
endif()

target_sources(vulkan PRIVATE ${NORMAL_LOADER_SRCS} ${OPT_LOADER_SRCS})

add_dependencies(vulkan loader_asm_gen_files)
target_sources(vulkan PRIVATE ${NORMAL_LOADER_SRCS})

set_target_properties(vulkan PROPERTIES
SOVERSION "1"
Expand Down Expand Up @@ -432,9 +398,12 @@ else()
file(GLOB_RECURSE CONFIGURE_DEPENDS FRAMEWORK_HEADERS ${VulkanHeaders_INCLUDE_DIRS})

add_library(vulkan-framework SHARED)
target_sources(vulkan-framework PRIVATE ${NORMAL_LOADER_SRCS} ${OPT_LOADER_SRCS} ${FRAMEWORK_HEADERS})
target_sources(vulkan-framework PRIVATE ${NORMAL_LOADER_SRCS} ${FRAMEWORK_HEADERS})

if (UNKNOWN_FUNCTIONS_SUPPORTED)
add_dependencies(vulkan-framework loader_asm_gen_files)
endif()

add_dependencies(vulkan-framework loader_asm_gen_files)
target_link_libraries(vulkan-framework ${CMAKE_DL_LIBS} Threads::Threads -lm "-framework CoreFoundation")
target_link_libraries(vulkan-framework loader_specific_options)

Expand Down Expand Up @@ -488,6 +457,11 @@ target_link_libraries(vulkan PRIVATE loader_specific_options)
target_link_libraries(vulkan PRIVATE Vulkan::Headers)
add_library(Vulkan::Loader ALIAS vulkan)

if (UNKNOWN_FUNCTIONS_SUPPORTED)
target_compile_definitions(vulkan PRIVATE UNKNOWN_FUNCTIONS_SUPPORTED)
add_dependencies(vulkan loader_asm_gen_files)
endif()

if (APPLE_STATIC_LOADER)
# TLDR: This feature only exists at the request of Google for Chromium. No other project should use this!
message(NOTICE "Apple STATIC lib: it will be built but not installed, and vulkan.pc and VulkanLoaderConfig.cmake won't be generated!")
Expand Down
Loading

0 comments on commit ffb58e2

Please sign in to comment.