KhronosGroup / KhronosGroup/SPIRV-Cross

Review use of assert()?

Open
#1,631 0 comments 0 reactions 0 assignees View on GitHub
maintenance
Dominant language
GLSL
Stars
2.5k
Forks
713
Avg merge
2d 18h
Merged PRs (30d)
16

Description

So I ran into this in a Release build of MoltenVK, which catalyzed me to add `NDEBUG` to MoltenVK builds:

https://github.com/KhronosGroup/SPIRV-Cross/blob/d57ab68a219831900fa2b8a3bd529413e01f3b9f/spirv_msl.cpp#L1830-L1831

Unless I am mistaken, this appears to be an assertion that could arise from the content of the SPIR-V, rather than an error in SPIRV-Cross code logic.

Another example is likely here:

https://github.com/KhronosGroup/SPIRV-Cross/blob/d57ab68a219831900fa2b8a3bd529413e01f3b9f/spirv_msl.cpp#L10826

which is probably an indication of a mis-alignment between the incoming SPIR-V content and the level of MSL available on the runtime platform.

This got me wondering whether we should review if some of these `assert()` calls should be converted to exception throws.

The problem with `assert()`, is that it can't be caught be the calling app. I expect that `assert()` should be used only to evaluate code logic in SPIRV-Cross, and exception throws should be used to abort a shader conversion due to any user error, including environmental incompatibilities, or shader content problems, whether it be legal SPIR-V that can't be converted, or illegal SPIR-V that is incorrectly submitted by the app.

A simpler alternative might be to replace all `assert()` calls with a `SPIRV_CROSS_ASSERT()` macro, which will test the assertion condition and then call `SPIRV_CROSS_THROW()`. And if `SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS` is defined, this will all revert to an assertions anyway.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reviewing the assert() calls in spirv_msl.cpp, including the cited locations around lines 1830 and 10826, and compare them with the existing exception behavior. Determine which failures arise from conversion inputs or runtime capabilities versus internal logic, then define a consistent, testable scope for the proposed review.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.