vkd3d: Allow to configure the behavior on assertion failures.
I converted a couple of files to the new implementation so that we can have some discussion about it; the idea, if this gets accepted, is to start using the new implementation everywhere.
Merge request reports
Activity
mentioned in merge request !873 (closed)
53 # define VKD3D_ASSERT(cond) \ 54 do { \ 55 if (!(cond)) \ 56 { \ 57 ERR("Failed assertion: %s\n", #cond); \ 58 abort(); \ 59 } \ 60 } while (0) 61 # else 62 # define VKD3D_ASSERT(cond) \ 63 do { \ 64 if (!(cond)) \ 65 ERR("Failed assertion: %s\n", #cond); \ 66 } while (0) 67 # endif 68 #endif - Comment on lines +49 to +68
Or use
elif defined
:#ifdef VKD3D_IGNORE_ASSERTIONS # define VKD3D_ASSERT(cond) #elif defined VKD3D_ABORT_ON_ASSERTIONS # define VKD3D_ASSERT(cond) \ do { \ if (!(cond)) \ { \ ERR("Failed assertion: %s\n", #cond); \ abort(); \ } \ } while (0) #else # define VKD3D_ASSERT(cond) \ do { \ if (!(cond)) \ ERR("Failed assertion: %s\n", #cond); \ } while (0) #endif
changed this line in version 9 of the diff
I converted a couple of files to the new implementation so that we can have some discussion about it;
Unfortunately it doesn't look like that discussion really happened. Let's see if I can get something going then...
Here I try to reach for a common ground; a new assertion implementation is added with three configurable behaviors: doing nothing, dropping the assertion entirely (similar to the standard NDEBUG); emitting an error and aborting (similar the the standard behavior without NDEBUG); and emitting an error without aborting, with this last behavior being the default.
It seems like we could get all of those by optionally aborting on ERRs. (And for what it's worth, I've used something along those lines as a patch for running Wine's D3D tests back in the day.)
It seems like we could get all of those by optionally aborting on ERRs. (And for what it's worth, I've used something along those lines as a patch for running Wine's D3D tests back in the day.)
Maybe. I'm not sure I'd want to abort on any
ERR()
, even while willing to abort on failed assertions, but I can try.
@@ -116,6 +116,11 @@ void vkd3d_dbg_printf(enum vkd3d_dbg_level level, const char *function, const ch va_start(args, fmt); vkd3d_dbg_voutput(fmt, args); va_end(args); + +#ifdef VKD3D_ABORT_ON_ERR + if (level == VKD3D_DBG_LEVEL_ERR) + abort(); +#endif } void vkd3d_dbg_set_log_callback(PFN_vkd3d_log callback)
That's pretty much what I had in mind, yes.
It's a bit unfortunate that this causes the CI to abort on the vkd3d_glsl_generator_generate() unsupported message, but arguably we shouldn't be using ERR for those in the first place, and at least we know it's working as intended now. :)
The latest push should fix the CI pipeline, but I don't think it's indeed an improvement. I had to downgrade several
ERR()
's toWARN()
's, and I don't think that's good idea for a fair amount of them. Conceptually I don't think it's appropriate to conflateERR()
's and (failed) assertions: from my point of view anERR()
might simply be an indication that the user is doing something very strange or that the environment is broken to the point that there's no way to service the user's request, but it doesn't mean that our internal state is corrupted. For example it makes sense to emitERR()
if we can't create a Vulkan device, because that's something that really prevents us from doing anything good, and it's a condition that a developer would like to immediately know when reading a log; but we can carry on doing work: if the user tries again to create a device and this time it works (say because they removed some requires user extensions) everything is fine again. That's not the case after a failed assertion, which means that we're not even able to guarantee about our internal state any more.So I'd either propose to introduce a severity level higher than
ERR()
and use that to signal assertions (and optionally allow to abort), or to keep usingERR()
for failed assertions, but only abort (if requested) on failed assertions, not on anyERR()
. Any opinion?added 106 commits
-
388544df...ccb6150a - 103 commits from branch
wine:master
- fdb40c59 - vkd3d: Introduce a softer form of assertion.
- fbcb5efe - vkd3d: Allow aborting on ERR().
- 5e81e31d - ci: Abort on assertions on the CI.
Toggle commit list-
388544df...ccb6150a - 103 commits from branch
The latest push should fix the CI pipeline, but I don't think it's indeed an improvement. I had to downgrade several
ERR()
's toWARN()
's, and I don't think that's good idea for a fair amount of them.I disagree; I'd take most of those changes independent of the MR. The exceptions would be the ones in dxil_parse() and vkd3d_glsl_generator_generate(); we probably want to do something else for those, perhaps something like Wine's MESSAGE.
Conceptually I don't think it's appropriate to conflate
ERR()
's and (failed) assertions: from my point of view anERR()
might simply be an indication that the user is doing something very strange or that the environment is broken to the point that there's no way to service the user's request, but it doesn't mean that our internal state is corrupted.It may not always be how these are used in practice, but the intention is very much for ERR to indicate an internal error or an otherwise unrecoverable state. Incorrect (vkd3d) API usage would be WARN. If anything, aborting on ERR on the CI seems like a good way to prevent people from abusing ERR...
For example it makes sense to emit
ERR()
if we can't create a Vulkan device, because that's something that really prevents us from doing anything good, and it's a condition that a developer would like to immediately know when reading a log; but we can carry on doing work: if the user tries again to create a device and this time it works (say because they removed some requires user extensions) everything is fine again. That's not the case after a failed assertion, which means that we're not even able to guarantee about our internal state any more.It seems entirely legitimate to probe for features by attempting to create a device with some features and seeing whether it succeeds or not; I don't think that warrants an ERR, and you could even make a case that TRACE would be more appropriate than WARN.
Ok, if that's ok for you it's ok for me too. Should I convert
assert()
toVKD3D_ASSERT()
already or in later MRs? Anything else to do for this MR?I'm tempted to suggest converting them to explicit ERRs, but won't insist. Do we need VKD3D_ENABLE_ASSERTIONS?
I think the main thing this MR still needs is some kind of solution of dxil_parse() and vkd3d_glsl_generator_generate(). I think in principle VKD3D_DBG_LOG(NONE) will do the right thing, but haven't tested that.
I understand the idea behind this view of ERR vs WARN, and I appreciate its consistency, but I fear we're also defeating ourself by making it harder to notice interesting failures.
Perhaps that's just a sign that things like missing features should be FIXME instead?
Well, they shouldn't use ERR in any case. If you're talking about missing vkd3d features, FIXME seems entirely appropriate; if you're referring to missing features in the setup/environment, FIXME may not quite right. Wine would use the winediag debug channel for that, so that you can do something like WINEDEBUG=-all,+winediag and only get those messages. We haven't had a strong need for different debug channels in vkd3d so far, but perhaps it's time to add them.
I'm tempted to suggest converting them to explicit ERRs, but won't insist.
Currently
VKD3D_ASSERT()
is basicallyif (!cond) ERR("cond")
, so that you don't have to write theif
and the error message explicitly. The idea is to make as easy as possible to add assertions, without having to write elaborate messages that are probably not particularly useful.Do we need VKD3D_ENABLE_ASSERTIONS?
The idea is the same as
NDEBUG
: assertions are only assumed to be useful for development, so they can be stripped away from production builds to make the code smaller and quicker. I can enable them inconditionally if that's better for you.I think the main thing this MR still needs is some kind of solution of dxil_parse() and vkd3d_glsl_generator_generate(). I think in principle VKD3D_DBG_LOG(NONE) will do the right thing, but haven't tested that.
I tried to introduce something like Wine's
MESSAGE()
.
added 95 commits
-
c7742a55...7eb63a7c - 91 commits from branch
wine:master
- 1e3ad69f - vkd3d: Introduce debug severity MESSAGE.
- 4ee8b8d9 - vkd3d: Introduce a softer form of assertion.
- 4395a8c7 - vkd3d: Allow aborting on ERR().
- 13c427d4 - ci: Abort on assertions on the CI.
Toggle commit list-
c7742a55...7eb63a7c - 91 commits from branch
Do we need VKD3D_ENABLE_ASSERTIONS?
The idea is the same as
NDEBUG
: assertions are only assumed to be useful for development, so they can be stripped away from production builds to make the code smaller and quicker. I can enable them inconditionally if that's better for you.We have VKD3D_NO_TRACE_MESSAGES to strip out TRACE, and VKD3D_NO_DEBUG_MESSAGES to strip out WARN and FIXME. The reasoning for not disabling ERR as well was that you'd pretty much always want to see ERRs, but we could reconsider that. If we were to introduce something like that though, I think VKD3D_ENABLE_ASSERTIONS would no longer add much. So I think we have a few options:
- VKD3D_ENABLE_ASSERTIONS, as in this MR.
- The status quo. This is always an easy option, of course.
- Disable VKD3D_ASSERT and possibly ERR when NDEBUG is defined.
- Introduce something like VKD3D_NO_ERROR_MESSAGES, and possibly disable VKD3D_ASSERT when it's defined.
Introduce something like VKD3D_NO_ERROR_MESSAGES, and possibly disable VKD3D_ASSERT when it's defined.
Ok, I did this last point. Given that now we have a number of knob to tune what
ERR()
is supposed to do, I also rewrotevkd3d_unreachable()
in terms of that. Essentially,vkd3d_unreachable()
just callsERR()
with a standard message, but it makes it unconditionally unreachable, according to its original goal to avoid warnings caused, e.g., by not returning a value on the unreachable branch.
mentioned in merge request !936 (merged)
added 20 commits
-
13c427d4...947b937a - 16 commits from branch
wine:master
- dda3d8f2 - vkd3d: Introduce debug severity MESSAGE.
- 5c5c507f - vkd3d: Introduce a softer form of assertion.
- 27af09b3 - vkd3d: Allow aborting on ERR().
- af6ebf0d - ci: Abort on assertions on the CI.
Toggle commit list-
13c427d4...947b937a - 16 commits from branch
added 8 commits
- 3255e8a8 - vkd3d: Disable WARN_ON() when VKD3D_NO_DEBUG_MESSAGES is defined.
- 50497723 - vkd3d: Disable FIXME_ONCE() when VKD3D_NO_DEBUG_MESSAGES is defined.
- 24edd9f5 - vkd3d: Allow disabling ERR() by defining VKD3D_NO_ERROR_MESSAGES.
- 12ae4cdb - vkd3d: Introduce debug severity MESSAGE.
- 05aded49 - vkd3d: Introduce a softer form of assertion.
- eb6bfc3e - vkd3d: Allow aborting on ERR().
- 658d59c9 - vkd3d: Emit an ERR() when reaching unreachable code.
- 9ef35ca2 - ci: Abort on assertions on the CI.
Toggle commit list