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

Rua · 2021-03-08T15:15:01Z

Fixes #387. The error is returned by StaticFn::load and then propagated by EntryCustom::new_custom and Entry::new.

Unfortunately docs are not explicitly (build-)tested as part of `--all-targets` allowing broken code to slip in as shown by ash-rs#390. Also remove the `rust` listing type which is the default, leaving only `no_run` (until the CI has a loadable Vulkan library).

MarijnS95

Cool, thanks! Not as easy as it seems, one of the changes is in a generated file and will get lost on the next regeneration attempt. I think the CI lost its ability to run the generator and error if that results in a diff, or am I misremembering?

ash/src/vk/features.rs

ash/src/entry.rs

MarijnS95 · 2021-03-08T23:16:41Z

CI now spots missed code regeneration in #393.

Rua · 2021-03-10T09:24:54Z

I reworked it a bit so only Entry does the error checking now. Unfortunately, in order to save the library name from within the closure, I had to use an owned CString because the argument to the closure is not &'static.

MarijnS95

Have you tested this change for your use-case? As it stands a failure to load any symbol is only handled after new_custom completes, meaning get_instance_proc_addr is inevitably called leading to the original panic. In other words, in its current form this PR makes no difference to the original issue you attempt to solve.

As stated before just move load to entry.rs and return a Result there, that can subsequently be forwarded in new_custom. Then you can also use a &'static str again, though holding on to the original libloading::Error (and pairing it with the symbol name) is even better.

ash/src/entry_libloading.rs

Rua · 2021-03-11T09:45:56Z

Did you mean move StaticFn to entry.rs, or just an impl block with load in it?

MarijnS95 · 2021-03-11T09:47:16Z

Did you mean move StaticFn to entry.rs, or just an impl block with load in it?

Only the impl block with fn load; you can either create a second load function and leave the existing one for what it is, or make an edgecase to not generate it here:

https://github.com/MaikKlein/ash/blob/b3a010a3154d0cd9d4943e794a0e90916da93936/generator/src/lib.rs#L2210-L2217

Rua · 2021-03-11T10:20:19Z

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.

MarijnS95 · 2021-03-11T10:25:31Z

Since the name is hardcoded (and where possible autogenerated may you want to extend this error to other places in the future) I'm not opposed to &'static str and a simple "vkGetInstanceProcAddr". A &'static [u8] with b"vkGetInstanceProcAddr\0" isn't too bad either. That is, in fact, what I had locally when implementing your feature request initially.

Rua · 2021-03-11T10:49:47Z

Ok, another attempt...

MarijnS95 · 2021-03-11T10:52:11Z

Should be good now, thanks :)

ash/src/entry.rs

* ci: Test docs in addition to `cargo t --all-targets` Unfortunately docs are not explicitly (build-)tested as part of `--all-targets` allowing broken code to slip in as shown by #390. Also remove the `rust` listing type which is the default, leaving only `no_run` (until the CI has a loadable Vulkan library). * ash: Fix errors and warnings in (now tested) documentation comments

ash/src/entry.rs

…fn-result

ash/src/entry.rs

Rua · 2021-04-18T09:26:47Z

Does anything else need to be done here?

MaikKlein · 2021-04-18T19:53:51Z

Does anything else need to be done here?

Sorry I forgot about it, looks good. LGTM

MarijnS95 · 2023-03-09T14:44:25Z

ash/src/entry.rs

+        // 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);


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)

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

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

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

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.

MarijnS95 mentioned this pull request Mar 8, 2021

ci: Test docs in addition to cargo t --all-targets #392

Merged

MarijnS95 requested changes Mar 8, 2021

View reviewed changes

ash/src/vk/features.rs Outdated Show resolved Hide resolved

ash/src/vk/features.rs Outdated Show resolved Hide resolved

ash/src/vk/features.rs Outdated Show resolved Hide resolved

ash/src/entry.rs Outdated Show resolved Hide resolved

Rua force-pushed the staticfn-result branch from 80d616c to 2082e91 Compare March 10, 2021 09:23

Rua changed the title ~~StaticFn::load returns Err when entry point isn't found~~ Entry::new returns Err when entry point isn't found Mar 10, 2021

MarijnS95 requested changes Mar 10, 2021

View reviewed changes

ash/src/entry_libloading.rs Outdated Show resolved Hide resolved

ash/src/entry_libloading.rs Outdated Show resolved Hide resolved

Check for entry point in EntryCustom::new and return error

1959612

Rua force-pushed the staticfn-result branch from 2082e91 to 1959612 Compare March 11, 2021 10:48

MarijnS95 reviewed Mar 11, 2021

View reviewed changes

ash/src/entry.rs Outdated Show resolved Hide resolved

Split off into load_checked

215f53b

Rua force-pushed the staticfn-result branch from 22beadc to 215f53b Compare March 11, 2021 11:01

Merge branch 'master' into staticfn-result

a657cd7

Merge branch 'master' into staticfn-result

3bee8af

MaikKlein reviewed Mar 20, 2021

View reviewed changes

ash/src/entry.rs Outdated Show resolved Hide resolved

Rua added 2 commits March 20, 2021 14:34

Merge branch 'master' of https://github.com/MaikKlein/ash into static…

ad22866

…fn-result

Remove string from MissingEntryPoint

d76725b

MarijnS95 requested changes Mar 20, 2021

View reviewed changes

ash/src/entry.rs Outdated Show resolved Hide resolved

Change MissingEntryPoint error message

b0e2031

MaikKlein approved these changes Apr 18, 2021

View reviewed changes

MaikKlein merged commit 90b0531 into ash-rs:master Apr 18, 2021

Rua deleted the staticfn-result branch April 30, 2021 15:18

MarijnS95 reviewed Mar 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Rua commented Mar 8, 2021

MarijnS95 left a comment

MarijnS95 commented Mar 8, 2021

Rua commented Mar 10, 2021

MarijnS95 left a comment •

edited

Loading

Rua commented Mar 11, 2021

MarijnS95 commented Mar 11, 2021

Rua commented Mar 11, 2021

MarijnS95 commented Mar 11, 2021

Rua commented Mar 11, 2021

MarijnS95 commented Mar 11, 2021

Rua commented Apr 18, 2021

MaikKlein commented Apr 18, 2021

MarijnS95 Mar 9, 2023

Ralith Mar 9, 2023

MarijnS95 Mar 9, 2023

Ralith Mar 9, 2023

MarijnS95 Mar 9, 2023

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

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

Conversation

Rua commented Mar 8, 2021

MarijnS95 left a comment

Choose a reason for hiding this comment

MarijnS95 commented Mar 8, 2021

Rua commented Mar 10, 2021

MarijnS95 left a comment • edited Loading

Choose a reason for hiding this comment

Rua commented Mar 11, 2021

MarijnS95 commented Mar 11, 2021

Rua commented Mar 11, 2021

MarijnS95 commented Mar 11, 2021

Rua commented Mar 11, 2021

MarijnS95 commented Mar 11, 2021

Rua commented Apr 18, 2021

MaikKlein commented Apr 18, 2021

MarijnS95 Mar 9, 2023

Choose a reason for hiding this comment

Ralith Mar 9, 2023

Choose a reason for hiding this comment

MarijnS95 Mar 9, 2023

Choose a reason for hiding this comment

Ralith Mar 9, 2023

Choose a reason for hiding this comment

MarijnS95 Mar 9, 2023

Choose a reason for hiding this comment

MarijnS95 left a comment •

edited

Loading