Thunks/gen: Specialize layout wrappers for pointer types

Pointer types inherently cause data layout compatibility issues, so they're worth special-casing here. The wrappers will type-pun pointers to 32-bit or 64-bit integers (matching the guest architecture) to avoid direct host-side use of guest pointers without consideration.
FEX-Emu · Dec 22, 2023 · ccf19ed · ccf19ed
1 parent 1e0620f
commit ccf19ed
Show file tree

Hide file tree

Showing 9 changed files with 163 additions and 21 deletions.
diff --git a/ThunkLibs/Generator/data_layout.cpp b/ThunkLibs/Generator/data_layout.cpp
@@ -85,6 +85,7 @@ std::unordered_map<const clang::Type*, TypeInfo> ComputeDataLayout(const clang::
                 .type_name = get_type_name(context, field_type),
                 .member_name = field->getNameAsString(),
                 .array_size = array_size,
+                .is_function_pointer = field_type->isFunctionPointerType(),
             };
 
             // TODO: Process types in dependency-order. Currently we skip this

diff --git a/ThunkLibs/Generator/data_layout.h b/ThunkLibs/Generator/data_layout.h
@@ -28,13 +28,15 @@ struct StructInfo : SimpleTypeInfo {
         std::string type_name;
         std::string member_name;
         std::optional<uint64_t> array_size;
+        bool is_function_pointer;
 
         bool operator==(const MemberInfo& other) const {
             return  size_bits == other.size_bits &&
                     offset_bits == other.offset_bits &&
                     type_name == other.type_name &&
                     member_name == other.member_name &&
-                    array_size == other.array_size;
+                    array_size == other.array_size &&
+                    is_function_pointer == other.is_function_pointer;
         }
     };
 

diff --git a/ThunkLibs/Generator/gen.cpp b/ThunkLibs/Generator/gen.cpp
@@ -216,7 +216,12 @@ void GenerateThunkLibsAction::EmitLayoutWrappers(
                 auto type_name = member->getType().getAsString();
                 auto array_type = llvm::dyn_cast<clang::ConstantArrayType>(member->getType());
                 if (!array_type && skip_arrays) {
-                    fmt::print(file, "      .{} = host_layout<{}> {{ from.data.{} }}.data,\n", decl_name, type_name, decl_name);
+                    if (member->getType()->isFunctionPointerType()) {
+                        // Function pointers must be handled manually, so zero them out by default
+                        fmt::print(file, "      .{} {{ }},\n", decl_name);
+                    } else {
+                        fmt::print(file, "      .{} = host_layout<{}> {{ from.data.{} }}.data,\n", decl_name, type_name, decl_name);
+                    }
                 } else if (array_type && !skip_arrays) {
                     // Copy element-wise below
                     fmt::print(file, "      for (size_t i = 0; i < {}; ++i) {{\n", array_type->getSize().getZExtValue());
@@ -250,7 +255,12 @@ void GenerateThunkLibsAction::EmitLayoutWrappers(
                 auto& decl_name = member.member_name;
                 auto& array_size = member.array_size;
                 if (!array_size && skip_arrays) {
-                    fmt::print(file, "    .{} = to_guest(to_host_layout(from.data.{})),\n", decl_name, decl_name);
+                    if (member.is_function_pointer) {
+                        // Function pointers must be handled manually, so zero them out by default
+                        fmt::print(file, "    .{} {{ }},\n", decl_name);
+                    } else {
+                        fmt::print(file, "    .{} = to_guest(to_host_layout(from.data.{})),\n", decl_name, decl_name);
+                    }
                 } else if (array_size && !skip_arrays) {
                     // Copy element-wise below
                     fmt::print(file, "    for (size_t i = 0; i < {}; ++i) {{\n", array_size.value());
@@ -589,7 +599,10 @@ void GenerateThunkLibsAction::OnAnalysisComplete(clang::ASTContext& context) {
             }
 
             if (!thunk.return_type->isVoidType()) {
-                fmt::print(file, "  args->rv = to_guest(to_host_layout<{}>(", thunk.return_type.getAsString());
+                fmt::print(file, "  args->rv = ");
+                if (!thunk.return_type->isFunctionPointerType()) {
+                    fmt::print(file, "to_guest(to_host_layout<{}>(", thunk.return_type.getAsString());
+                }
             }
             fmt::print(file, "{}(", function_to_call);
             {
@@ -600,9 +613,14 @@ void GenerateThunkLibsAction::OnAnalysisComplete(clang::ASTContext& context) {
                     if (cb != thunk.callbacks.end() && cb->second.is_stub) {
                         return "fexfn_unpack_" + get_callback_name(function_name, cb->first) + "_stub";
                     } else if (cb != thunk.callbacks.end()) {
-                        auto arg_name = fmt::format("args->a_{}.data", idx);
+                        auto arg_name = fmt::format("args->a_{}", idx); // Use parameter directly
                         // Use comma operator to inject a function call before returning the argument
-                        return "(FinalizeHostTrampolineForGuestFunction(" + arg_name + "), " + arg_name + ")";
+                        // TODO: Avoid casting away the guest_layout
+                        if (thunk.custom_host_impl) {
+                            return fmt::format("(FinalizeHostTrampolineForGuestFunction({}), {})", arg_name, arg_name);
+                        } else {
+                            return fmt::format("(FinalizeHostTrampolineForGuestFunction({}), ({})(uint64_t {{ {}.data }}))", arg_name, get_type_name(context, thunk.param_types[idx].getTypePtr()), arg_name);
+                        }
                     } else if (thunk.param_annotations[idx].is_passthrough) {
                         // Pass raw guest_layout<T*>
                         return fmt::format("args->a_{}", idx);
@@ -611,9 +629,9 @@ void GenerateThunkLibsAction::OnAnalysisComplete(clang::ASTContext& context) {
                     }
                 };
 
-                file << format_function_args(thunk, format_param);
+                fmt::print(file, "{}", format_function_args(thunk, format_param));
             }
-            if (!thunk.return_type->isVoidType()) {
+            if (!thunk.return_type->isVoidType() && !thunk.return_type->isFunctionPointerType()) {
                 fmt::print(file, "))");
             }
             fmt::print(file, ");\n");

diff --git a/ThunkLibs/include/common/Host.h b/ThunkLibs/include/common/Host.h
@@ -97,8 +97,63 @@ struct guest_layout {
   static_assert(!std::is_enum_v<T>, "No guest layout defined for this enum type. This is a bug in the thunk generator.");
   static_assert(!std::is_void_v<T>, "Attempted to get guest layout of void. Missing annotation for void pointer?");
 
-  using type = T;
+  using type = std::enable_if_t<!std::is_pointer_v<T>, T>;
   type data;
+
+  guest_layout& operator=(const T from) {
+    data = from;
+    return *this;
+  }
+};
+
+template<typename T>
+struct guest_layout<T*> {
+#ifdef IS_32BIT_THUNK
+  using type = uint32_t;
+#else
+  using type = uint64_t;
+#endif
+  type data;
+
+  // Allow implicit conversion for function pointers, since they disallow use of host_layout
+  guest_layout& operator=(const T* from) requires (std::is_function_v<T>) {
+    // TODO: Assert upper 32 bits are zero
+    data = reinterpret_cast<uintptr_t>(from);
+    return *this;
+  }
+
+  guest_layout<T>* get_pointer() {
+    return reinterpret_cast<guest_layout<T>*>(uintptr_t { data });
+  }
+
+  const guest_layout<T>* get_pointer() const {
+    return reinterpret_cast<const guest_layout<T>*>(uintptr_t { data });
+  }
+};
+
+template<typename T>
+struct guest_layout<T* const> {
+#ifdef IS_32BIT_THUNK
+  using type = uint32_t;
+#else
+  using type = uint64_t;
+#endif
+  type data;
+
+  // Allow implicit conversion for function pointers, since they disallow use of host_layout
+  guest_layout& operator=(const T* from) requires (std::is_function_v<T>) {
+    // TODO: Assert upper 32 bits are zero
+    data = reinterpret_cast<uintptr_t>(from);
+    return *this;
+  }
+
+  guest_layout<T>* get_pointer() {
+    return reinterpret_cast<guest_layout<T>*>(uintptr_t { data });
+  }
+
+  const guest_layout<T>* get_pointer() const {
+    return reinterpret_cast<const guest_layout<T>*>(uintptr_t { data });
+  }
 };
 
 template<typename T>
@@ -133,7 +188,32 @@ const host_layout<T>& to_host_layout(const T& t) {
 }
 
 template<typename T>
-inline guest_layout<T> to_guest(const host_layout<T>& from) {
+struct host_layout<T*> {
+  T* data;
+
+  static_assert(!std::is_function_v<T>, "Function types must be handled separately");
+
+  // Assume underlying data is compatible and just convert the guest-sized pointer to 64-bit
+  explicit host_layout(const guest_layout<T*>& from) : data { (T*)(uintptr_t)from.data } {
+  }
+
+  // TODO: Make this explicit?
+  host_layout() = default;
+};
+
+template<typename T>
+struct host_layout<T* const> {
+  T* data;
+
+  static_assert(!std::is_function_v<T>, "Function types must be handled separately");
+
+  // Assume underlying data is compatible and just convert the guest-sized pointer to 64-bit
+  explicit host_layout(const guest_layout<T* const>& from) : data { (T*)(uintptr_t)from.data } {
+  }
+};
+
+template<typename T>
+inline guest_layout<T> to_guest(const host_layout<T>& from) requires(!std::is_pointer_v<T>) {
   if constexpr (std::is_enum_v<T>) {
     // enums are represented by fixed-size integers in guest_layout, so explicitly cast them
     return guest_layout<T> { static_cast<std::underlying_type_t<T>>(from.data) };
@@ -143,6 +223,14 @@ inline guest_layout<T> to_guest(const host_layout<T>& from) {
   }
 }
 
+template<typename T>
+inline guest_layout<T*> to_guest(const host_layout<T*>& from) {
+  // TODO: Assert upper 32 bits are zero
+  guest_layout<T*> ret;
+  ret.data = reinterpret_cast<uintptr_t>(from.data);
+  return ret;
+}
+
 template<typename>
 struct CallbackUnpack;
 
@@ -254,6 +342,13 @@ void FinalizeHostTrampolineForGuestFunction(F* PreallocatedTrampolineForGuestFun
       (void*)&CallbackUnpack<F>::CallGuestPtr);
 }
 
+template<typename F>
+void FinalizeHostTrampolineForGuestFunction(guest_layout<F*> PreallocatedTrampolineForGuestFunction) {
+  FEXCore::FinalizeHostTrampolineForGuestFunction(
+      (FEXCore::HostToGuestTrampolinePtr*)PreallocatedTrampolineForGuestFunction.data,
+      (void*)&CallbackUnpack<F>::CallGuestPtr);
+}
+
 // In the case of the thunk host_loader being the default, FEX need to use dlsym with RTLD_DEFAULT.
 // If FEX queried the symbol object directly then it wouldn't follow symbol overriding rules.
 //

diff --git a/ThunkLibs/libvulkan/Host.cpp b/ThunkLibs/libvulkan/Host.cpp
@@ -77,11 +77,21 @@ static VkResult FEXFN_IMPL(vkCreateInstance)(const VkInstanceCreateInfo* a_0, co
     }
   }
 
-  return LDR_PTR(vkCreateInstance)(vk_struct_base, nullptr, a_2.data);
+  VkInstance out;
+  auto ret = LDR_PTR(vkCreateInstance)(vk_struct_base, nullptr, &out);
+  if (ret == VK_SUCCESS) {
+    *a_2.get_pointer() = to_guest(to_host_layout(out));
+  }
+  return ret;
 }
 
 static VkResult FEXFN_IMPL(vkCreateDevice)(VkPhysicalDevice a_0, const VkDeviceCreateInfo* a_1, const VkAllocationCallbacks* a_2, guest_layout<VkDevice*> a_3){
-  return LDR_PTR(vkCreateDevice)(a_0, a_1, nullptr, a_3.data);
+  VkDevice out;
+  auto ret = LDR_PTR(vkCreateDevice)(a_0, a_1, nullptr, &out);
+  if (ret == VK_SUCCESS) {
+    *a_3.get_pointer() = to_guest(to_host_layout(out));
+  }
+  return ret;
 }
 
 static VkResult FEXFN_IMPL(vkAllocateMemory)(VkDevice a_0, const VkMemoryAllocateInfo* a_1, const VkAllocationCallbacks* a_2, VkDeviceMemory* a_3){
@@ -95,7 +105,7 @@ static void FEXFN_IMPL(vkFreeMemory)(VkDevice a_0, VkDeviceMemory a_1, const VkA
 }
 
 static VkResult FEXFN_IMPL(vkCreateDebugReportCallbackEXT)(VkInstance a_0, guest_layout<const VkDebugReportCallbackCreateInfoEXT*> a_1, const VkAllocationCallbacks* a_2, VkDebugReportCallbackEXT* a_3) {
-  VkDebugReportCallbackCreateInfoEXT overridden_callback = *a_1.data;
+  auto overridden_callback = host_layout<VkDebugReportCallbackCreateInfoEXT> { *a_1.get_pointer() }.data;
   overridden_callback.pfnCallback = DummyVkDebugReportCallback;
   (void*&)LDR_PTR(vkCreateDebugReportCallbackEXT) = (void*)LDR_PTR(vkGetInstanceProcAddr)(a_0, "vkCreateDebugReportCallbackEXT");
   return LDR_PTR(vkCreateDebugReportCallbackEXT)(a_0, &overridden_callback, nullptr, a_3);
@@ -115,7 +125,7 @@ extern "C" VkBool32 DummyVkDebugUtilsMessengerCallback(
 static VkResult FEXFN_IMPL(vkCreateDebugUtilsMessengerEXT)(
     VkInstance_T* a_0, guest_layout<const VkDebugUtilsMessengerCreateInfoEXT*> a_1,
     const VkAllocationCallbacks* a_2, VkDebugUtilsMessengerEXT* a_3) {
-  VkDebugUtilsMessengerCreateInfoEXT overridden_callback = *a_1.data;
+  auto overridden_callback = host_layout<VkDebugUtilsMessengerCreateInfoEXT> { *a_1.get_pointer() }.data;
   overridden_callback.pfnUserCallback = DummyVkDebugUtilsMessengerCallback;
   (void*&)LDR_PTR(vkCreateDebugUtilsMessengerEXT) = (void*)LDR_PTR(vkGetInstanceProcAddr)(a_0, "vkCreateDebugUtilsMessengerEXT");
   return LDR_PTR(vkCreateDebugUtilsMessengerEXT)(a_0, &overridden_callback, nullptr, a_3);

diff --git a/ThunkLibs/libvulkan/libvulkan_interface.cpp b/ThunkLibs/libvulkan/libvulkan_interface.cpp
@@ -52,6 +52,11 @@ template<> struct fex_gen_type<VkDeviceOrHostAddressConstKHR> : fexgen::assume_c
 template<> struct fex_gen_type<VkPerformanceValueDataINTEL> : fexgen::assume_compatible_data_layout {};
 #endif
 
+// Structures that contain function pointers
+// TODO: Use custom repacking for these instead
+template<> struct fex_gen_type<VkDebugReportCallbackCreateInfoEXT> : fexgen::emit_layout_wrappers {};
+template<> struct fex_gen_type<VkDebugUtilsMessengerCreateInfoEXT> : fexgen::emit_layout_wrappers {};
+
 #ifndef IS_32BIT_THUNK
 // The pNext member of this is a pointer to another VkBaseOutStructure, so data layout compatibility can't be inferred automatically
 template<> struct fex_gen_type<VkBaseOutStructure> : fexgen::assume_compatible_data_layout {};

diff --git a/ThunkLibs/libwayland-client/Host.cpp b/ThunkLibs/libwayland-client/Host.cpp
@@ -60,7 +60,7 @@ extern "C" int fexfn_impl_libwayland_client_wl_proxy_add_listener(struct wl_prox
     // ? just indicates that the argument may be null, so it doesn't change the signature
     signature.erase(std::remove(signature.begin(), signature.end(), '?'), signature.end());
 
-    auto callback = callback_raw.data[i];
+    auto callback = reinterpret_cast<void(*)()>(uintptr_t { callback_raw.get_pointer()[i].data });
 
     if (signature == "") {
       // E.g. xdg_toplevel::close
@@ -162,7 +162,9 @@ extern "C" int fexfn_impl_libwayland_client_wl_proxy_add_listener(struct wl_prox
   }
 
   // Pass the original function pointer table to the host wayland library. This ensures the table is valid until the listener is unregistered.
-  return fexldr_ptr_libwayland_client_wl_proxy_add_listener(proxy, callback_raw.data, data);
+  return fexldr_ptr_libwayland_client_wl_proxy_add_listener(proxy,
+                                                            reinterpret_cast<void(**)(void)>(callback_raw.get_pointer()),
+                                                            data);
 }
 
 wl_interface* fexfn_impl_libwayland_client_fex_wl_exchange_interface_pointer(wl_interface* guest_interface, char const* name) {

diff --git a/unittests/FEXLinuxTests/tests/thunks/thunk_testlib.cpp b/unittests/FEXLinuxTests/tests/thunks/thunk_testlib.cpp
@@ -30,8 +30,6 @@ TEST_CASE_METHOD(Fixture, "Trivial") {
   CHECK(GetDoubledValue(10) == 20);
 }
 
-// Passing pointers across architecture boundaries is not yet supported on 32-bit
-#if __SIZEOF_POINTER__ != 4
 TEST_CASE_METHOD(Fixture, "Opaque data types") {
   {
     auto data = MakeOpaqueType(0x1234);
@@ -44,4 +42,3 @@ TEST_CASE_METHOD(Fixture, "Opaque data types") {
     CHECK(GetUnionTypeA(&data) == 0x04030201);
   }
 }
-#endif
diff --git a/unittests/ThunkLibs/generator.cpp b/unittests/ThunkLibs/generator.cpp
@@ -287,14 +287,26 @@ SourceWithAST Fixture::run_thunkgen_host(std::string_view prelude, std::string_v
         "};\n"
         "\n"
         "template<typename T>\n"
+        "struct guest_layout<T*> {\n"
+        "#ifdef IS_32BIT_THUNK\n"
+        "  using type = uint32_t;\n"
+        "#else\n"
+        "  using type = uint64_t;\n"
+        "#endif\n"
+        "  type data;\n"
+        "};\n"
+        "\n"
+        "template<typename T>\n"
         "struct host_layout {\n"
         "  T data;\n"
         "\n"
         "  host_layout(const guest_layout<T>& from);\n"
         "};\n"
         "\n"
-        "template<typename T> guest_layout<T> to_guest(const host_layout<T>& from);\n"
+        "template<typename T> guest_layout<T> to_guest(const host_layout<T>& from) requires(!std::is_pointer_v<T>);\n"
+        "template<typename T> guest_layout<T*> to_guest(const host_layout<T*>& from);\n"
         "template<typename F> void FinalizeHostTrampolineForGuestFunction(F*);\n"
+        "template<typename F> void FinalizeHostTrampolineForGuestFunction(guest_layout<F*>);\n"
         "template<typename T> const host_layout<T>& to_host_layout(const T& t);\n";
 
     auto& filename = output_filenames.host;
@@ -434,7 +446,7 @@ TEST_CASE_METHOD(Fixture, "FunctionPointerParameter") {
             hasDescendant(callExpr(callee(functionDecl(hasName("FinalizeHostTrampolineForGuestFunction"))), hasArgument(0, expr().bind("funcptr"))))
         )).check_binding("funcptr", +[](const clang::Expr* funcptr) {
             // Check that the argument type matches the function pointer
-            return funcptr->getType().getAsString() == "int (*)(char, char)";
+            return funcptr->getType().getAsString() == "guest_layout<int (*)(char, char)>";
         }));
 
     // Host should export the unpacking function for function pointer arguments