You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Sander Striker <st...@samba-tng.org> on 2001/06/05 01:12:56 UTC
APR, asserts, policy
Hi all,
The assert() discussion seems to have taken place a few times
now, but I still feel the need to vent my thoughts on the
matter.
To get one thing out of the way beforehand: asserts have no
meaning in production builds. They are simply not compiled in
[well, they become noops].
The same goes for symbols, effectively rendering a coredump
useless. So lets talk about debug builds and the groups of
programmers, the APR programmers and the application programmers
(ie. the APR users).
Then I need to tell something about my background. I work on
multiple platforms, two of them being Windows 2000 and Linux.
I use MS Visual C++ on windows and this has gotten me being
spoilt. When running an application in debug mode and testing
it by using it like a user would, an encountered assert brings
up the visual debugger on the line of the assert and halts the
application there. When this happens I usually know instantly
what I forgot to do [so this is from the 'app programmers'
point of view].
To wrap the assert subject up I'd like to say that I, personally,
like asserts because they allow me to let the code bomb out
in a (semi)controlled way in debug time. Whereas in production
the code simply bombs out 'uncontrolled'. This seems a minor
difference, but it is easier to debug [why were asserts invented
in the first place if this wasn't the case? I have the feeling
someone is going to smack me on the head with a history book on
that one :-) ].
Finally: what is the general feeling about asserts?
On to policy. These last few hours I ran into a 'policy violation'.
I used the following construction somewhere *):
apr_status_t some_apr_func(some_type *pointer_argument)
{
if (!pointer)
return APR_EINVAL;
...
}
I thought this would be good conduct because APR is a library
and should report errors to the application (the user) instead
of bombing out. I agree that this is unneccesary bagage in a
production environment with decent tested/debugged applications.
But what about the testing/debugging phase? The APR library is
probably built with debug flags turned on full in this phase.
Should the code still segfault?
Anyhow, if the policy is segfaulting when passing a NULL pointer,
lets segfault. However, lets document this policy somewhere, so
new apr/patch developers know about it and can adopt the policy.
Ofcourse the review system filters out a lot of this stuff, but
I wonder if the entire APR team knows about the policy.
If there already is a document describing this policy please
provide me with a pointer to it. Maybe a link on the APR development
pages isn't such a bad idea either. If such a document doesn't
exist it is probably wise to put one together. Preferably someone
who is familiar with the policy and 'correct' APR coding style.
Just my opinion on this,
Sander
*) this time it was in apr_md4.c, but it is plastered all over the
sms code aswell. I wonder if this check is valid anywhere?
PS. Sometimes my english is a bit crude, this is caused by the fact
that english isn't my native tongue. So, if it is the language
that ticked someone of, please don't be :-)
Re: APR, asserts, policy
Posted by Ben Laurie <be...@algroup.co.uk>.
Justin Erenkrantz wrote:
> My argument is that I don't like having production code differ from
> debug code AT ALL. I want the debug builds to behave almost
> exactly like the release builds. IMHO, the differences are the
> compiler optimizations, symbols and debugging information that the
> compiler itself places in the binaries. The only difference in a
> release build vs. a debug build is what the compiler does with it. If
> that is violated (i.e. if you have special DEBUG #defines), then you can
> encounter the case of not being able to reproduce the release code. I
> think that is troublesome.
Clearly it is awkward to maintain but there are good reasons for having
debug only code...
> That said, I think there are some special cases inside of the pool - the
> memory management stuff can get so confusing that there is often a need
> for debug functions or routines. However, these wouldn't be enabled by
> default for most developers - only those developers explicitly working
> with the pool code. No one else needs to enable these #defines. The
> code within the #define for apr_md4, apr_sms, etc. weren't really adding
> any value in the manner that some of the pool debug stuff does
> (APR_POOL_DEBUG adds the joined stuff).
>
> Okay, that's why I think the #defines for debug-related things are bad.
> That doesn't necessarily answer the issue with asserts. Why are asserts
> bad? I believe that in a debug build they aren't of any value - because
> your compiler should add enough debugging information to allow a
> debugger to know where something went wrong. I don't see what the
> assert does for you in a debug build that the debugger can't help you
> find already. Just let the bad thing happen (i.e. try to dereference a
> null pointer) and your debugger (if it is worth its salt) will jump to
> there. Once you see the code, you should be able to figure out what
> went wrong.
Checking for null is not a particularly good reason (unless the
detection without an assert is going to be miles away or rare), but I
would certainly agree that in this case the release and debug code
should be the same (i.e. the assert should not go away when debugging is
switched off, if an assert is required at all).
But here's a couple of examples of where you simply can't have the same
code in release as in debug:
a) "is this object in that pool?" - too slow for production code
b) "is my heap consistent?" - too slow and too big for production code.
> There is one set of cases that I think asserts are possibly useful -
> trying to isolate things that just shouldn't happen and you can't
> reproduce it, but somehow it does happen. An assert should only be
> present when you KNOW there is no way in hell that these things can
> happen and you want to dump core to figure out how in the heck this
> happened *or* you can't feasibly attached with a debugger with a debug
> build. Things that might be handled or could be caused by the caller's
> stupidity don't need an assert.
Precisely so. But these cases should assert in production code, too (so
long as they pass the "not too slow or too big" test).
Cheers,
Ben.
--
http://www.apache-ssl.org/ben.html
"There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit." - Robert Woodruff
Re: APR, asserts, policy
Posted by Greg Ames <gr...@remulak.net>.
Justin Erenkrantz wrote:
>
> There is one set of cases that I think asserts are possibly useful -
> trying to isolate things that just shouldn't happen and you can't
> reproduce it, but somehow it does happen. An assert should only be
> present when you KNOW there is no way in hell that these things can
> happen and you want to dump core to figure out how in the heck this
> happened *or* you can't feasibly attached with a debugger with a debug
> build. Things that might be handled or could be caused by the caller's
> stupidity don't need an assert.
>
OK, maybe we agree. There are a couple I put in where I felt we had
dependencies on things that could break someday on some platform under
some load conditions, and that would really be hard to debug without an
assert.
For example, in sendfile_it_all() (server/core.c in httpd-2.0) there's
some hairy logic that does bookkeeping under partial write conditions,
which is dependent on apr_sendfile working, which is dependent on every
platform's sendfile implementation working correctly all the time.
Well, guess what? Every once in a while this assert pops on FreeBSD
(apache.org of course), and now I need to drill down closer to the OS
(i.e. APR) and put in another one. There's no way in hell I can figure
this out with a debugger as it stands.
Other examples are in ap_getline() and ap_setup_client_block(). These
popped intermittently in the past - this time due to our own bugs in
error legs in the httpd 2.0 input filtering stuff. Ryan put in an
assert that caught one of my bugs, and vice versa.
> I think when Jeff and Greg Ames were debugging www.apache.org, they used
> some asserts in httpd in a positive manner.
well, thanks, but most of those are just "traps" looking for a
particular symptom. We throw them away once we understand the problem
and have a fix, and wouldn't put them into CVS. I don't think temporary
traps are an issue.
Greg
RE: APR, asserts, policy
Posted by Sander Striker <st...@samba-tng.org>.
Hi,
I failed to say that I found Justins view of things
(although lengthy... :-) ...like the question :-)
insightfull. At least now I know how he came to
adopt this view. I reckon the rest of you opposing
null asserts share this view.
Sander
RE: APR, asserts, policy
Posted by Sander Striker <st...@samba-tng.org>.
Hi,
> Okay, let me clarify my position on this. I apologize for being
> quasi-abrupt earlier.
No prob.
[...]
> As far as a policy goes, I'm not sure of an official one. Since APR is
> essentially branched from httpd, it really follows httpd policy. AFAIK,
> httpd has always had this policy. And, there are VERY few committers -
> I think I might be the exception (maybe not) - who have APR but not httpd
> access - so most people with APR access are aware of httpd policies.
Ofcourse, well, I didn't go looking in that area... :-)
> I should have spotted this from the get-go, but it slipped by. My fault.
> I'm the one who committed it. It's a learning experience for all of us.
> Don't worry too much. Other people will yell and scream if they find
> something wrong. That's the beauty of open-source.
I'm not at all worried about people pointing out mistakes to one and other.
Not even if the mistakes are mine :-)
My problem was not knowing about the policy and not finding it (maybe I
didn't look hard enough). The trouble with open source is that it sometimes
is a bit chaotic. I must say that apache and apr are rather streamlined
in this light. These projects pay by being a bit bureaucratic once in
a while. But, almost each and every open source project makes the same
mistake (including projects I work on), we fail to document policies and
guidelines properly. Or we forget to place them somewhere people can
easiliy find them. For any new developers on a project this is a hurdle
to take. Since the number of open source developers is not really large
each hurdle is a potential scare away, which is too bad IMHO. [for the
record, if you want to get rid of me, you have a lot more hurdles to set
up :-) ]
Going to sleep soon (2.10 am here...),
Sander
Re: APR, asserts, policy
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Tue, Jun 05, 2001 at 01:12:56AM +0200, Sander Striker wrote:
> Hi all,
>
> The assert() discussion seems to have taken place a few times
> now, but I still feel the need to vent my thoughts on the
> matter.
>
> To get one thing out of the way beforehand: asserts have no
> meaning in production builds. They are simply not compiled in
> [well, they become noops].
> The same goes for symbols, effectively rendering a coredump
> useless. So lets talk about debug builds and the groups of
> programmers, the APR programmers and the application programmers
> (ie. the APR users).
Okay, let me clarify my position on this. I apologize for being
quasi-abrupt earlier.
My argument is that I don't like having production code differ from
debug code AT ALL. I want the debug builds to behave almost
exactly like the release builds. IMHO, the differences are the
compiler optimizations, symbols and debugging information that the
compiler itself places in the binaries. The only difference in a
release build vs. a debug build is what the compiler does with it. If
that is violated (i.e. if you have special DEBUG #defines), then you can
encounter the case of not being able to reproduce the release code. I
think that is troublesome.
That said, I think there are some special cases inside of the pool - the
memory management stuff can get so confusing that there is often a need
for debug functions or routines. However, these wouldn't be enabled by
default for most developers - only those developers explicitly working
with the pool code. No one else needs to enable these #defines. The
code within the #define for apr_md4, apr_sms, etc. weren't really adding
any value in the manner that some of the pool debug stuff does
(APR_POOL_DEBUG adds the joined stuff).
Okay, that's why I think the #defines for debug-related things are bad.
That doesn't necessarily answer the issue with asserts. Why are asserts
bad? I believe that in a debug build they aren't of any value - because
your compiler should add enough debugging information to allow a
debugger to know where something went wrong. I don't see what the
assert does for you in a debug build that the debugger can't help you
find already. Just let the bad thing happen (i.e. try to dereference a
null pointer) and your debugger (if it is worth its salt) will jump to
there. Once you see the code, you should be able to figure out what
went wrong.
There is one set of cases that I think asserts are possibly useful -
trying to isolate things that just shouldn't happen and you can't
reproduce it, but somehow it does happen. An assert should only be
present when you KNOW there is no way in hell that these things can
happen and you want to dump core to figure out how in the heck this
happened *or* you can't feasibly attached with a debugger with a debug
build. Things that might be handled or could be caused by the caller's
stupidity don't need an assert.
I think when Jeff and Greg Ames were debugging www.apache.org, they used
some asserts in httpd in a positive manner. This was because they had
no clue how this particular bug happened, so they wanted to dump core
immediately to analyze it - they couldn't conceivably wait around with a
debugger for the problem to occur. That's where the assert is useful.
They came back a few hours later and picked up the core files.
Using asserts only when necessary leads to cleaner code (IMHO) that
doesn't need to deal with no-ops. If they are no-ops, they shouldn't be
there in the first place.
As far as a policy goes, I'm not sure of an official one. Since APR is
essentially branched from httpd, it really follows httpd policy. AFAIK,
httpd has always had this policy. And, there are VERY few committers -
I think I might be the exception (maybe not) - who have APR but not httpd
access - so most people with APR access are aware of httpd policies. I
should have spotted this from the get-go, but it slipped by. My fault.
I'm the one who committed it. It's a learning experience for all of us.
Don't worry too much. Other people will yell and scream if they find
something wrong. That's the beauty of open-source.
My $.02. -- justin
Re: APR, asserts, policy
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 5 Jun 2001, Sander Striker wrote:
> To wrap the assert subject up I'd like to say that I, personally,
> like asserts because they allow me to let the code bomb out
> in a (semi)controlled way in debug time. Whereas in production
> the code simply bombs out 'uncontrolled'. This seems a minor
> difference, but it is easier to debug [why were asserts invented
> in the first place if this wasn't the case? I have the feeling
> someone is going to smack me on the head with a history book on
> that one :-) ].
>
> Finally: what is the general feeling about asserts?
I see a use for them... but checking NULL pointers isn't usually it IMHO.
As I see it, an assert() is good if you want to test for some non-trivial
condition that wouldn't result in a segfault (at least not right away),
but is nevertheless really, really bad. Assertions for NULL pointers are
okay IMHO if the pointer isn't going to be deref'ed right away (ie, no
immediate segfault), but the pointer still shouldn't ever be NULL.
In any other case (ie, any case where you can skip the assert and still
get a coredump right away), you don't need an assert(). That's my take,
and I *think* it echos the feelings of many people here.
> On to policy. These last few hours I ran into a 'policy violation'.
> I used the following construction somewhere *):
>
> apr_status_t some_apr_func(some_type *pointer_argument)
> {
> if (!pointer)
> return APR_EINVAL;
>
> ...
>
> }
>
> I thought this would be good conduct because APR is a library
> and should report errors to the application (the user) instead
> of bombing out.
I've had that feeling myself. But in the end I'm forced to agree that
it's better to segfault than to just return an error... as Jeff said, if
the caller couldn't manage to pass us a non-NULL pointer, they probably
aren't going to bother checking our return code. That could make for a
really tough debug in some situations.
> Anyhow, if the policy is segfaulting when passing a NULL pointer,
> lets segfault. However, lets document this policy somewhere, so
> new apr/patch developers know about it and can adopt the policy.
Fine by me.
> Ofcourse the review system filters out a lot of this stuff, but
> I wonder if the entire APR team knows about the policy.
<shrug>
> PS. Sometimes my english is a bit crude, this is caused by the fact
> that english isn't my native tongue. So, if it is the language
> that ticked someone of, please don't be :-)
I honestly didn't notice. =-)
Anyway, I've just committed a biggish patch to SMS that removes all of the
parameter checking, extraneous asserts (ie, ones where the code would
segfault right away anyhow), and fixed a bug which caused
apr_sms_assert() to never be compiled. Do me a favor and look it over.
If you feel I've removed too much, just say so and I'll put it back.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA