Volumes Of Fun
http://www.volumesoffun.com/phpBB3/

Please change the way PolyVox reacts to errors
http://www.volumesoffun.com/phpBB3/viewtopic.php?f=14&t=493		 Page 1 of 2
Author:  TheSHEEEP [ Mon Mar 25, 2013 12:08 pm ]
Post subject:  Please change the way PolyVox reacts to errors
Hey,

I had some time again to play around with 3D noise.
And while doing so, I had multiple crashes in PolyVox.

Or, rather, I WISH I had crashes, so I could jump in with gdb and debug them. Instead, the application just quits. No chance to find out where the error actually came from.

Code:
std::exit

Is something that IMO should never, never, never, never, never be used in a library. It is assuming that the user wants to quit the application if there is an error. Something which is very dangerous (as it removes the possibility to do proper debugging) in your own application, and even more hazardous in an external library such as PolyVox.

A crash is far more helpful than an application quitting silently.

On some occasions, I could find that something like "NaN after normalize" (don't remember exactly) was printed to the console, together with the file in which it happened, but no trace, variable values, etc.
But it did not really help me figuring out where the actual problem was.

So, if PolyVox knows that something will not work out, printing that knowledge is fine and helpful, but silently quitting the application instead of letting it crash is something I actually consider very harmful.
Author:  David Williams [ Mon Mar 25, 2013 4:17 pm ]
Post subject:  Re: Please change the way PolyVox reacts to errors
Yep, I agree with you here. The code in question is in ErrorHandling.h:

Code:
#if defined(_MSC_VER)
    #define POLYVOX_HALT() __debugbreak()
#else
    #define POLYVOX_HALT() std::exit(EXIT_FAILURE)
#endif


As you can see it does something sensible on Windows (goes into the debugger so you can examine the callstack, etc) but I think I just didn't know the equivalent for Linux. However a quick bit of Googling has led me to this:

http://stackoverflow.com/a/173656

So probably on Linux it should use '__builtin_trap()' instead of std::exit(EXIT_FAILURE). If you can test this and let me know if it works then I'll commit it to PolyVox. Failing that it could just use a divide-by-zero of null pointer dereference to force a crash.

As for the root cause of your problem I have seen similar crashes when running the MarchingCubesSurfaceExtractor. It seems it calls 'normalise()' on a vector of length zero so I need to fix this.

If it continues to be a problem you can comment out the relevant POLYVOX_ASSERT() (it should print a filename and line number when it crashes/exits), or you can also disable the asserts completely in the following file:

https://bitbucket.org/volumesoffun/poly ... elop#cl-27
Author:  David Williams [ Wed Mar 27, 2013 9:54 am ]
Post subject:  Re: Please change the way PolyVox reacts to errors
Added an issue here: https://bitbucket.org/volumesoffun/poly ... debugbreak
Author:  TheSHEEEP [ Wed Mar 27, 2013 12:13 pm ]
Post subject:  Re: Please change the way PolyVox reacts to errors
Code:
#if defined(_MSC_VER)

Is the problem, I guess.
I am using MinGW, which naturally does not define _MSC_VER. Getting rid of MSVC is one of the main points of using MinGW ;)
_WIN32 would be a more fitting flag, I think.

I don't know if MinGW has __debugbreak(), though.
It certainly has DebugBreak().

Don't know about Linux, though, sorry :)
Also don't know about clang.
So it is possible that this whole thing should depend not on the platform, but on the compiler.
Author:  David Williams [ Wed Mar 27, 2013 2:02 pm ]
Post subject:  Re: Please change the way PolyVox reacts to errors
TheSHEEEP wrote:
So it is possible that this whole thing should depend not on the platform, but on the compiler.


It is possible, but I think some testing would be needed of the various configurations to really find out. I don't really want to do this (some platforms I don't use myself) so I think the safest option is to fall back on a crash. Then if people want to test better options for per-platform or per-compiler then they can do this, and submit a patch.
Author:  David Williams [ Wed Mar 27, 2013 2:24 pm ]
Post subject:  Re: Please change the way PolyVox reacts to errors
Ok, it now looks like this:

Code:
#if defined(_MSC_VER)
   // In Visual Studio we can use this function to go into the debugger.
    #define POLYVOX_HALT() __debugbreak()
#else
   // On other platforms we just halt by forcing a crash.
   // Hopefully this puts us in the debugger if one is running
    #define POLYVOX_HALT() *((unsigned int*)0) = 0xDEAD
#endif


Hopefully that is ok for most situations, but if people want to test and submit platform/compiler specific solutions then we can also include them.
Author:  milliams [ Wed Mar 27, 2013 2:54 pm ]
Post subject:  Re: Please change the way PolyVox reacts to errors
Having a quick look around, it seems many people suggest
Code:
raise(SIGTRAP);
I mostly got this from a thread on SO. When run outside a debugger, it will just crash but inside GDB it will act like a breakpoint. I've not tested it at all but it's likely to be better than a bare segfault.
Author:  ker [ Thu Mar 28, 2013 8:40 am ]
Post subject:  Re: Please change the way PolyVox reacts to errors
why don't you simply use an exception?
an exception only introduces overhead when thrown and it already has all the features you want (like breakpoint & crash with useful text in release)
+ it can be caught.
Author:  David Williams [ Thu Mar 28, 2013 9:13 am ]
Post subject:  Re: Please change the way PolyVox reacts to errors
milliams wrote:
Having a quick look around, it seems many people suggest
Code:
raise(SIGTRAP);
I mostly got this from a thread on SO.


Yep, this one is also possible. But I guess it's Linux/Unix specific (rather than being GCC specific) which means it's dependant on a runtime test to check which platform we are on, rather than a compiler check.

But you're welcome to add some better platform/compiler specific solutions as long as the fallback is in place. Actually the fallback is also sub-optimal as some platforms actually allow the write to address '0'. I tried doing a divide by zero instead but Visual Studio catches it at compile time.

ker wrote:
why don't you simply use an exception?


Hmmm.... this is actually an interesting suggestion. The code we are discussing above is called by the POLYVOX_ASSERT macro (our alternative to assert() which also works in release builds) and both asserts and exceptions have a valid place in an error handling system.

If you are saying that in some places we should throw an exception rather than asserting then I think you are right, and attempting to normalise a zero-length vector is a good example of this. There are places in PolyVox where we throw exceptions but overall the usage is not consistent and we do need to fix this.

However, I think what you are actually saying is that we could implement the POLYVOX_ASSERT macro by having it throw an exception, and this would then cause the crash into the debugger. I hadn't thought about this and it seems a bit unconventional, but I see the idea discussed here so it could be worth investigating: http://stackoverflow.com/a/37474

One additional point on the subject of exceptions is that they are not supported on all platforms. At least my Android NDK did not support them but I think they are available in newer versions. For now, exception throwing is wrapped in the POLYVOX_THROW macro so that it can be disabled.
Author:  David Williams [ Sat May 11, 2013 8:13 am ]
Post subject:  Re: Please change the way PolyVox reacts to errors
I've just committed some changes to the way errors are handled in PolyVox. Basically I've replaced a number of asserts with exceptions so that the application doesn't need to stop in these situations.

You should also read this addition to the user manual: https://bitbucket.org/volumesoffun/poly ... at=develop
Page 1 of 2 All times are UTC
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group
http://www.phpbb.com/