Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Logic error handling in SFML  (Read 2901 times)

0 Members and 1 Guest are viewing this topic.

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Logic error handling in SFML
« on: November 29, 2015, 07:53:58 pm »
Since the topic reoccurs at every second pull request, it's time to discuss a library-wide best practice to handle logic errors, rather than a different one for every use case.

First, it is crucial to get the terminology right. Logic errors do not appear in correct programs. They are developer mistakes, such as out-of-bound access or division by zero. These can be prevented and are fundamentally different from runtime errors such as resource not found, which developers have to be prepared for. There are border cases, for example when user input is directly forwarded to the library, or when the error check can easily be done by the library itself and is expensive outside. But let's focus on the unambiguous cases for now.

Second, we're considering SFML 2 here, so exceptions are out of question.

There are several aspects I consider fundamental in the treatment of logic errors.
  • The handling of logic errors should not affect the semantics or the flow of the program. Since logic errors are bugs, they cannot be handled meaningfully at runtime. Their presence indicates a mistake in the program logic, and further execution of the program is unsafe, as it is based on erroneous code.
  • So why even bother at all? The idea is to prevent common mistakes and help the developer find them more easily. It's thus a pure quality-of-implementation (QoI) feature and not part of the API. As such, the extent to which assisting measures are provided is left to the implementation and need not be documented.
  • We should not punish developers who know how to write their code correctly. In particular, the goal is to have zero performance or memory overhead in Release mode.
  • Logic errors must not propagate through the program at all costs. The most difficult-to-track bugs are those appearing in a program part completely unrelated to the error source. Consequently, we should report logic errors as soon as possible. Ideally, running debuggers are interrupted immediately.
  • Logic errors must not be hidden at all costs. Related to the last point, a library should not aim to "fix" a user's errors by making assumptions on what he might have meant. An API exists to be used correctly, and mistakes should be reported rather than hidden, to make users aware and explicitly correct them.
C++ offers a feature that is tailored precisely to address all those points: Assertions. In my opinion, SFML should make use of them where appropriate.

The difficult part is to not overuse them and to avoid cluttering the code with loads of precondition checks. I think we should focus on mistakes that are 1. likely to happen and 2. not already caught anyway (e.g. by underlying STL data structures), to achieve a good trade-off between code clarity and error checks.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

Foaly

  • Sr. Member
  • ****
  • Posts: 453
    • View Profile
Re: Logic error handling in SFML
« Reply #1 on: November 29, 2015, 10:59:05 pm »
Since I was responsible for some of these pull requests, I guess I should answer something to this.
I come from a background where it is desired to keep the programs running for as long as possible, despite maybe some wrong input. I now come to realize that from a library standpoint this might not be what you want.
I think using assertions to prevent logic errors by showing library users, that they are doing something wrong is a great idea.
If you are looking for a way to interrupt the debugger immediatly I suggest a custom assert implementation. You could emit a interrupt 3 which triggers a breakpoint. A way of doing this is shown here. A custom assert also brings the possiblity of printing a message explaining how to fix the problem. This can be benifitial in cases where the fix is not trivial on first sight. (For example the current audio capture threading issue, where you have to call stop() in the destructor of the deriving class) Some thing like this.
Overall I think it is a good idea to use more asserts to improve the usability of SFML.
« Last Edit: November 29, 2015, 11:51:10 pm by Foaly »

FRex

  • Hero Member
  • *****
  • Posts: 1843
    • View Profile
    • My GitHub Page
    • Email
Re: Logic error handling in SFML
« Reply #2 on: November 30, 2015, 12:43:36 am »
There exists a library to trigger breakpoints: https://github.com/scottt/debugbreak
ZipSavings, script to count rar/7z/zip savings: https://goo.gl/vvBj5M
LuaConsole: https://goo.gl/X4kRUk
FoxRaycaster: https://goo.gl/27nVS8
Small Games - Heart, Routing and Snek: https://goo.gl/15ZGWE https://goo.gl/k5gwWD https://goo.gl/4nKPnT
Botes - a notes app in Pascal: https://goo.gl/bzTqsi

Hiura

  • SFML Team
  • Hero Member
  • *****
  • Posts: 4328
    • View Profile
    • Email
Re: Logic error handling in SFML
« Reply #3 on: November 30, 2015, 10:57:44 am »
C++ offers a feature that is tailored precisely to address all those points: Assertions. In my opinion, SFML should make use of them where appropriate.

Sounds good to me.


There exists a library to trigger breakpoints: https://github.com/scottt/debugbreak
It would be best if we don't have additional external deps I believe.

If you are looking for a way to interrupt the debugger immediatly I suggest a custom assert implementation. You could emit a interrupt 3 which triggers a breakpoint.
Actually, a good tool suite (i.e. compiler + debugger) doesn't need that. Clang+lldb will break on `assert(false)` for example. Well, right afterwards but the assertion already brings a lot of information about the error (stack trace and so on) so that you/we should be able to reproduce the issue and debug it further if needed. No need to further instrument the assertion I'd say.
SFML / OS X developer

Jesper Juhl

  • Hero Member
  • *****
  • Posts: 1405
    • View Profile
    • Email
Re: Logic error handling in SFML
« Reply #4 on: November 30, 2015, 09:29:34 pm »
Alternatively, one can use "raise(SIGTRAP)".

Jabberwocky

  • Full Member
  • ***
  • Posts: 157
    • View Profile
Re: Logic error handling in SFML
« Reply #5 on: December 01, 2015, 08:25:46 am »
Thumbs up on assertions.

All dev environments I've used can be configured to automatically break on assert, so in my experience no additional code is required there.

The only downside with asserts are that they're obviously of no use unless they go off on the developer's computer, in a debug build.

What I'll propose here might seem like overkill to some.  But it has been invaluable for me in the past dealing with errors on other user's computers.  I figured I'd suggest it and let you folks decide if it's useful.

My preference is to define an ASSERT macro which asserts and logs once.

In SFML's case, the error would be sent to the sf::err stream instead of a logfile.  But the discussion below applies regardless, since I imagine many SFML applications redirect the SFML error stream to a log file.

With logging, you can diagnose the logic error on a remote user's computer in release mode by examining the log file.  But it's important to only log once, so that if an assert goes off in a per-frame loop (or worse), you don't create a gazigabyte logfile with the error spammed over and over.

My ASSERT macro looks something like this:

#define ASSERT(x) \
   do { if (!(x)) { if ( LogAssert( __FILE__, __LINE__ ) ) assert(x); } } while( 0 );

 

Here's a good discussion about why all that extra do/if/while stuff is needed:
http://cnicholson.net/2009/02/stupid-c-tricks-adventures-in-assert/

My LogAssert function is pretty complicated, but here's a simplified version which captures the essence.  It uses a data member to track previous asserts as follows:

   // A set of (file name, line number) of previously triggered asserts
   std::set< std::pair<std::string, int> > m_setPreviousAssert;


bool LogAssert( const char* const i_szFile, int i_nLine )
{
   std::pair< std::string, int > pairAssertInfo( i_szFile, i_nLine );

   if ( m_setPreviousAssert.count(pairAssertInfo) == 0 )
   {
      // First trigger of this assert
      // Track and log.
      m_setPreviousAssert.insert(pairAssertInfo);
      std::stringstream ssMsg;
      ssMsg << "[ASSERT] " << i_szFile << ":" << i_nLine;
      Log( ssMsg.str() );
      return true;
   }
   return false;
}
 

This achieves logging all asserts once, and provides a file and line number in the log file for the assert.

« Last Edit: December 01, 2015, 08:35:03 am by Jabberwocky »

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Re: Logic error handling in SFML
« Reply #6 on: December 01, 2015, 09:48:25 am »
Yes, the idea of assert is exactly to stop debuggers. I don't think it makes sense to build a non-portable solution unless there's an actually proven need for it. And such things are always possible to add in the future, the important point here is to add error checks at all, and to have a defined and uniform way of handling logic errors.

Quote from: Jabberwocky
What I'll propose here might seem like overkill to some.  But it has been invaluable for me in the past dealing with errors on other user's computers.  I figured I'd suggest it and let you folks decide if it's useful.
What I like at assert is that you can perform a lot of smart error checks at potentially high performance costs (the STL checked iterators are a perfect example of how valuable this can be). See my point 3.

With your approach, this is no longer possible -- we always have to ask the question "is this error check worth the wasted performance?" because Release mode bears it, too. And this is problematic, because it tends to make error checks less frequent, essentially decreasing safety.

And what I have not even considered is all other sorts of problems that your book-keeping set brings: global lifetime and destruction order, thread safety, ...
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

Jabberwocky

  • Full Member
  • ***
  • Posts: 157
    • View Profile
Re: Logic error handling in SFML
« Reply #7 on: December 01, 2015, 11:26:29 am »
What I like at assert is that you can perform a lot of smart error checks at potentially high performance costs (the STL checked iterators are a perfect example of how valuable this can be). See my point 3.

With your approach, this is no longer possible -- we always have to ask the question "is this error check worth the wasted performance?" because Release mode bears it, too. And this is problematic, because it tends to make error checks less frequent, essentially decreasing safety.

Perhaps you use asserts differently than I do.  Most often, my asserts are simple bounds checks, pointer checks, or other very simple verifications.  Stuff that has negligible impact on performance.  I rarely see much beyond that in most code.

True, the std lib does some crazy assert checking.  But that kind of code rarely exists outside of the STL.  I did a quick search for "assert" in the SFML code base.  There's not much there.  Everything in SFML so far would absolutely be negligible.

Remember, under non-error conditions, the set query, and logging/error stream part will never occur.  So it's only the assertion check itself. 

Under error conditions, the performance hit is likely acceptable in order to detect and fix the problem.  The application is malfunctioning anyway, perhaps even about to crash, so perf doesn't matter much

What you're gaining is potentially huge.  A bare assert is fine for catching any common case errors.  But for anything rare that is perhaps only triggered outside of the development environment (e.g. hardware related, or based on an end-user's particular use of the software), the bare assert does nothing.  That to me is a huge safety risk.  A risk that should be weighed against what is likely a negligible perf hit.

And what I have not even considered is all other sorts of problems that your book-keeping set brings: global lifetime and destruction order, thread safety, ...

The set doesn't need to be global.  And actually, mine isn't, but I didn't want to include my entire logging class in the code snippet.  But maybe the way SFML is broken up into separate modules would make this state tracking tricky, I'm not sure.

I agree thread safety may require some additional code.  How much of SFML is threaded?  I was under the impression it wasn't much except for a bit of SoundStream audio stuff.

Of course it's ok if you don't want to use this kind of ASSERT macro.  But I'm not yet convinced the arguments you've provided are good reasons not to.

Kojay

  • Full Member
  • ***
  • Posts: 104
    • View Profile
Re: Logic error handling in SFML
« Reply #8 on: December 01, 2015, 01:53:54 pm »
If I want to deploy a 'release' build that still does some logic error checking, then let me worry about configuring that.

*adds vote for plain assert*

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Re: Logic error handling in SFML
« Reply #9 on: December 01, 2015, 02:32:37 pm »
True, the std lib does some crazy assert checking.  But that kind of code rarely exists outside of the STL.
I wouldn't say that. To what extent assertions are used is up to each library. My library Thor uses them a bit more widely. Most are simple checks, but some assertions come with decent performance overhead (like searching in containers), which is something I definitely wouldn't consider acceptable for Release builds. The only thing you achieve by enabling assertions in Release mode is 1. punish everybody with slower code (in particular people who don't make mistakes), and 2. make everybody worry about performance, and hence be more reluctant about using smarter, more resource-intense assertions. That completely misses the point -- there's a good reason why standard assert is disabled for NDEBUG.

I did a quick search for "assert" in the SFML code base.  There's not much there.
Guess why I opened this thread ;)

What you're gaining is potentially huge.  A bare assert is fine for catching any common case errors.  But for anything rare that is perhaps only triggered outside of the development environment (e.g. hardware related, or based on an end-user's particular use of the software), the bare assert does nothing.
That's why assert is the wrong choice for such scenarios in the first place. Hardware or user input errors are not logic errors.
« Last Edit: December 01, 2015, 02:44:44 pm by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

Jabberwocky

  • Full Member
  • ***
  • Posts: 157
    • View Profile
Re: Logic error handling in SFML
« Reply #10 on: December 01, 2015, 03:02:08 pm »
Alright, it was worth a shot.
Thanks for the discussion.  Raw asserts are certainly better than nothing.

Incidentally, I will be highly temped to do a search and replace for "assert" in SFML a year or two down the road, and test the performance of an  assert macro in release mode.  Then, when the performance turns out to be practically identical, come back and post a highly annoying and smug "I told you so!"   :-*    ;)

As always, I appreciate the great work on SFML.

 

anything