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

glfw: Vulkan loading base handle panics in release #99

Closed
Avokadoen opened this issue Nov 26, 2021 · 10 comments
Closed

glfw: Vulkan loading base handle panics in release #99

Avokadoen opened this issue Nov 26, 2021 · 10 comments
Labels
Milestone

Comments

@Avokadoen
Copy link
Contributor

Avokadoen commented Nov 26, 2021

Inital issue was filed in the vulkan example as I though it was a bug in that code and can be found here

The following will panic in release (but not in debug):

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

replacing the following code in mach-glfw resolves the issue:
vulkan.zig

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

with

pub const getInstanceProcAddress = c.glfwGetInstanceProcAddress;
@Avokadoen
Copy link
Contributor Author

Avokadoen commented Nov 26, 2021

Since the mach wrapper does not really add any direct improvements, I vote for it to simply forward export of the function instead of having its own wrapper implementation. The only issue is that getError() will pollute other functions. A wrapper implementation could be introduced if this shows to be a problem

Avokadoen added a commit to Avokadoen/mach that referenced this issue Nov 26, 2021
The previous code cause a panic in release builds.
This commit resolved that issue at the cost of leaking glfw errors.

Resolves issue hexops#99
@InKryption
Copy link
Contributor

Which error(s) does it panic with? Is it 'ApiUnavailable'?

@Avokadoen Avokadoen changed the title glfw: vulkan loading base handle crashes in release glfw: Vulkan loading base handle crashes in release Nov 26, 2021
@Avokadoen
Copy link
Contributor Author

Avokadoen commented Nov 26, 2021

Which error(s) does it panic with? Is it 'ApiUnavailable'?

Included in the original issue: thread 1030138 panic: reached unreachable code

@Avokadoen Avokadoen changed the title glfw: Vulkan loading base handle crashes in release glfw: Vulkan loading base handle panics in release Nov 26, 2021
@InKryption
Copy link
Contributor

It's definitely a weird issue; your fix does stop the panic from happening, but removing the 'getError' call and catch also stops it. As well, when I run it with gdb, without the fix, it seems to panic before even calling root.main, instead panicking in 'start.zig', while measuring the c_envp parameter of start.main. On Windows 10 at least.

@Avokadoen
Copy link
Contributor Author

Indeed, then we know the issue is cross platform as i tested on ubuntu 20.04.

I'm am having issues other places in my code on release, so I am starting to suspect there might be some weird miss compilations in zig master.

@InKryption
Copy link
Contributor

InKryption commented Nov 26, 2021

After flipping a bunch of knobs on and off, I've discovered what I believe to be the culprit: in 'graphics_context.zig', inside the function GraphicsContext.init, the line of code:

const vk_proc = @ptrCast(fn(instance: vk.Instance, procname: [*:0]const u8) vk.PfnVoidFunction, glfw.getInstanceProcAddress);

It would seem that the issue here is to do with calling convention. By changing the line of code to:

const vk_proc = @ptrCast(fn(instance: vk.Instance, procname: [*:0]const u8) callconv(vk.vulkan_call_conv) vk.PfnVoidFunction, glfw.getInstanceProcAddress);

The panic no longer occurs, so it is very probable that the issue here is how the compiler is deciding to call the function that is calling all the weirdness.

Edit: in fact, this is probably even better:

const vk_proc = @ptrCast(vk.PfnGetInstanceProcAddr, glfw.getInstanceProcAddress);

@Avokadoen
Copy link
Contributor Author

Tested and works. Good job! I'll close this issue, but keep the issue on the vulkan project.

@slimsag
Copy link
Member

slimsag commented Nov 26, 2021

Thanks for reporting this and debugging! You are definitely right about the pointer cast missing a calling convention being the culprit here.

However, I want to call out that using vk.vulkan_call_conv is not right in this case. You should use callconv(.C) I believe as this is a GLFW C proc, not a Vulkan proc, as I described previously in #49 (comment)

Unfortunately, vulkan-zig does not declare vk.PfnGetInstanceProcAddr with the correct calling convention either, I have filed an issue: Snektron/vulkan-zig#30

The most correct change I see, aside from fixing that issue in zig-vulkan, is the following:

-const vk_proc = @ptrCast(fn(instance: vk.Instance, procname: [*:0]const u8) vk.PfnVoidFunction, glfw.getInstanceProcAddress);
+const vk_proc = @ptrCast(fn(instance: vk.Instance, procname: [*:0]const u8) callconv(.C) vk.PfnVoidFunction, glfw.getInstanceProcAddress);

@InKryption
Copy link
Contributor

Good point, I hadn't thought about that, and it just happened to work so my brain decided that "welp, our work here is done".

@slimsag slimsag closed this as completed Nov 26, 2021
@Snektron
Copy link

Snektron commented Nov 27, 2021

pub fn getInstanceProcAddress(vk_instance: ?*opaque {}, proc_name: [*:0]const u8) callconv(.C) ?VKProc {

const vk_proc = @ptrCast(fn(instance: vk.Instance, procname: [*:0]const u8) vk.PfnVoidFunction, glfw.getInstanceProcAddress);

There is a calling convention mismatch here, getInstanceProcAddress is defined as having the C calling convention, while it's cast to a function pointer with unspecified calling convention. Note that the load function for Wrappers does not require any specific calling convention, specifically to allow you to load using functions that do not necessarily follow the Vulkan calling convention. This means that

- pub fn getInstanceProcAddress(vk_instance: ?*opaque {}, proc_name: [*:0]const u8) callconv(.C) ?VKProc {
+ pub fn getInstanceProcAddress(vk_instance: ?*opaque {}, proc_name: [*:0]const u8) ?VKProc {

and

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

should also resolve the problem.

@slimsag slimsag added this to the Mach 0.1 milestone Mar 19, 2022
@slimsag slimsag added the glfw label Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants