Merge pull request #3371 from neobrain/feature_thunk_automatic_repacking

Library Forwarding: Implement automatic argument repacking

ThunkLibs/Generator/gen.cpp

@@ -144,6 +144,9 @@ void GenerateThunkLibsAction::EmitLayoutWrappers(
 
         // Opaque types don't need layout definitions
         if (type_repack_info.assumed_compatible && type_repack_info.pointers_only) {
+            if (guest_abi.pointer_size != 4) {
+                fmt::print(file, "template<> inline constexpr bool has_compatible_data_layout<{}*> = true;\n", struct_name);
+            }
             continue;
         } else if (type_repack_info.assumed_compatible) {
             // TODO: Handle more cleanly
@@ -280,6 +283,9 @@ void GenerateThunkLibsAction::EmitLayoutWrappers(
         }
         fmt::print(file, "  return ret;\n");
         fmt::print(file, "}}\n\n");
+
+        fmt::print(file, "template<> inline constexpr bool has_compatible_data_layout<{}> = {};\n",
+                   struct_name, (type_compat.at(type) == TypeCompatibility::Full));
     }
 }
 
@@ -563,6 +569,16 @@ void GenerateThunkLibsAction::OnAnalysisComplete(clang::ASTContext& context) {
                 function_to_call = "fexfn_impl_" + libname + "_" + function_name;
             }
 
+            auto get_type_name_with_nonconst_pointee = [&](clang::QualType type) {
+                type = type.getLocalUnqualifiedType();
+                if (type->isPointerType()) {
+                    // Strip away "const" from pointee type
+                    type = context.getPointerType(type->getPointeeType().getLocalUnqualifiedType());
+                }
+                return get_type_name(context, type.getTypePtr());
+            };
+
+
             file << "static void fexfn_unpack_" << libname << "_" << function_name << "(" << struct_name << "* args) {\n";
 
             for (unsigned param_idx = 0; param_idx != thunk.param_types.size(); ++param_idx) {
@@ -592,7 +608,8 @@ void GenerateThunkLibsAction::OnAnalysisComplete(clang::ASTContext& context) {
                     // Fully compatible
                     fmt::print(file, "  host_layout<{}> a_{} {{ args->a_{} }};\n", get_type_name(context, param_type.getTypePtr()), param_idx, param_idx);
                 } else if (pointee_compat == TypeCompatibility::Repackable) {
-                    throw report_error(thunk.decl->getLocation(), "Pointer parameter %1 of function %0 requires automatic repacking, which is not implemented yet").AddString(function_name).AddTaggedVal(param_type);
+                    // TODO: Require opt-in for this to be emitted since it's single-element only; otherwise, pointers-to-arrays arguments will cause stack trampling
+                    fmt::print(file, "  auto a_{} = make_repack_wrapper<{}>(args->a_{});\n", param_idx, get_type_name_with_nonconst_pointee(param_type), param_idx);
                 } else {
                     throw report_error(thunk.decl->getLocation(), "Cannot generate unpacking function for function %0 with unannotated pointer parameter %1").AddString(function_name).AddTaggedVal(param_type);
                 }
@@ -607,8 +624,6 @@ void GenerateThunkLibsAction::OnAnalysisComplete(clang::ASTContext& context) {
             fmt::print(file, "{}(", function_to_call);
             {
                 auto format_param = [&](std::size_t idx) {
-                    std::string raw_arg = fmt::format("a_{}.data", idx);
-
                     auto cb = thunk.callbacks.find(idx);
                     if (cb != thunk.callbacks.end() && cb->second.is_stub) {
                         return "fexfn_unpack_" + get_callback_name(function_name, cb->first) + "_stub";
@@ -625,7 +640,8 @@ void GenerateThunkLibsAction::OnAnalysisComplete(clang::ASTContext& context) {
                         // Pass raw guest_layout<T*>
                         return fmt::format("args->a_{}", idx);
                     } else {
-                        return raw_arg;
+                        // Unwrap host_layout/repack_wrapper layer
+                        return fmt::format("unwrap_host(a_{})", idx);
                     }
                 };
 

ThunkLibs/include/common/Host.h

@@ -11,6 +11,7 @@ category: thunklibs ~ These are generated + glue logic 1:1 thunks unless noted o
 #include <cstdlib>
 #include <cstring>
 #include <dlfcn.h>
+#include <optional>
 
 #include "PackedArguments.h"
 
@@ -89,6 +90,35 @@ struct ParameterAnnotations {
     bool assume_compatible = false;
 };
 
+// Generator emits specializations for this for each type that has compatible layout
+template<typename T>
+inline constexpr bool has_compatible_data_layout =
+  std::is_integral_v<T> || std::is_enum_v<T> || std::is_floating_point_v<T>
+#ifndef IS_32BIT_THUNK
+  // If none of the previous predicates matched, the thunk generator did *not* emit a specialization for T.
+  // This should not happen on 64-bit with the currently thunked libraries, since their types
+  // * either have fully consistent data layout across 64-bit architectures.
+  // * or use custom repacking, in which case has_compatible_data_layout isn't used
+  //
+  // Throwing a fake exception here will trigger a build failure.
+  || (throw "Instantiated on a type that was expected to be compatible", true)
+#endif
+;
+
+#ifndef IS_32BIT_THUNK
+// Pointers have the same size, hence data layout compatibility only depends on the pointee type
+template<typename T>
+inline constexpr bool has_compatible_data_layout<T*> = has_compatible_data_layout<std::remove_cv_t<T>>;
+template<typename T>
+inline constexpr bool has_compatible_data_layout<T* const> = has_compatible_data_layout<std::remove_cv_t<T>*>;
+
+// void* and void** are assumed to be compatible to simplify handling of libraries that use them ubiquitously
+template<> inline constexpr bool has_compatible_data_layout<void*> = true;
+template<> inline constexpr bool has_compatible_data_layout<const void*> = true;
+template<> inline constexpr bool has_compatible_data_layout<void**> = true;
+template<> inline constexpr bool has_compatible_data_layout<const void**> = true;
+#endif
+
 // Placeholder type to indicate the given data is in guest-layout
 template<typename T>
 struct guest_layout {
@@ -97,6 +127,8 @@ 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?");
 
+  static_assert(std::is_fundamental_v<T> || has_compatible_data_layout<T>, "Default guest_layout may not be used for non-compatible data");
+
   using type = std::enable_if_t<!std::is_pointer_v<T>, T>;
   type data;
 
@@ -212,6 +244,63 @@ struct host_layout<T* const> {
   }
 };
 
+// Wrapper around host_layout that repacks from a guest_layout on construction
+// and exit-repacks on scope exit (if needed). The wrapper manages the storage
+// needed for repacked data itself.
+// This also implicitly converts to a pointer of the wrapped host type, since
+// this conversion is required at all call sites anyway
+template<typename T, typename GuestT>
+struct repack_wrapper {
+  static_assert(std::is_pointer_v<T>);
+
+  // Strip "const" from pointee type in host_layout storage
+  using PointeeT = std::remove_cv_t<std::remove_pointer_t<T>>;
+
+  std::optional<host_layout<PointeeT>> data;
+  guest_layout<GuestT>& orig_arg;
+
+  repack_wrapper(guest_layout<GuestT>& orig_arg_) : orig_arg(orig_arg_) {
+    if (orig_arg.get_pointer()) {
+      data = { *orig_arg_.get_pointer() };
+    }
+  }
+
+  ~repack_wrapper() {
+    // TODO: Properly detect opaque types
+    if constexpr (requires(guest_layout<T> t, decltype(data) h) { t.get_pointer(); (bool)h; *data; }) {
+      if constexpr (!std::is_const_v<std::remove_pointer_t<T>>) { // Skip exit-repacking for const pointees
+        if (data) {
+          constexpr bool is_compatible = has_compatible_data_layout<T> && std::is_same_v<T, GuestT>;
+          if constexpr (!is_compatible && std::is_class_v<std::remove_pointer_t<T>>) {
+            *orig_arg.get_pointer() = to_guest(*data); // TODO: Only if annotated as out-parameter
+          }
+        }
+      }
+    }
+  }
+
+  operator PointeeT*() {
+    static_assert(sizeof(PointeeT) == sizeof(host_layout<PointeeT>));
+    static_assert(alignof(PointeeT) == alignof(host_layout<PointeeT>));
+    return data ? &data.value().data : nullptr;
+  }
+};
+
+template<typename T, typename GuestT>
+static repack_wrapper<T, GuestT> make_repack_wrapper(guest_layout<GuestT>& orig_arg) {
+  return { orig_arg };
+}
+
+template<typename T>
+T& unwrap_host(host_layout<T>& val) {
+  return val.data;
+}
+
+template<typename T, typename T2>
+T* unwrap_host(repack_wrapper<T*, T2>& val) {
+  return val;
+}
+
 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>) {
@@ -234,6 +323,34 @@ inline guest_layout<T*> to_guest(const host_layout<T*>& from) {
 template<typename>
 struct CallbackUnpack;
 
+template<typename T, ParameterAnnotations Annotation>
+constexpr bool IsCompatible() {
+  if constexpr (Annotation.assume_compatible) {
+    return true;
+  } else if constexpr (has_compatible_data_layout<T>) {
+    return true;
+  } else {
+    if constexpr (std::is_pointer_v<T>) {
+      return has_compatible_data_layout<std::remove_cv_t<std::remove_pointer_t<T>>>;
+    } else {
+      return false;
+    }
+  }
+}
+
+template<ParameterAnnotations Annotation, typename HostT, typename T>
+auto Projection(guest_layout<T>& data) {
+  if constexpr (Annotation.is_passthrough) {
+    return data;
+  } else if constexpr ((IsCompatible<T, Annotation>() && std::is_same_v<T, HostT>) || !std::is_pointer_v<T>) {
+    return host_layout<HostT> { data }.data;
+  } else {
+    // This argument requires temporary storage for repacked data
+    // *and* it needs to call custom repack functions (if any)
+    return make_repack_wrapper<HostT>(data);
+  }
+}
+
 template<typename Result, typename... Args>
 struct CallbackUnpack<Result(Args...)> {
   static Result CallGuestPtr(Args... args) {
@@ -250,15 +367,6 @@ struct CallbackUnpack<Result(Args...)> {
   }
 };
 
-template<ParameterAnnotations Annotation, typename T>
-auto Projection(guest_layout<T>& data) {
-  if constexpr (Annotation.is_passthrough) {
-    return data;
-  } else {
-    return host_layout<T> { data }.data;
-  }
-}
-
 template<typename>
 struct GuestWrapperForHostFunction;
 

ThunkLibs/libfex_thunk_test/Host.cpp

@@ -13,4 +13,12 @@ tags: thunklibs|fex_thunk_test
 
 #include "thunkgen_host_libfex_thunk_test.inl"
 
+static uint32_t fexfn_impl_libfex_thunk_test_QueryOffsetOf(guest_layout<ReorderingType*> data, int index) {
+    if (index == 0) {
+        return offsetof(guest_layout<ReorderingType>::type, a);
+    } else {
+        return offsetof(guest_layout<ReorderingType>::type, b);
+    }
+}
+
 EXPORTS(libfex_thunk_test)

ThunkLibs/libfex_thunk_test/api.h

@@ -28,4 +28,25 @@ union UnionType {
 UnionType MakeUnionType(uint8_t a, uint8_t b, uint8_t c, uint8_t d);
 uint32_t GetUnionTypeA(UnionType*);
 
+
+/// Interfaces used to test automatic struct repacking
+
+// A simple struct with data layout that differs between guest and host.
+// The thunk generator should emit code that swaps the member data into
+// correct position.
+struct ReorderingType {
+#if !defined(GUEST_THUNK_LIBRARY)
+    uint32_t a;
+    uint32_t b;
+#else
+    uint32_t b;
+    uint32_t a;
+#endif
+};
+
+ReorderingType MakeReorderingType(uint32_t a, uint32_t b);
+uint32_t GetReorderingTypeMember(ReorderingType*, int index);
+void ModifyReorderingTypeMembers(ReorderingType* data);
+uint32_t QueryOffsetOf(ReorderingType*, int index);
+
 }

ThunkLibs/libfex_thunk_test/lib.cpp

@@ -30,4 +30,21 @@ uint32_t GetUnionTypeA(UnionType* value) {
   return value->a;
 }
 
+ReorderingType MakeReorderingType(uint32_t a, uint32_t b) {
+  return ReorderingType { .a = a, .b = b };
+}
+
+uint32_t GetReorderingTypeMember(ReorderingType* data, int index) {
+  if (index == 0) {
+    return data->a;
+  } else {
+    return data->b;
+  }
+}
+
+void ModifyReorderingTypeMembers(ReorderingType* data) {
+  data->a += 1;
+  data->b += 2;
+}
+
 } // extern "C"

ThunkLibs/libfex_thunk_test/libfex_thunk_test_interface.cpp

@@ -21,3 +21,10 @@ template<> struct fex_gen_config<DestroyOpaqueType> {};
 template<> struct fex_gen_type<UnionType> : fexgen::assume_compatible_data_layout {};
 template<> struct fex_gen_config<MakeUnionType> {};
 template<> struct fex_gen_config<GetUnionTypeA> {};
+
+template<> struct fex_gen_config<MakeReorderingType> {};
+template<> struct fex_gen_config<GetReorderingTypeMember> {};
+template<> struct fex_gen_config<ModifyReorderingTypeMembers> {};
+
+template<> struct fex_gen_config<QueryOffsetOf> : fexgen::custom_host_impl {};
+template<> struct fex_gen_param<QueryOffsetOf, 0, ReorderingType*> : fexgen::ptr_passthrough {};

ThunkLibs/libvulkan/libvulkan_interface.cpp

@@ -52,6 +52,9 @@ template<> struct fex_gen_type<VkDeviceOrHostAddressConstKHR> : fexgen::assume_c
 template<> struct fex_gen_type<VkPerformanceValueDataINTEL> : fexgen::assume_compatible_data_layout {};
 #endif
 
+// Explicitly register types that are only ever referenced through nested pointers
+template<> struct fex_gen_type<VkAccelerationStructureBuildRangeInfoKHR> {};
+
 // Structures that contain function pointers
 // TODO: Use custom repacking for these instead
 template<> struct fex_gen_type<VkDebugReportCallbackCreateInfoEXT> : fexgen::emit_layout_wrappers {};

unittests/FEXLinuxTests/tests/thunks/thunk_testlib.cpp

@@ -24,6 +24,11 @@ struct Fixture {
 
   GET_SYMBOL(MakeUnionType);
   GET_SYMBOL(GetUnionTypeA);
+
+  GET_SYMBOL(MakeReorderingType);
+  GET_SYMBOL(GetReorderingTypeMember);
+  GET_SYMBOL(ModifyReorderingTypeMembers);
+  GET_SYMBOL(QueryOffsetOf);
 };
 
 TEST_CASE_METHOD(Fixture, "Trivial") {
@@ -42,3 +47,25 @@ TEST_CASE_METHOD(Fixture, "Opaque data types") {
     CHECK(GetUnionTypeA(&data) == 0x04030201);
   }
 }
+
+TEST_CASE_METHOD(Fixture, "Automatic struct repacking") {
+  {
+    // Test repacking of return values
+    ReorderingType test_struct = MakeReorderingType(0x1234, 0x5678);
+    REQUIRE(test_struct.a == 0x1234);
+    REQUIRE(test_struct.b == 0x5678);
+
+    // Test offsets of the host-side guest_layout wrapper match the guest-side ones
+    CHECK(QueryOffsetOf(&test_struct, 0) == offsetof(ReorderingType, a));
+    CHECK(QueryOffsetOf(&test_struct, 1) == offsetof(ReorderingType, b));
+
+    // Test repacking of input pointers
+    CHECK(GetReorderingTypeMember(&test_struct, 0) == 0x1234);
+    CHECK(GetReorderingTypeMember(&test_struct, 1) == 0x5678);
+
+    // Test repacking of output pointers
+    ModifyReorderingTypeMembers(&test_struct);
+    CHECK(GetReorderingTypeMember(&test_struct, 0) == 0x1235);
+    CHECK(GetReorderingTypeMember(&test_struct, 1) == 0x567a);
+  };
+}