Skip to content
Snippets Groups Projects

vkd3d: Allow to configure the behavior on assertion failures.

Merged Giovanni Mascellani requested to merge giomasce/vkd3d:amiata into master

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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
  • added 2 commits

    • d0bb261b - vkd3d: Allow to configure the behavior on assertion failures.
    • 0f3c3873 - ci: Crash on assertions on the CI.

    Compare with previous version

  • Conor McCarthy approved this merge request

    approved this merge request

    • 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.

    • Please register or sign in to reply
  • added 3 commits

    • bf7cdbc0 - vkd3d: Introduce a softer form of assertion.
    • b136bc77 - vkd3d: Allow aborting on ERR().
    • 244b47c4 - ci: Abort on assertions on the CI.

    Compare with previous version

  • @@ -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. :)

  • added 1 commit

    • 388544df - ci: Abort on assertions on the CI.

    Compare with previous version

  • 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 to WARN()'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 conflate ERR()'s and (failed) assertions: from my point of view an ERR() 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 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.

    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 using ERR() for failed assertions, but only abort (if requested) on failed assertions, not on any ERR(). Any opinion?

  • added 106 commits

    Compare with previous version

    • 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 to WARN()'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 an ERR() 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() to VKD3D_ASSERT() already or in later MRs? Anything else to do for this MR?

    • Please register or sign in to reply
  • 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?

    • Ok, if that's ok for you it's ok for me too. Should I convert assert() to VKD3D_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 basically if (!cond) ERR("cond"), so that you don't have to write the if 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().

    • Please register or sign in to reply
  • added 4 commits

    • 4a1fa94f - vkd3d: Introduce debug severity MESSAGE.
    • b4c9d186 - vkd3d: Introduce a softer form of assertion.
    • 40db50bb - vkd3d: Allow aborting on ERR().
    • c7742a55 - ci: Abort on assertions on the CI.

    Compare with previous version

  • 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.

    Compare with previous version

    • 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 rewrote vkd3d_unreachable() in terms of that. Essentially, vkd3d_unreachable() just calls ERR() 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.

    • Please register or sign in to reply
  • Francisco Casas mentioned in merge request !936 (merged)

    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.

    Compare with previous version

  • 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.

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading