getInstanceProcAddress can't be used with zig-vulkan

[Avokadoen]

glfwGetInstanceProcAddress is used in zig-vulkan to gather all function addresses required for the requested vkInstance. Here is a generated code example:

        //          v--- Loader here is glfwGetInstanceProcAddress 
        pub fn load(loader: anytype) !Self {
            var self: Self = undefined;
            inline for (std.meta.fields(Dispatch)) |field| {
                const name = @ptrCast([*:0]const u8, field.name ++ "\x00");
                const cmd_ptr = loader(.null_handle, name) orelse return error.CommandLoadFailure;
                @field(self.dispatch, field.name) = @ptrCast(field.field_type, cmd_ptr);
            }
            return self;
        }

In the definition of glfw.getInstanceProcAddress there are two major issues.

1. Function is inlined. This means the call convention is not transitive with zig-vulkan which expect a vulkan_call_conv which is defined as:

pub const vulkan_call_conv: std.builtin.CallingConvention = if (builtin.os.tag == .windows and builtin.cpu.arch == .i386)
    .Stdcall
else if (builtin.abi == .android and (builtin.cpu.arch.isARM() or builtin.cpu.arch.isThumb()) and std.Target.arm.featureSetHas(builtin.cpu.features, .has_v7) and builtin.cpu.arch.ptrBitWidth() == 32)
    // On Android 32-bit ARM targets, Vulkan functions use the "hardfloat"
    // calling convention, i.e. float parameters are passed in registers. This
    // is true even if the rest of the application passes floats on the stack,
    // as it does by default when compiling for the armeabi-v7a NDK ABI.
    .AAPCSVFP
else
    .C;

2. The returned signature does not match. mach-glfw define the return value as Error!?VKProc while glfw define it as ?VKProc

I'm unsure how this should be resolved, but two suggestions are:

• Make a builder for the function that takes a ABI = enum { Preserved, NotPreserved } where based on input enum it returns either current implementation or a true to ABI function
• Rewrite function to preserve ABI

[Avokadoen]

This works:

mach-glfw:

pub fn getInstanceProcAddress(vk_instance: ?*opaque {}, proc_name: [*c]const u8) callconv(.C) ?VKProc {
    const proc_address = c.glfwGetInstanceProcAddress(if (vk_instance) |v| @ptrCast(c.VkInstance, v) else null, proc_name);
    getError() catch {}; // ignore errors for now
    if (proc_address) |addr| return addr;
    return null;
}

user code:

// load base dispatch wrapper
const vk_proc = @ptrCast(fn(instance: vk.Instance, procname: [*:0]const u8) vk.PfnVoidFunction, glfw.getInstanceProcAddress);
self.vkb = try dispatch.Base.load(vk_proc);

[slimsag]

Thanks for flagging this, I knew the Vulkan procs had a different calling convention but didn't realize I had made them inline - habit from working on this code I suppose :)

I think we can preserve the ABI as you suggest even without returning a different implementation, by simply panicking on error and indicating that we do so in the docstring. That's not perfect, but preserving ABI seems far more important for this function.

This also generally matches the C usage of the function:

mach/glfw/upstream/glfw/include/GLFW/glfw3.h

Lines 5754 to 5755 in 3d38f56

	* @return The address of the function, or `NULL` if an
	* [error](@ref error_handling) occurred.

mach/glfw/upstream/glfw/include/GLFW/glfw3.h

Lines 5757 to 5758 in 3d38f56

	* @errors Possible errors include @ref GLFW_NOT_INITIALIZED and @ref
	* GLFW_API_UNAVAILABLE.

For ours, it will just be a requirement that the caller has ensured GLFW is initialized and that glfw.vulkanSupported reported true, or else they can receive a panic.

[Avokadoen]

One thing related to glfw init checks: in zig it is trivial to move such a check do compile time! Of course it would be a lot of work simply because of the scale of the API

Example: https://github.com/Avokadoen/zig_vulkan/blob/f7ae8834aba6b7f99c6c53e5272a813844ff60b2/src/sprite/sprite.zig#L170

[slimsag]

I don't think it's possible if your initialization requires heap allocation (which is true in the case of glfwInit and its internal implementation). Additionally because it calls into native X11/Wayland/Cocoa/etc APIs internally, it must be performed at runtime not comptime even if comptime heap allocation was possible - similar to if you needed to open a file at comptime, the file handle would become invalid at runtime.

Unless I've misunderstood something and you have a trick there for enforcing at comptime that a runtime function is to be called.

[Avokadoen]

It's possible. The example i sent shows code that does it. You can run custom checks on compile time in your functions to verify on compile time whether the user has called glfw.init at any point in the code base. It might however not be something that we would want in the library 😅

[slimsag]

Very interesting, will read more into this later and come back with more informed thoughts :)

[slimsag]

One thing I should note: The actual calling convention of this is not the same as the Vulkan calling convention you had posted in the issue description. The GLFW definition is:

GLFWAPI GLFWvkproc glfwGetInstanceProcAddress(VkInstance instance, const char* procname);

And GLFWAPI is defined as just:

/* GLFWAPI is used to declare public API functions for export
 * from the DLL / shared library / dynamic library.
 */
#if defined(_WIN32) && defined(_GLFW_BUILD_DLL)
 /* We are building GLFW as a Win32 DLL */
 #define GLFWAPI __declspec(dllexport)
#elif defined(_WIN32) && defined(GLFW_DLL)
 /* We are calling GLFW as a Win32 DLL */
 #define GLFWAPI __declspec(dllimport)
#elif defined(__GNUC__) && defined(_GLFW_BUILD_DLL)
 /* We are building GLFW as a shared / dynamic library */
 #define GLFWAPI __attribute__((visibility("default")))
#else
 /* We are building or calling GLFW as a static library */
 #define GLFWAPI
#endif

So, it's the C calling convention (since _GLFW_BUILD_DLL is false for us)

And actually if we look closely at the generate Zig code you had posted, we can also see it doesn't specify a calling convention either.

Now, having the C calling convention specified here is still a good idea (someone may want to pass this into a C library) - just thought I'd note this.

[slimsag]

Should be fixed, will sync to the mach-glfw repo later today. Also created follow-up issues for doing the same for OpenGL proc addr functions, and for investigating the comptime init idea:

• glfw: maintain C ABI compatability for OpenGL proc addrs #52
• glfw: add comptime enforcement of init #53