Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Functions Pointers #19

Open
leopoldek opened this issue Aug 19, 2021 · 8 comments
Open

Functions Pointers #19

leopoldek opened this issue Aug 19, 2021 · 8 comments

Comments

@leopoldek
Copy link

Some of the extensions I'm using are optional which means that sometimes the function pointers will not exist. The problem with this is that the api fails when it hits a function pointer it can't find.
const cmd_ptr = loader(.null_handle, name) orelse return error.CommandLoadFailure;
Maybe it could return one after it has set all the pointers.
Also, why not just load all the function pointers you can instead of having to define them one-by-one? If the pointer doesn't exist just skip it. Less hassle.

Finally, I think you could probably offer a builtin load library to remove the dependency on something like GLFW. Here's what I use:

const libnames = switch(builtin.os.tag){
    .windows => [_][:0]const u8{"vulkan-1.dll"},
    .macos, .tvos, .watchos, .ios => [_][:0]const u8{"libvulkan.dylib", "libvulkan.1.dylib", "libMoltenVK.dylib"},
    .linux, .freebsd, .openbsd => [_][:0]const u8{"libvulkan.so.1", "libvulkan.so"},
    else => [_][:0]const u8{},
};

pub fn loadLibrary() error{InitializationFailed}!PfnGetInstanceProcAddr{
    var lib: std.DynLib = for(libnames) |name|{
        break std.DynLib.open(name) catch continue;
    }else return error.InitializationFailed;
    errdefer lib.close();
    return lib.lookup(PfnGetInstanceProcAddr, "vkGetInstanceProcAddr") orelse error.InitializationFailed;
}
@Snektron
Copy link
Owner

Some of the extensions I'm using are optional which means that sometimes the function pointers will not exist. The problem with this is that the api fails when it hits a function pointer it can't find.
const cmd_ptr = loader(.null_handle, name) orelse return error.CommandLoadFaiure

My idea for doing this was to create different wrapper structs depending on the required features:

const DeviceDispatch = vk.DeviceWrapper(.{
  .DestroyDevice,
});

const DeviceSwapchainDispatch = vk.DeviceWrapper(.{
  .CreateSwapchainKHR,
});

device_functions: DeviceDispatch,
swapchain_functions: ?SwapchainDispatch,

Also, why not just load all the function pointers you can instead of having to define them one-by-one? If the pointer doesn't exist just skip it. Less hassle.

I don't think loading all the function pointers at once by default is a good idea, though there might be something that can be done here. For example, since #11 wrappers are made by passing an array of the desired functions instead of a struct, and so the generator could export array literals that define all commands of different versions/extensions. Think something like vk.DeviceWrapper(vk.versions.vk10.functions ++ vk.extensions.VK_KHR_swapchain.functions) or vk.DeviceWrapper(vk.all_functions) or similar.

Finally, I think you could probably offer a builtin load library to remove the dependency on something like GLFW.

There is no hard dependency on GLFW, that is just for the example. That uses GLFW for the window, and GLFW loads Vulkan regardless (if the Vulkan backend is selected). Loading the Vulkan dynamic library yourself while also using GLFW is asking for trouble, because both implementations might resolve to a different dynamic library (for instance, one might choose the SDK version and the other the system version), which leads to horrible nightmares.

@leopoldek
Copy link
Author

I was actually able to fix it on my local copy by modifying the renderer. I ran into some problems like extensions that only existed on certain platforms, which I fixed by adding a whitelist parameter to the build and only loading functions not part of an extension or from an extension from the whitelist. I can submit a PR if you'd like but I'm currently using the 0.8 compat branch so there may be some merge conflicts.

Also, I made some convenience changes to the wrappers that allow you to pass in the instance/device+allocator and saves it in the wrapper since AFAIK they cannot change once they are set so no point in manually passing it in each time, if you're also interested in that.
In general, this is what it currently looks like:

pub const InstanceWrapper = comptime blk: {
    const cmds = @typeInfo(InstanceCommand).Enum.fields;
    comptime var fields: [cmds.len]std.builtin.TypeInfo.StructField = undefined;
    inline for (cmds) |cmd, i| {
        const cmd_type = @field(pfn, cmd.name);
        fields[i] = .{
            .name = "vk" ++ cmd.name,
            .field_type = cmd_type,
            .default_value = null,
            .is_comptime = false,
            .alignment = @alignOf(*cmd_type),
        };
    }
    const Dispatch = @Type(.{
        .Struct = .{
            .layout = .Auto,
            .fields = &fields,
            .decls = &[_]std.builtin.TypeInfo.Declaration{},
            .is_tuple = false,
        },
    });
    break :blk struct {
        handle: Instance,
        allocator: ?*const AllocationCallbacks,
        dispatch: Dispatch,

        const Self = @This();
        pub fn load(handle: Instance, allocator: ?*const AllocationCallbacks, loader: anytype) !Self {
            var self: Self = .{
                .handle = handle,
                .allocator = allocator,
                .dispatch = undefined,
            };
            inline for (std.meta.fields(Dispatch)) |field| {
                const name = @ptrCast([*:0]const u8, field.name ++ "\x00");
                if (loader(handle, name)) |cmd_ptr| @field(self.dispatch, field.name) = @ptrCast(field.field_type, cmd_ptr);
            }
            return self;
        }
        // Example of a function that uses the handle and allocator in the wrapper.
        pub fn destroyInstance(
            self: Self,
        ) void {
            self.dispatch.vkDestroyInstance(
                self.handle,
                self.allocator,
            );
        }
        // Other functions here...
}

I'm also considering passing success codes (besides VK_SUCCESS) as errors since usually you want to handle them like errors anyway. But that's probably more a matter of taste and I'll have to try it out for sure.

That uses GLFW for the window, and GLFW loads Vulkan regardless (if the Vulkan backend is selected).

I didn't actually know about that. In that case I understand.

@Snektron
Copy link
Owner

Also, I made some convenience changes to the wrappers that allow you to pass in the instance/device+allocator and saves it in the wrapper since AFAIK they cannot change once they are set so no point in manually passing it in each time

I think that allocators may be changed between calls, so long as you make sure the create, destroy and any related functions accept the same allocator. I'm not sure how well that is supported in drivers though.

Regarding the instance, i thought about doing that also, but there are a number of things with that. For example, it would require you to store the instance/device twice if you're making two different wrappers like i did above, although i guess you could argue that the overhead is relatively small compared to all the function pointers in that struct anyway. Initially my opinion was that passing the handle with the pointers creates this weird asymmetry where if you use multiple wrappers where one has the device and the other not like how it works with ash, but i guess it would not be an issue if invalid function pointers are simply ignored. In that case it would be the programmer's discretion to not call invalid function pointers.

In your example, i recommend setting the function pointer explicitly to undefined, so that you can catch invalid calls in debug mode:

const name = @ptrCast([*:0]const u8, field.name ++ "\x00");
@field(self.dispatch, field.name) = @ptrCast(field.field_type, loader(handle, name) orelse undefined);

I'm also considering passing success codes (besides VK_SUCCESS) as errors since usually you want to handle them like errors anyway. But that's probably more a matter of taste and I'll have to try it out for sure.

Note that Vulkan might return a success code AND a valid return value, so this is invalid. For example vkAcquireNextImageKHR might return VK_SUBOPTIMAL_KHR, in which case the pImageIndex is still set to a valid value. If it returns an error status, this is not the case, and so we can safely ignore the value and return an error code. This is the reason why they are split in the first place.

I was hoping ziglang/zig#498 would be accepted to allow a user to write const result, const status = ...; but there seem to be some problems with that proposal so i doubt something like that is going to be introduced any time soon.

@Snektron
Copy link
Owner

Snektron commented Dec 8, 2021

I have added an additional loadNoFail function to each wrapper, which continues if a particular command is not present instead of failing.

I ran into some problems like extensions that only existed on certain platforms, which I fixed by adding a whitelist parameter to the build and only loading functions not part of an extension or from an extension from the whitelist.

According to the vulkan spec calling vkGetInstanceProcAddr with the name of a function from an extension that is not enabled yields defined behavior (the result is NULL). Did you experience something else?

@leopoldek
Copy link
Author

It may be due to the way I'm gathering the functions but I get this error:

./external/vk.zig:536:82: error: container 'std.os' has no member called 'HINSTANCE'
pub const HINSTANCE = if (@hasDecl(root, "HINSTANCE")) root.HINSTANCE else std.os.HINSTANCE;
                                                                                 ^
./external/vk.zig:1960:16: note: referenced here
    hinstance: HINSTANCE,
               ^
./external/vk.zig:14830:29: note: referenced here
    const Dispatch = @Type(.{
                            ^
./external/vk.zig:14830:22: note: referenced here
    const Dispatch = @Type(.{
                     ^
./examples/example1_cube.zig:37:16: note: referenced here
    var lol: vk.InstanceWrapper = undefined;
               ^
/usr/lib/zig/std/start.zig:471:40: note: referenced here
            const result = root.main() catch |err| {

This is the diff of the vk.zig when I include win32 extensions:

124a125,126
>     CreateWin32SurfaceKHR,
>     GetPhysicalDeviceWin32PresentationSupportKHR,
139a142,143
>     AcquireWinrtDisplayNV,
>     GetWinrtDisplayNV,
154a159
>     GetPhysicalDeviceSurfacePresentModes2EXT,
296a302
>     GetMemoryWin32HandleNV,
304a311,312
>     GetMemoryWin32HandleKHR,
>     GetMemoryWin32HandlePropertiesKHR,
306a315,316
>     GetSemaphoreWin32HandleKHR,
>     ImportSemaphoreWin32HandleKHR,
308a319,320
>     GetFenceWin32HandleKHR,
>     ImportFenceWin32HandleKHR,
415a428,430
>     GetDeviceGroupSurfacePresentModes2EXT,
>     AcquireFullScreenExclusiveModeEXT,
>     ReleaseFullScreenExclusiveModeEXT,

Note that I'm not actually using any win32 functions, just referencing vk.InstanceWrapper does this. Likely due to the dispatch struct builder iterating over all functions including ones I never use, causing them to be evaluated. But if your function isn't having this problem then maybe I did something wrong.

@Snektron
Copy link
Owner

Is that still from your own fork? If so, it's because it's trying to reference some functions for windows-extensions which normally don't work under another operating system. I think this could be resolved by doing something like

const functions = instance_functions ++ (if (is_windows) windows_functions else ...);

in the upstream version, but it's not quite related to any issue i had in mind with vkGetInstanceProcAddr itself.

@evopen
Copy link

evopen commented Nov 27, 2023

I have added an additional loadNoFail function to each wrapper, which continues if a particular command is not present instead of failing.

I ran into some problems like extensions that only existed on certain platforms, which I fixed by adding a whitelist parameter to the build and only loading functions not part of an extension or from an extension from the whitelist.

According to the vulkan spec calling vkGetInstanceProcAddr with the name of a function from an extension that is not enabled yields defined behavior (the result is NULL). Did you experience something else?

Hi. If I load a function pointer of a disabled extension, the segfault happens in NVIDIA auxiliary thread not the main thread. There will be no stack trace. You can only capture this if validation layer is enabled. Regardless of use NoFail or not.

Reading symbols from /data/ssd/Dev/minnow/lib/maligog/zig-cache/o/3dfd563526d5792d5f08d65843f4c4b9/test...
(gdb) r
Starting program: /data/ssd/Dev/minnow/lib/maligog/zig-cache/o/3dfd563526d5792d5f08d65843f4c4b9/test 

This GDB supports auto-downloading debuginfo from the following URLs:
  <https://debuginfod.archlinux.org>
Enable debuginfod for this session? (y or [n]) n
Debuginfod has been disabled.
To make this setting permanent, add 'set debuginfod enabled off' to .gdbinit.
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
Test [1/1] test.create context... [New Thread 0x7fffe7f636c0 (LWP 703494)]
[New Thread 0x7fffe0cff6c0 (LWP 703496)]
[New Thread 0x7fffcffff6c0 (LWP 703497)]
[New Thread 0x7fffcf7fe6c0 (LWP 703498)]
[New Thread 0x7fffceffd6c0 (LWP 703500)]
[New Thread 0x7fffce5fc6c0 (LWP 703502)]
[New Thread 0x7fffb2bff6c0 (LWP 703503)]
[Thread 0x7fffe0cff6c0 (LWP 703496) exited]
[Thread 0x7fffb2bff6c0 (LWP 703503) exited]

Thread 6 "[vkps] Update" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffceffd6c0 (LWP 703500)]
0x00007ffff5ff035c in ?? ()
(gdb) info threads
  Id   Target Id                                            Frame 
  1    Thread 0x7ffff7cb2740 (LWP 703477) "test"            0x00007ffff7d4e26f in ?? () from /usr/lib/libc.so.6
  2    Thread 0x7fffe7f636c0 (LWP 703494) "cuda-EvtHandlr"  0x00007ffff7db7f6f in poll () from /usr/lib/libc.so.6
  4    Thread 0x7fffcffff6c0 (LWP 703497) "[vkrt] Analysis" 0x00007ffff7d3e4ae in ?? () from /usr/lib/libc.so.6
  5    Thread 0x7fffcf7fe6c0 (LWP 703498) "[vkcf] Analysis" 0x00007ffff7d3e4ae in ?? () from /usr/lib/libc.so.6
* 6    Thread 0x7fffceffd6c0 (LWP 703500) "[vkps] Update"   0x00007ffff5ff035c in ?? ()
  7    Thread 0x7fffce5fc6c0 (LWP 703502) "cuda-EvtHandlr"  0x00007ffff7db7f6f in poll () from /usr/lib/libc.so.6
(gdb) 
dhh@dhhpc ~> strings /usr/lib/libnvidia-glcore.so.545.29.02 | grep vkcf
[vkcf] Analysis
vkcfPass::TransferToSys
dhh@dhhpc ~> strings /usr/lib/libnvidia-glcore.so.545.29.02 | grep vkrt
[vkrt] Analysis
dhh@dhhpc ~> strings /usr/lib/libnvidia-glcore.so.545.29.02 | grep vkps
[vkps] Update

loadNoFail shouldn't be encourage. Like you said, it's an UB.

Separate dispatchs is a good approach. This is how rust ash crate does it.

I think it might be a good idea to create a new enhancement issue for associating an extension with the functions it provides.

@Snektron
Copy link
Owner

Snektron commented Nov 30, 2023

I think it might be a good idea to create a new enhancement issue for associating an extension with the functions it provides.

Thats a good idea, something that I probably should have looked into a while ago...

created #112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants