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

Entry::new returns Err when entry point isn't found #390

Merged
merged 7 commits into from
Apr 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions ash/src/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use crate::prelude::*;
use crate::vk;
use crate::RawPtr;
use std::error::Error;
use std::ffi::CStr;
use std::fmt;
use std::mem;
use std::os::raw::c_char;
Expand Down Expand Up @@ -163,11 +164,15 @@ impl<L> EntryV1_2 for EntryCustom<L> {
}

impl<L> EntryCustom<L> {
pub fn new_custom<Load>(mut lib: L, mut load: Load) -> Self
pub fn new_custom<Load>(
mut lib: L,
mut load: Load,
) -> std::result::Result<Self, MissingEntryPoint>
where
Load: FnMut(&mut L, &::std::ffi::CStr) -> *const c_void,
{
let static_fn = vk::StaticFn::load(|name| load(&mut lib, name));
// Bypass the normal StaticFn::load so we can return an error
let static_fn = vk::StaticFn::load_checked(|name| load(&mut lib, name))?;

let entry_fn_1_0 = vk::EntryFnV1_0::load(|name| unsafe {
mem::transmute(static_fn.get_instance_proc_addr(vk::Instance::null(), name.as_ptr()))
Expand All @@ -181,13 +186,13 @@ impl<L> EntryCustom<L> {
mem::transmute(static_fn.get_instance_proc_addr(vk::Instance::null(), name.as_ptr()))
});

EntryCustom {
Ok(EntryCustom {
static_fn,
entry_fn_1_0,
entry_fn_1_1,
entry_fn_1_2,
lib,
}
})
}

#[doc = "<https://www.khronos.org/registry/vulkan/specs/1.2-extensions/man/html/vkEnumerateInstanceVersion.html>"]
Expand Down Expand Up @@ -226,3 +231,34 @@ impl<L> EntryCustom<L> {
}
}
}

impl vk::StaticFn {
pub fn load_checked<F>(mut _f: F) -> Result<Self, MissingEntryPoint>
where
F: FnMut(&::std::ffi::CStr) -> *const c_void,
{
// TODO: Make this a &'static CStr once CStr::from_bytes_with_nul_unchecked is const
static ENTRY_POINT: &[u8] = b"vkGetInstanceProcAddr\0";

Ok(vk::StaticFn {
get_instance_proc_addr: unsafe {
let cname = CStr::from_bytes_with_nul_unchecked(ENTRY_POINT);
Comment on lines +240 to +245
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's proving impossible to create a &'static CStr because the functions needed for this aren't const. To do this would require lazy_static as an additional dependency. I can either make MissingEntryPoint contain a &'static [u8], or go back to owned CString.

2 years later I'm rather curious why I didn't notice the TODO (and static vs const), as the ENTRY_POINT string literal could be inlined in the CStr::from_bytes_with_nul_unchecked() call as I had done myself in #379 just a week before this PR was opened 😬

At this point I'm assuming it doesn't matter whether to assign to a let binding or change this to const (will be more verbose with forced const CNAME: &CStr type), as the function call is hopefully inlined either way? (this does matter for public symbols though, but that's a different beast)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect all of the above to compile down the same, yeah. Might as well favor clarity.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ralith What would you consider more clear for these one-off inline named bindings, const or let?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best practice to favor let when it's applicable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I can't imagine the compiler treating a fully-const path any different no matter whether it's via a let binding, inlined completely, or assigned to a const.

let val = _f(cname);
if val.is_null() {
return Err(MissingEntryPoint);
} else {
::std::mem::transmute(val)
}
},
})
}
}

#[derive(Clone, Debug)]
pub struct MissingEntryPoint;
impl std::fmt::Display for MissingEntryPoint {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::result::Result<(), std::fmt::Error> {
write!(f, "Cannot load `vkGetInstanceProcAddr` symbol from library")
}
}
impl std::error::Error for MissingEntryPoint {}
28 changes: 23 additions & 5 deletions ash/src/entry_libloading.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::entry::EntryCustom;
use crate::entry::MissingEntryPoint;
use libloading::Library;
use std::error::Error;
use std::ffi::OsStr;
Expand All @@ -25,17 +26,32 @@ const LIB_PATH: &str = "libvulkan.dylib";
pub type Entry = EntryCustom<Arc<Library>>;

#[derive(Debug)]
pub struct LoadingError(libloading::Error);
pub enum LoadingError {
LibraryLoadFailure(libloading::Error),
MissingEntryPoint(MissingEntryPoint),
}

impl fmt::Display for LoadingError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fmt::Display::fmt(&self.0, f)
match self {
LoadingError::LibraryLoadFailure(err) => fmt::Display::fmt(err, f),
LoadingError::MissingEntryPoint(err) => fmt::Display::fmt(err, f),
}
}
}

impl Error for LoadingError {
fn source(&self) -> Option<&(dyn Error + 'static)> {
Error::source(&self.0)
Some(match self {
LoadingError::LibraryLoadFailure(err) => err,
LoadingError::MissingEntryPoint(err) => err,
})
}
}

impl From<MissingEntryPoint> for LoadingError {
fn from(err: MissingEntryPoint) -> Self {
LoadingError::MissingEntryPoint(err)
}
}

Expand Down Expand Up @@ -71,13 +87,15 @@ impl EntryCustom<Arc<Library>> {
/// `dlopen`ing native libraries is inherently unsafe. The safety guidelines
/// for [`Library::new`] and [`Library::get`] apply here.
pub unsafe fn with_library(path: impl AsRef<OsStr>) -> Result<Entry, LoadingError> {
let lib = Library::new(path).map_err(LoadingError).map(Arc::new)?;
let lib = Library::new(path)
.map_err(LoadingError::LibraryLoadFailure)
.map(Arc::new)?;

Ok(Self::new_custom(lib, |vk_lib, name| {
vk_lib
.get(name.to_bytes_with_nul())
.map(|symbol| *symbol)
.unwrap_or(ptr::null_mut())
}))
})?)
}
}