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

Fix some robustness issues in the examples #5984

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

aleino-nv
Copy link
Collaborator

@aleino-nv aleino-nv commented Jan 2, 2025

Do some robustness fixes for a few of the examples.
This fixes a hang in the raytracing samples, and fixes the test failure in the hello-world Windows/Debug config.

This helps to address #5520.

@aleino-nv aleino-nv force-pushed the aleino/example-fixes branch 4 times, most recently from 54e9f41 to c7934fd Compare January 2, 2025 13:45
@aleino-nv aleino-nv changed the title Aleino/example fixes Fix some robustness issues in the examples Jan 2, 2025
@aleino-nv aleino-nv marked this pull request as ready for review January 2, 2025 13:47
@aleino-nv aleino-nv requested a review from a team as a code owner January 2, 2025 13:47
@aleino-nv aleino-nv added the pr: non-breaking PRs without breaking changes label Jan 2, 2025
@aleino-nv aleino-nv enabled auto-merge (squash) January 2, 2025 13:48
@@ -22,6 +22,7 @@ struct HelloWorldExample : public TestBase
{
// The Vulkan functions pointers result from loading the vulkan library.
VulkanAPI vkAPI;
bool vkAPIValid = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I check this value in HelloWorldExample::~HelloWorldExample which previously called some uninitialized VK API functions in the case where VK API initialization failed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Uninitialized vk api should be nullptr, you should be able to check for the function pointer before calling it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but then we would need to check every function pointer.
This variable means that all of the pointers should be initialized so we can just check this one value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don’t understand why this is needed. If the basic Vulkan functions are not found, shouldn’t we exit already? The VkAPI class already provides some methods that returns whether or not it is successfully loaded, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is the destructor that is causing the problem, checking for null vk device handle before calling destroy should be more than enough. If device is null, just return immediately.

Copy link
Collaborator Author

@aleino-nv aleino-nv Jan 3, 2025

Choose a reason for hiding this comment

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

I still don’t understand why this is needed. If the basic Vulkan functions are not found, shouldn’t we exit already?

The destructor of the app still runs, regardless of whether initializeVulkanDevice succeeded.

On the other hand, I believe initializeVulkanDevice could successfully set some of the VK API pointers (but not others) if it doesn't make it all the way to the end and returns 0, so I don't think it's enough to just check one of the pointers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is what the current code looks like

int main(int argc, char* argv[])
{
    initDebugCallback();
    HelloWorldExample example;
    example.parseOption(argc, argv);
    return example.run(); // ...destructor still runs and uses VK API
}

int HelloWorldExample::run()
{
    RETURN_ON_FAIL(initVulkanInstanceAndDevice()); // <--- This can fail if VK API initialization fails
    RETURN_ON_FAIL(createComputePipelineFromShader());
    RETURN_ON_FAIL(createInOutBuffers());
    RETURN_ON_FAIL(dispatchCompute());
    RETURN_ON_FAIL(printComputeResults());
    return 0;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The VkAPI class already provides some methods that returns whether or not it is successfully loaded, no?

Not as far as I can tell. Do you want me to move the boolean into the VkAPI struct and set it to true just before returning 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it is the destructor that is causing the problem, checking for null vk device handle before calling destroy should be more than enough. If device is null, just return immediately.

Ok I did something like this, please have a look.

@aleino-nv aleino-nv force-pushed the aleino/example-fixes branch 2 times, most recently from 60b9318 to fcd080d Compare January 3, 2025 07:38
@aleino-nv aleino-nv requested a review from csyonghe January 3, 2025 09:15
@@ -511,15 +515,18 @@ int HelloWorldExample::printComputeResults()

HelloWorldExample::~HelloWorldExample()
{
vkAPI.vkDestroyPipeline(vkAPI.device, pipeline, nullptr);
for (int i = 0; i < 3; i++)
if (vkAPI.device != VK_NULL_HANDLE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

always prefer early return over nesting if:

if (!vkAPI.device)
   return;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

…itialized

- Check if VK API function pointers are valid before using them
- Return 0 and exit if VK initialization fails
- Enable hello-world example
- Assert that accelleration structure buffer is not nullptr
- Check if buffer creation succeeded before proceeding
 - This makes initialization not hang, but it still fails.
   Therefore, the test expectations are just updated to point to another issue.
- Enable ray-tracing tests on Windows
@aleino-nv aleino-nv force-pushed the aleino/example-fixes branch from fcd080d to 80f8ea3 Compare January 5, 2025 08:17
@aleino-nv aleino-nv requested a review from csyonghe January 5, 2025 08:25
Copy link
Collaborator

@csyonghe csyonghe left a comment

Choose a reason for hiding this comment

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

Looks good!

@aleino-nv aleino-nv merged commit 7d4142e into shader-slang:master Jan 7, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants