errorchecking branch
Daniel K. O.
error checking code mentioned some days ago.
Currently I'm just replacing dAASSERT() and some dUASSERT() with
dCheck*(), taking care to not leak anything. I'm using camelCase instead
of ALLCAPS because it's more readable and easier to type. dAASSERT()
(and possibly dUASSERT too) will naturally disappear, and dIASSERT()
will be moved to the private header (error-private.h).
There are 3 behaviors:
* dNOERRORCHECK: as the name implies
* dFATALERRORS: just abort (will probably change into the current
d*ASSERT() behavior)
* dEXCEPTIONS: throws an exception
* default: register the error with dSetLastError(), usually returns
either an invalid or a safe value.
Currently only ode.cpp was updated, so I would like to ask for comments
about the implementation. Specially about how detailed the error
messages should be. Is it worth to have an extra descriptive string,
from the context where the error ocurred?
PS: As this will touch a ton of lines, I started adding modelines to the
end of the files (currently only tested on emacs). They should work with
mainstream editors (although most require them in the top of the files).
Would anyone oppose modelines on the top of the file (so it will work
with vim, kate, gedit, etc)? Does VS have anything similar?
--
Daniel K. O.
"The only way to succeed is to build success yourself"
Oleh Derevenko
User can assign its own assert handler and what he likes: throw and
exception, call abort or assign "last error".
Why are all these complications you started for?
Oleh Derevenko
-- ICQ: 36361783
Oleh Derevenko
NULL-pointer it will not do any good to just return because further code in
caller may not be able to handle that invalid return.
Each such assert must be checked. You must verify all the places the
function could be called from and make sure the caller can handle that
invalid result. Do you realize what amound of work is that? That is
monotonous work that makes you tired very soon. Can you predict how many
errors you'll make?
2) ASSERT is written in all uppercase to distinguish it from ordinary
functions. It shows you immediately, that ASSERT is not a function but a
macro and its behavior may change or it may be removed from code. Changing
case to function-like makes code much more unclear.
3) Returns from inside of the function are the bad style. They lead to
potential bugs. Especially dangerous are returns hidden in macros and
especially when those macros look like functions.
My resume:
You will not achieve anything by just changing one macro to another. You
need to distinguish real asserts from false asserts and change false asserts
to error return and error handling in all the callers. But this is huge
amount of work which requires analysis of whole codebase and I doubt you are
such a maniac to do it. So, please do not change anything.
Oleh Derevenko
-- ICQ: 36361783
----- Original Message -----
From: "Daniel K. O." <danielk...@gmail.com>
To: <ode-...@googlegroups.com>
Sent: 22 березня 2008 р. 16:18
Subject: [ode-users] errorchecking branch
>
Dave Grundgeiger
> PS: As this will touch a ton of lines, I started adding modelines to the
> end of the files (currently only tested on emacs). They should work with
> mainstream editors (although most require them in the top of the files).
> Would anyone oppose modelines on the top of the file (so it will work
> with vim, kate, gedit, etc)? Does VS have anything similar?
>
that's no guarantee. My preference is that editor-specific
meta-information not be added to the files at all--top or bottom.
Dave
Daniel K. O.
> Currently asserts are customizable.
They are non-recoverable.
For an application with limited interactivity, like a game, you don't
need it. You design and tune everything so it always work (heh... you
wish).
For more interactivity you need to fail gracefully.
> User can assign its own assert handler and what he likes: throw and
> exception
Ok, this is the only "recoverable" way, with the current asserts. But
only for an exception-enabled build. And exceptions are always slower
(although we can debate about efficiency when things fail...).
> call abort
That's the only way to use current asserts.
> or assign "last error".
How can you set the error code when all the handler gets is a string
describing the error?
> Why are all these complications you started for?
Because I need them for an interactive physics environment, where the
user is supposed to play with all of ODE. I don't want it aborting,
throwing exceptions to C glue code (script language bindings in C). If
I have to write checks for everything (say, to catch every exception
and translate into proper error handling, hoping nothing inside ODE
leaks because of the throw), I seems more useful to have into the
core, so others can benefit.
Daniel K. O.
> Each such assert must be checked. You must verify all the places the
> function could be called from and make sure the caller can handle that
> invalid result. Do you realize what amound of work is that? That is
> monotonous work that makes you tired very soon. Can you predict how many
> errors you'll make?
Geez, thanks for the motivation.
Don't worry, I'm carefully checking the codepath.
Note that even if I miss something, it won't be worse than the current
implementation. It's actually a superset of the asserts we are using.
> 2) ASSERT is written in all uppercase to distinguish it from ordinary
> functions. It shows you immediately, that ASSERT is not a function but a
> macro and its behavior may change or it may be removed from code. Changing
> case to function-like makes code much more unclear.
It's more a matter of taste. On glib, similar macros are lower-case
(g_return_if_fail()). Note that I'm encoding more than a single word
in the macro name, it's easier to read when there are some separators.
> 3) Returns from inside of the function are the bad style. They lead to
> potential bugs. Especially dangerous are returns hidden in macros and
> especially when those macros look like functions.
I'm being extra careful.
Note that it causes problem with just one of the possible semantics
(dSetLastError()), which you don't seem fond of. The current code is
just as broken (actually, more broken, as I added some more argument
tests that were missing).
> You will not achieve anything by just changing one macro to another. You
> need to distinguish real asserts from false asserts and change false asserts
> to error return and error handling in all the callers. But this is huge
> amount of work which requires analysis of whole codebase and I doubt you are
> such a maniac to do it. So, please do not change anything.
I'm not changing one macro to another. If I were, I would just rewrite
dAASSERT(). :P
I'm carefully adding semantics, one piece at time. It might take long
to finish, but it's supposed to be always consistent.
I won't even push to get this accepted into trunk, if there's really
opposition. But I hope that by "do not change anything" you don't mean
to not play around the branch.
Daniel K. O.
> I'm a longtime VS user and I haven't heard of such a feature, though
> that's no guarantee. My preference is that editor-specific
> meta-information not be added to the files at all--top or bottom.
Geez, it's always those MS devs that spoil the fun. Just because you
guys don't have shiny toys we are not allowed to play with them. :P
It would help improving code formatting - a problem that was mentioned
many times, but no one has balls to apply a reformatting tool to fix
all the code base. Modelines actually go a step further, making sure
the formatting stays the same.
But it's fine, I can just remove the extra comment lines at the end of
the file, if that really bothers VS users. Formatting policy is not
the primary objective anyways (for now).
PS: for those wondering what this is about,
http://www.gnu.org/software/emacs/manual/html_node/emacs/Specifying-File-Variables.html
Jon Watte
> But it's fine, I can just remove the extra comment lines at the end of
> the file, if that really bothers VS users. Formatting policy is not
> the primary objective anyways (for now).
>
I don't care, as long as they go on the end. Have the VS users who
actually care remove them, if they're so persnickety :-)
Btw: I use gvim on Windows at times, and Visual Studio at times. Visual
Studio takes a different approach to "common formatting:" everybody in
the organization can import the same settings file, and then all the
code you write will have the same settings. Can't say one is better or
worse than the other. (And I bet you could write a Visual Studio Macro
that would parse the mode lines).
Sincerely,
jw
gl
> that would parse the mode lines).
... which wouldn't work on the Express versions (no macro support).
--
gl
Oleh Derevenko
What do we have now
---------------------------------
We have several macros which call dDebug/dMessage/dError. These functions
either invoke a user provided callback or print the message and call
exit/abort/nothing. Not all the asserts currently are real asserts. Some of
them just substitute error checking (what is wrong and what's got to be
fixed).
From those macros we have:
1) dIASSERT is the real assertion check. It has to remain where it is used
propertly and replaced with other methods where it is used for wrong
purposes. It has to be used through whole codebase. To do it, you have to
have good understanding of whole library structure and of logic used in it
at all the levels.
2) dUASSERT is the user assert macro. Actually, this macro is the nonsense
and should be removed. User can't do anything wrong to trigger the assert.
If library allowed that, it's the fault of library, not the fault of the
user. Normally it has to be replaced with parameter validation and error
return.
Parameter validation must be used only on external entry points in public
API. You only need to validate input values that come from outside of
library. If all external entries are secured, there should never appear
invalid parameters in internal calls. If that happens that is dIASSERT.
3) dAASSERT is just a subclass of dUASSERT and should be removed as well.
4) dDEBUGMSG is not an assert but just a logging replacement. It should be
used to trace some debug information where it is difficult to view it with
debugger and should not influence the execution flow.
If dIASSERT is placed in the code correctly and if dUASSERT is replaced with
error checking that would be sufficient error checking measures.
What do I see in your changes
---------------------------------
1) An option to ignore all the checks.
Well, that's one of the options which we currently have. Though it is not
correct to ignore the parameter validation. You can ignore asserts, but
parameter validation on external entry points must be performed always. The
library is a closed entity and it can't trust any external data whether it
is a release build or not. If you trust the validity of external input you
agree to be guilty for the faults in external code which may result in
feeding you corrupted input data.
2) An option to call system assert().
This is even less than what we have now. Because now we have a customizable
asserts. And I personally use that feature in my project. I also do call
abort() but I do it a special way from my own assertion failure handler.
3) An option to thow an exception or SetLastError(dErrNoError) if condition
is true.
First of all, let me mention that setting LastError on succes is not the
best approach. All unixes obey the rule (unfortunartely, Microsoft were not
wise enough to follow it in WinAPI) that errno is assigned on failure only.
A successful call preserves last error code. The intent of this rule is to
keep error code intact during exit from the call stack on failure.
Imagine, in a middle of the invocation stack you open a file or allocate
some memory and call child function after that. When that lower level
function fails you have to free temporary memory or close the file before
you pass the error through to the upper levels. But memory deallocation of
file closure will succeed. And if they assigned EOK they would have erased
the last error code which you have to pass through.
Now, about exceptions themselves. As I can see you have several exception
codes that classify the nature of the fault. But truly speaking that is just
slightly better from what we have now. Since all the assertion checking
macroes are currently customizable you can assign your own handler, pack
source line and file name along with assertion kind into and exception and
throw it. Both your exceptions and exceptions that might be currently
generated from assertion handler are useles for the program. They are not
suitable for carrying out any automatic handling in client code. They are
only worth for presenting them to user in a bessage box. Imagine, you call
dQuickStep() and get exception with dErrInvalidBody. Well, and what? How is
this better from getting exception with the string "assert in file aaa.cpp
in line 1234"?
Also, please remember that just raising an exception is not enough. If you
raise the exception you must look through all the possible call stacks and
put try ... catch() { free(); throw; } to deallocate all the temporary
resources (memory, handles, objects) that might be allocated in callers.
4) An option to set LastError and return (with void or with invalid value)
in case of error.
The remark about assignment of last error on success applies here as well. I
also have forgotten to mention that assignment of last error on success is
the unnecessary waste of CPU resources. Returning a boolean would be much
cheaper.
Next, setting LastError and returning void is a weird approach. People will
not understand it. I understand that you do not want to change function
prototypes, but you should change them if you want to achieve any
improvement. If function has a void return it implies that it can't fail.
And it looks very unnatural to call a void function and check last error
after that.
By the way, how do you plan to check last error after void functions?
Depending on compilation mode they may not return last error at all. They
may throw an exception or just ignore the error. Adding checking for last
error after function calls would be an unnecessary waste of code in these
modes. Are you going to add one more macro?
Returning invalid value (false status) is the most correct approach. But
just like with exceptions it is not enough to just return NULL. You have to
check the possible callers and make sure that they are prepared for the
possibility of invalid return and that they cancel their job, free resources
that they might have allocated, and return an invalid result on their own.
And then you have to check all callers of possible callers to make sure they
handle invalid return correctly. And the callers of the callers of the
callers. And so on until exit from the library into client code where our
responsibility ends. Many of public APIs are not designed for error
handling. They do not have execution status returns. You will have to change
them if you do not want to just make things even worse.
By adding possibility of invalid returns or exceptions you create new rules
which will have to be obeyed for future code development. Let's imagine that
the miracle happens and you implement correct error handling in all the
code. But who will watch for future changes and who will tell all the
developers that they have to check for lasterror after each void call, and
that they have to put try..catch() { free/delete; throw; } in the code
because each function can throw an exception? Here all the developers are
uncontrollable. There is no management. It will be a complete mess.
Also, parameter validation is a good thing (on the public API only!!!) but
do not hide "if" statements in the macros. It is very unclear and confusing.
And it is hard to step through it in debugger. Do you remember we agreed to
limit macro usage? If you do parameter validation, put conditional
statements explicitly in the code and only hide failure branch in the macro
(because it may change).
Also, about my statement that returns from inside of the functions are the
bad style and your response that you are checking everything carefully.
Even if you watch for returns from inside of the functions, others may miss
them. It is common source of bugs if you add something at the end of large
function and do not notice that your code may be skipped because somewhere
in middle of it there is a burried return statement.
To conclude:
1) You can implement correct error handling, but only one of them. Error
returns and exceptions are quite incompatible and supporting them both will
make code too heavy.
2) Replacing all the assertions with return NULL or throw Exception is
incorrect. Assertions are the assertions and parameter validation is the
parameter validation. They can't be substituted with each other.
3) It is incorrect to allow eliminating all the validations in release
build. Especially external input validations. And also there may be the
needed to have special assertions kind which can be left active in release
build as welll. There can be places where functions should normally never
fail and adding error returns is difficult and/or would make code
unnecessarily complicated. So placing release asserts is the best choice for
these cases. A great example is the call to pthread_cond_signal() or
pthread_mutex_unlock().
4) You should not restrict yourself in changing function prototypes. Because
if something is not designed for some purpose it is not designed for that
purpose. It is better to change it once and live happily ever after than to
struggle with it and try to conform to it all your life with no success.
5) You do not show enough understanding of the complexity and depth of the
problem (that's just my personal opinion based on what I've seen in your
initial design) and if you continue the development this way and the changes
will be accepted that will be a disaster for the library. I suggest you
spending more time on planning the apporaches and developing base
instruments (macroes/functions) you will use. Because these are the most
important things and a little mistake at the beginning can make all your
work worthless and changes unusable.You have to have a clear image in your
mind of how it will work on all levels and in all variants in whole library
before you start implementing anything.
Oleh Derevenko
-- ICQ: 36361783
----- Original Message -----
From: "Daniel K. O." <danielk...@gmail.com>
To: <ode-...@googlegroups.com>
Oleh Derevenko
>> functions. It shows you immediately, that ASSERT is not a function but a
>> macro and its behavior may change or it may be removed from code.
>> Changing
>> case to function-like makes code much more unclear.
>
> It's more a matter of taste. On glib, similar macros are lower-case
> (g_return_if_fail()). Note that I'm encoding more than a single word
> in the macro name, it's easier to read when there are some separators.
It is not the matter of taste. Macros must obey different naming conventions
than that of identifiers to prevent possibility of coincidence of macro name
and function/type/variable name. If they use g_return_if_fail() as macro
that means that they should never allow all lowercase identifiers with
underscores in them. If despite of all they do, that's only the indication
that glib was designed by unskilled students.
Daniel K. O.
> and function/type/variable name. If they use g_return_if_fail() as macro
> that means that they should never allow all lowercase identifiers with
> underscores in them. If despite of all they do, that's only the indication
> that glib was designed by unskilled students.
Comparing the amount of nonsense I hear from here and gtk-devel, I
would say the opposite.
Daniel K. O.
> Parameter validation must be used only on external entry points in public
> API.
Which is what I currently touched.
> 2) An option to call system assert().
> This is even less than what we have now.
Yeah, I would leave it all (but the error return version) blank. It's
trivial to replace assert() by dAASSERT(), or whatever.
> First of all, let me mention that setting LastError on succes is not the
> best approach.
Fair enough. Removing less than half a dozen lines fixes this.
> throw it. Both your exceptions and exceptions that might be currently
> generated from assertion handler are useles for the program. They are not
> suitable for carrying out any automatic handling in client code.
Partially true.
Should the user know when an ODE function fails for lack of memory?
Maybe he can free some memory and try again. This is the usual runtime
error that nobody has control over, and should always be recoverable.
Did he try to create a contact joint with bad parameters? Maybe it was
a fault from the CD code, and he can chose to ignore this joint for
the next step.
A stepping error caused by bad-parametrized joints (e.g.
non-perpendicular axis) can be recovered in an interactive
environment. It's a runtime error that could be avoided, but it's not
always clear where it might be coming from (error is detected sometime
after bad data got into ODE). To recover from it the user might need
to "undo" more than one operation.
Argument errors, as precondition/logic errors (e.g. "bodies and joints
must be in the same world"), come from programming errors, and
reporting them is only useful for debugging. These should never happen
in the final program. Still, a hard interruption of execution is not
always the best way to debug them.
> Also, please remember that just raising an exception is not enough. If you
> raise the exception you must look through all the possible call stacks and
> put try ... catch() { free(); throw; } to deallocate all the temporary
> resources (memory, handles, objects) that might be allocated in callers.
Good thing C++ has RAII, so we don't actually have to track resources
by hand when properly programmed.
> And it looks very unnatural to call a void function and check last error
> after that.
You have not made much error checking in OpenGL lately, did you? :)
Agreed, changing the API to always return something is more elegant.
What about functions that return values (like dReal, or int)?
> Returning invalid value (false status) is the most correct approach. But
> just like with exceptions it is not enough to just return NULL. You have to
> check the possible callers and make sure that they are prepared for the
> possibility of invalid return and that they cancel their job, free resources
> that they might have allocated, and return an invalid result on their own.
Yes, of course. ODE doesn't have that many layers (ok, maybe on the CD
code, but I don't intend to add error checking to that part, it's a
different beast anyways), like say, a GUI library.
> By adding possibility of invalid returns or exceptions you create new rules
> which will have to be obeyed for future code development. Let's imagine that
> the miracle happens and you implement correct error handling in all the
> code. But who will watch for future changes and who will tell all the
> developers that they have to check for lasterror after each void call, and
> that they have to put try..catch() { free/delete; throw; } in the code
> because each function can throw an exception? Here all the developers are
> uncontrollable. There is no management. It will be a complete mess.
(I was going to write a funny, sarcastic comment about this, but I
will keep it to myself, in case it might offend some.)
You are saying that if I create an unmaintainable framework for error
checking, people will break it. So I'll create a maintainable
framework instead. And by "I" I mean "we", because comments like yours
also help.
> To conclude:
> 1) You can implement correct error handling, but only one of them. Error
> returns and exceptions are quite incompatible and supporting them both will
> make code too heavy.
I'm fine with just error returns and no exceptions. Exceptions can be
generated in the C++ wrapper.
> 2) Replacing all the assertions with return NULL or throw Exception is
> incorrect. Assertions are the assertions and parameter validation is the
> parameter validation. They can't be substituted with each other.
The code I touched didn't require any rewriting, that's why I
mentioned I just replaced the macros. I intend to rewrite significant
parts of the code if needed.
> 3) It is incorrect to allow eliminating all the validations in release
> build.
Agreed. See if you agree with my point above, about non-controlled
runtime errors, user's fault runtime errors, and argument/logic
errors. The later should be allowed to be eliminated from release
builds.
From this, I assume there are 2 "recipes":
* Optional error checking should be macros.
* Mandatory error checking should have explicit conditionals. And by
that I assume it's implicit to go for the error returns only.
> 5) You do not show enough understanding of the complexity and depth of the
> problem (that's just my personal opinion based on what I've seen in your
> initial design) and if you continue the development this way and the changes
> will be accepted that will be a disaster for the library. I suggest you
> spending more time on planning the apporaches and developing base
> instruments (macroes/functions) you will use.
Well, I got the ball rolling, and started experimenting with it
(really, less than one hour of work). Having feedback and critics are
much efficient than planning and iterating the development all by
myself.
Oleh Derevenko
----- Original Message -----
>> And it looks very unnatural to call a void function and check last error
>> after that.
>
> You have not made much error checking in OpenGL lately, did you? :)
> Agreed, changing the API to always return something is more elegant.
> What about functions that return values (like dReal, or int)?
I have never seen the sources of OpenGL and have even hardly ever tried to
use it.
dReal is not a good return for error checking. Comparing floating point
values is relatively slow and floating-point foes not have invalid value in
its doman. Well, it has NaN but it can't be compared for equality directly.
I'm convinced that the best choice is the one I mentioned recently: all the
functions that may fail return boolean status and if status is "false" they
set last error information. All the rest outputs should be returned via
parameters. Also if function can not fail, but needs to return a value (even
and especially a boolean) it should return it via output parameter, not via
result. Exceptions are simple accessorss Get.../Set...() for private fields
and methods which stand for object/memory allocators.
You'll be able to see an example of that style soon, when I finish my TLS
support library. Actually, it is relatively finished already but I need to
make test application to cover all the functionality (many of features are
macroes or templates and you will not even get compilation errors until you
make references to them from a program). Also, in addition to TLS there will
be, three variations of ASSERT macros, atomic (interlocked) API support,
classes for flags with ordinary and atomic access, templates for
encoding/deconing enumerated types via arrays, templates for enumerated
types' increment/decrement operations, some useful macros, and all these
with possibility to customize asserts and memory management functions and
possibility to link the library into one binary more than once as a
sub-library of other libraries.
>> 3) It is incorrect to allow eliminating all the validations in release
>> build.
>
> Agreed. See if you agree with my point above, about non-controlled
> runtime errors, user's fault runtime errors, and argument/logic
> errors. The later should be allowed to be eliminated from release
> builds.
I'm not quite sure I correctly link this sentence to the earlier text, but
runtime errors are unavoidable and therefore we have no choice except for
handling them. User's fault runtime errors are the kind of parameter
validation errors. Just it is not always easy/possible to validate
everything at once. Some complex relations must be validated deeper inside
of the library, when the necessary data is prepared and program enters
required execution context. Well, that's normal. You only need to keep in
mind that program should be structured the way to allow to perform each
validation only once, and not to re-validate the same data in each next
nested call or algorithm stage.
Logic error checks (i.e. asserts) should be eliminated from release build.
Argument errors... well, it mostly depends where the data comes from. You
write a library which will run inside of host process you may remove
parameter validation as the process and the library are executed as one. If
you write a process which gets the data from outside of its virtual address
space (a file, a socket, a user input) that data must always be validated
whether it is release build or not (well, I'm sure you know it very well
yourself, though).
Daniel K. O.
> I'm convinced that the best choice is the one I mentioned recently: all the
> functions that may fail return boolean status and if status is "false" they
> set last error information. All the rest outputs should be returned via
> parameters. Also if function can not fail, but needs to return a value (even
> and especially a boolean) it should return it via output parameter, not via
> result. Exceptions are simple accessorss Get.../Set...() for private fields
> and methods which stand for object/memory allocators.
There's another alternative: don't touch the return value, return the
error via output parameter. As an example, see:
http://library.gnome.org/devel/glib/stable/glib-Error-Reporting.html
Oleh Derevenko
I'll tell you what's wrong with "output parameter only" approach. Output
parameter is always allocated on stack. That means that to assign it and to
test it you need at least two memory access operations. Examining the result
of function execution is almost always the first thing that is to be done
after a call returns. Therefore it is better for quick decision to check
return value (which is stored in CPU register) rather than examine a memory
location. If you find that return status is "false" well, then you can spend
time for checking additional error info in memory and for
accumulating/trannsferring it further. But since normally calls should
succeed, extended error information (LastError in simplest case) is not
generated and error test is limited to only one register value comparison.
Regarding, what is to be used for extended error information storage: user
provided structures or TLS - it is the matter of instruments available to
developer. If you do not have uniform TLS support for all intended build
targets you are forced to use user provided buffer or dynamic memory
allocation. However, if you have TLS, it is much more convenient as it does
not require passing extra parameter and does not require explicit error
buffer allocation by client or function being called. You can just have
sufficient memory block allocated in TLS and use it for storing/retrieving
error details.