Policy for null pointers returned by drivers? #484
Comments
|
What driver is this? Are you certain that it advertises support for the extension? |
I haven't confirmed it myself. cc @cwfitzgerald, who was investigating this on Discord. Here is how the initialization in wgpu goes:
|
|
Thanks, wgpu's behavior looks sane to me. Debug utils are often implemented by layers, so I wonder if there might be a janky layer involved. Overall, I'm reluctant to support ash going out of its way to defend against things that are expressly forbidden by the spec. I think this should be reported upstream, provided a driver update doesn't fix the user's problem. A mitigation could be applied at a higher layer by allowing the user to explicitly disable the debug extension (perhaps this is already supported?) and/or by blacklisting it for the driver. If we add extra checking/workarounds at the ash API level every time we see a driver bug, we might end up with arbitrarily large amounts of cruft, with little ability to remove it. |
|
If we can work around this on our side, that's an acceptable outcome. The function we call - |
|
Reviewing the specific implementation of Are you certain that's what's happening? If so, can you trace where the pointer is becoming null? |
|
I've also run into this issue when running to install Vulkan drivers and then confirming that cmd_begin_debug_utils_label_ext: unsafe {
unsafe extern "system" fn cmd_begin_debug_utils_label_ext(
_command_buffer: CommandBuffer,
_p_label_info: *const DebugUtilsLabelEXT,
) {
panic!(concat!(
"Unable to load ",
stringify!(cmd_begin_debug_utils_label_ext)
))
}
let cname = ::std::ffi::CStr::from_bytes_with_nul_unchecked(
b"vkCmdBeginDebugUtilsLabelEXT\0",
);
let val = _f(cname);
// right here, specifically
if val.is_null() {
cmd_begin_debug_utils_label_ext
} else {
::std::mem::transmute(val)
}
}
libvulkan.so.1`___lldb_unnamed_symbol1120:
0x7ffff4f1aaa0 <+0>: movq (%rdi), %rax
0x7ffff4f1aaa3 <+3>: jmpq *0x640(%rax)
0x7ffff4f1aaa9 <+9>: nopl (%rax)I found out that when the program reaches this code, pub unsafe fn cmd_begin_debug_utils_label_ext(
&self,
command_buffer: CommandBuffer,
p_label_info: *const DebugUtilsLabelEXT,
) {
// right here
(self.cmd_begin_debug_utils_label_ext)(command_buffer, p_label_info)
}the disassembly (and next instruction, marked by an arrow) is: boids`cmd_begin_debug_utils_label_ext:
0x5555561bed90 <+0>: subq $0x28, %rsp
0x5555561bed94 <+4>: movq %rdx, (%rsp)
0x5555561bed98 <+8>: movq %rsi, %rax
0x5555561bed9b <+11>: movq (%rsp), %rsi
0x5555561bed9f <+15>: movq %rax, 0x8(%rsp)
0x5555561beda4 <+20>: movq %rdi, %rax
0x5555561beda7 <+23>: movq 0x8(%rsp), %rdi
0x5555561bedac <+28>: movq %rax, 0x10(%rsp)
0x5555561bedb1 <+33>: movq %rdi, 0x18(%rsp)
0x5555561bedb6 <+38>: movq %rsi, 0x20(%rsp)
-> 0x5555561bedbb <+43>: callq *0x28(%rax)
0x5555561bedbe <+46>: addq $0x28, %rsp
0x5555561bedc2 <+50>: retq and the registers are: Reading from The pointer at Running the |
|
@kvark has this "driver returns nullpointer" issue been resolved? @josh65536 that seems to be a different issue, where the driver function is indeed non-NULL but the internal implementation/state is botched somehow; seems we've got a separate report of "something similar" in #700, but yet to be confirmed with more detailed dumps/stacktraces. |
|
Closing this after not receiving any followup, there appears to be nothing actionable for |
Following up to gfx-rs/wgpu#2091 (comment)
TL;DR:
ExtDebugUtilsFn::cmd_begin_debug_utils_label_extis NULLSomething needs to check for NULL. It seems to me that Ash is in a better position to do this.
One way to proceed would be to return an Error from
DebugUtils::newif any of the pointers are NULL. This is unfortunate because it's an API change, and in order to be consistent with the rest all initialization would have to return a result.Another way is to add something like
fn new_falliblespecifically toDebugUtilsto let clients work around bad drivers.The text was updated successfully, but these errors were encountered: