The way Anvil uses assertions is inconsistent and dangerous

[marti4d]

Generally, asserts are used in code for things that should never happen and can't reasonably be recovered from, where allowing the program to continue running would be invalid. Logging and calling abort() is generally recognized as a perfectly-reasonable resolution to triggering one.

But it seems like in Anvil, the use of anvil_assert() is used for both cases where reasonable recovery is not possible, but also just for general errors that need to be reported.

This is true even in the same function within a few lines of each other. Using Queue::present() as an example:

Line #227:

    /* Sanity checks */
    anvil_assert(in_n_wait_semaphores < sizeof(wait_semaphores_vk) / sizeof(wait_semaphores_vk[0]) );

This is an assert that cannot fail. If it does, code later on will cause a buffer overflow.

But then later on in the same function:

Line #284:

    result = swapchain_entrypoints.vkQueuePresentKHR(m_queue,
                                                    &image_presentation_info);

    anvil_assert_vk_call_succeeded(result);

    if (is_vk_call_successful(result) )
    {
        anvil_assert(is_vk_call_successful(presentation_results[0]));

        /* Return the most important error code reported */
        switch (presentation_results[0])
        {
    ...

It's perfectly-reasonable for that assert to fail, and for the program to handle the error and continue running. That's what's expected to happen in the case of an out-of-date swapchain.

And, in fact, the code below it even continues and handles the error as-if the assert wasn't there, so why even bother asserting?

Anvil has this problem literally everywhere, and fixing it would require a thorough review of all the places where asserts happen, properly separating the can never fail from the can fail and we'll handle it.

[DominikWitczakAMD]

Good & useful observation.

This has actually been bothering me for some time. With me having some more time to spend on Anvil right now, I'll try to get this addressed for the coming update.

[madwax]

Sorry but I have to disagree with using abort() and hears the why: 1) Anvil is a library to be used by other applciations? A 3rd party lib should never ever abort a process. At the very least provide a way to override the call to abort(). Just because something bad happend in Anvil code I might have a clean/controlled way to perform an app shutdown. 2) abort() is fine on unixs/gdb/xcode but for MSVC abort() is a bad idea as the process is going down as the debugger kicks in. To this end you should use assert/_wassert or __debugbreak which pauses the process so other threads don't die or resources released.

[marti4d]

@madwax - I have to respectfully disagree with your first point:

abort() is a perfectly reasonable thing to do in a 3rd party library as well. An assert is for errors that cannot be recovered from. This means that allowing the program to continue running is potentially undefined behavior, and that is potentially a security hazard.

If allowed to continue running, the program may do many terrible things, such as hanging in an infinite loop or overflowing the stack. That could allow for code injection attacks or privilege escalation. Allowing user code to intercept the call to abort() just means now the user has a callback where they can't be sure what the state of the system is like when they get it. How far did your library code get before the bug happened? Is there some file your library was working on that's only half-written?

If you do have some cleanup routine that's so important that it needs to be run no-matter-what, you are already screwed anyway because the library you're using has a bug in it and may be about to do something like "dereference a null pointer" or "write to a read-only memory location" that's going to abort the program anyway. So you already have to have some other way of dealing with that.

To your second point - I don't have MSVC right in front of me, but I'm pretty sure abort() in a debug build does give the debugger an interception point. If I'm wrong then I guess I agree that it's better to use the built-in MSVC assert() function.

My main reason for not straight-up suggesting it instead of abort() is that a lot of people want custom assert messages or to do a little something more (like logging the failure) instead of just aborting with the default error message. I guess you could accomplish the same thing by writing your own assert function and ending it with ::assert(false);

[madwax]

@marti4d - I (and a number of colleagues) will just have to disagree with you in that case.

1) I have a big problem with using asserts from a deign point of view. A 3rd party library should check it's inputs first and prevent an unrecoverable condition from happening in the first place, that's why we have exceptions in C++ or error codes.
2) I don't also agree with you definition of assert means. Assert() is there to aid development allowing inputs/conditions/outputs to be tested. This is why they should be compiled out in release builds however people don't. 3) While I agree most people don't handle terminal situations properly but that's on them. I know that the app I'm thinking about using Anvil in does have safe dirty shutdown code path that does stuff like flushing log files. 4) If your app is a Win32 when you call abort() it just disappears. Having been faced with very large and complex applications that have been released all of a sudden "disappearing" without any information in a log file (or message) is a support nightmare. I have been down that road to meany times.

I offer regarding point 1) : https://www.softwariness.com/articles/assertions-in-cpp/

[marti4d]

@madwax - I could also point you to several articles that state the opposite - Including "The C++ Programming Language" by Bjarne Stroustrup, "Code Complete" by Steve McConnell, and "C++ coding standards: 101 rules, guidelines, and best practices" by Herb Sutter and Andrei Alexandrescu.

Here is a link that includes cited sources to all those books and the respective section it's discussed.

For the record though, let me be clear about what I'm saying about asserts. I'm not saying you should be using assert() instead of doing proper checking of inputs/outputs and reporting errors via return codes or exceptions. Not at all. If it's something you can handle, report, and recover from, then do it.

What I'm saying is that asserts are for logic errors - Bugs in your program that represent conditions that should never be true by-design, or else you've hit some undefined state that your testing didn't catch, and that you can't design your program to handle.

Example - Your class has some member variable it uses to index into an array. Class invariant states that the index should always be valid. But through some program bug (maybe you forgot to decrement it in a weird corner case), it now points off the end of the array.

What should you do if you detect that error in a later function call? There's nothing you or the user can reasonably do to recover from it. The library is literally in a state it never thought it should be in.

I hear you about to say, "Well then throw an exception! This is C++ after all!" Not so fast. Your class is in an invalid state. If you throw, its destructor will get called, and so will the destructors of other classes that may rely on it. Who knows what the destructors will do given the library is in some state it never expected to exist. The dtor may use that invalid index in some for-loop to try and free stuff, at which point it will run off the end of the array.

This is why all those authors are saying that the best thing you can do is throw your hands up in the air and go, "I should just stop running - Anything I do may potentially make the situation worse."

[madwax]

@marti4d - I know the theory and don't disagree with it however my opion is based on passed experiances of others and myself (been at this for +20 years, now I feel old!) working on some very large projects. sorry as for using exceptions (I personlly don't like them tbh) that's why we have them, to inform us of out of bounds conditions plus I think it's bad-design not to make safe an object that throws an exception if it is to be reused.

In the end this comes down into one of those holy war issues we all say we hate but secretly love ;) but I'm shutting up now.