[task #6792] Create ASSERTs for DEBUG compile

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

URL:
  <http://savannah.nongnu.org/task/?6792>

                 Summary: Create ASSERTs for DEBUG compile
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: goldsimon
            Submitted on: Freitag 20.04.2007 um 19:06
                Category: None
         Should Start On: Freitag 20.04.2007 um 00:00
   Should be Finished on: Freitag 20.04.2007 um 00:00
                Priority: 1 - Later
                  Status: None
                 Privacy: Public
        Percent Complete: 0%
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
                  Effort: 0.00

    _______________________________________________________

Details:

I've read on a different thread that we don't take much care in checking
application error conditions (wrong arguments etc.)?
This probably comes from fear of performance loss, which I can understand.
This could make porting / learning lwIP a hard time.

As a solution, I propose to add a new assert macro, LWIP_DEBUG_ASSERT() that
only executes if LWIP_DEBUG is defined. That way we can avoid performance
loss due to error checking and still can discover misbehaviour (mostly of the
programmer) during creation/debugging phase. Currently, we can only turn on or
off all asserts together and can't decide between critical and
programmer-fault asserts.




    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #1, task #6792 (project lwip):

Asserts are a very useful tool, and this is definitely good.  One thing I
would request is that whenever you add an assert you think about if you also
need to add something to cope if the assert doesn't fire (i.e. on a not-DEBUG
build).  Too often people add asserts and forget that they won't always be
there.  Sometimes (if they are asserting against programmer error or API
abuse) that's OK, but other times dealing with it gracefully in a release
build is possible too.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #2, task #6792 (project lwip):

>Too often people add asserts and forget that they won't always be there.

They won't? I guess without them, the stack wouldn't work properly as it is
now!

Maybe we should add LWIP_FATAL(), then? OR at least we should go through all
code ensuring the above isn't the case!

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #3, task #6792 (project lwip):

I added LWIP_DEBUG_ASSERT(x,y) to debug.h in CVS HEAD. This triggers #ifndef
LWIP_NOASSERT and #ifdef LWIP_DEBUG.

Maybe we should change some of the LWIP_ASSERTs to LWIP_DEBUG_ASSERTs and add
LWIP_FATAL which always triggers (and maybe spins in a loop using "while(1);"
to catch some fatal errors)?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #4, task #6792 (project lwip):

I would argue that asserts are a debug-only tool, and that they should
compile out with no side effects (other than the assertion not being there)
by default, in the same way that the LWIP_DEBUGF  macros do.

In theory, there should be no fatal errors in the release code!  However, in
practice I can see that that won't always be true, but in such situations
people can turn on the debug macros to get the assertions and find out what
the problem is.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #5, task #6792 (project lwip):

I think some documentation and clarity is required for what the distinction
between these two types of assert is. I'm still not clear on this.

Also developers need to know somewhere or other about the effect of this?
Perhaps LWIP_NOASSERT could be mentioned in opt.h, along with  a new define
in there for LWIP_DEBUG, which surprisingly is not already in there, despite
being quite an important option IMHO.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #6, task #6792 (project lwip):

I wanted to introduce LWIP_DEBUG_ASSERT() to have an easy way of adding extra
checks when developing which should normally not fire. This could be an
lwIP-newbie for example, who needs some extra-checks.

Or a developer like me, who wants to make sure the code works using some
ASSERTs not really needed after the code works. In this case,  I think having
DEBUG-ASSERTs is better than using normal ASSERTs and removing them later, as
you have kind of documentation of the original programmer's idea/checks in
the code. I think that alone is worth a lot looking at some of the
discussions we had throughout the last months.

On the other hand, I think it sometimes is easier to run into an ASSERT
(which for me, automatically reboots the system through a watchdog) than
including code for all kind of failures. So I think there should be a
distiction between normal ASSERTs (triggering on errors that can happen
during normal running of an application) and DEBUG-ASSERTs (triggering if you
use the stack in a wrong way). The latter can be turned off once you know your
code/configuration is OK.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #7, task #6792 (project lwip):

I think I see what you're getting at. Perhaps there might be a clearer way to
put this in the name - for me asserts are always a "debug" thing, so having
one be LWIP_ASSERT and another LWIP_DEBUG_ASSERT doesn't offer much
difference.

Just a suggestion, but perhaps there could be other names like
LWIP_APP_ASSERT, LWIP_USER_ASSERT, LWIP_API_ASSERT?

Anyway, thanks for the explanation. Perhaps there's some file (or in a
comment in opt.h) a summary of this could go into?


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #8, task #6792 (project lwip):

I agree that asserts are always debug.  If you're trying to check for errors
in release code, asserts are not the way to do it - you should write an error
handler instead if the error is expected.  However, I can see that others
might want them.  I therefore propose:

LWIP_ASSERT for the debug variety that causes whatever is running to fail in
a non-graceful manner
LWIP_CHECK for the non-debug variety that gives a warning, but doesn't do the
non-graceful failure.

In a debug build LWIP_CHECK could be compiled to LWIP_ASSERT



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #9, task #6792 (project lwip):

>In a debug build LWIP_CHECK could be compiled to LWIP_ASSERT

But in a release build both will be left out? That's OK (and faster) if we
make sure that all errors are catched through normal program flow / return
values. We can then remove LWIP_DEBUG_ASSERT and also are faster than now.
That would be a new task, I think.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #10, task #6792 (project lwip):

I can see I wasn't very clear.  What I'm proposing is:

In a release build:
 - LWIP_ASSERT would compile out (as they're only really there to catch
programmer errors)
 - LWIP_CHECK would be a non-fatal warning that something is wrong (as
they're checks for things that could potentially go wrong in real world
conditions)

In a debug build:
 - LWIP_ASSERT and LWIP_CHECK would both be fatal assertions

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #11, task #6792 (project lwip):

So it seems you're suggesting that LWIP_ASSERT would then correspond with
what Simon proposed as LWIP_DEBUG_ASSERT, and many of the existing
LWIP_ASSERTs in current lwIP would become LWIP_CHECK?

Or asking it another way, would LWIP_CHECK be checking for things that really
can go wrong sometimes, or only go wrong if there's a bug in lwIP? It seems to
me that the former is better handled with error returns. So I guess you mean
the latter?




    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #12, task #6792 (project lwip):

>Or asking it another way, would LWIP_CHECK be checking for things that
really can go wrong sometimes, or only go wrong if there's a bug in lwIP? It
seems to me that the former is better handled with error returns. So I guess
you mean the latter?

I'd also like to disable both to have faster code in a bug-free lwIP stack
;-) But maybe it is better to have some checks all the time...

And if you mean LWIP_CHECK should fire for a bug in lwip, I'd call it
LWIP_FATAL. I don't really understand the meaning of LWIP_CHECK. If it's a
non-fatal check, why include it in a release build?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #13, task #6792 (project lwip):

I'm still of the opinion that asserts are a debug feature, and so all the
existing LWIP_ASSERTs would remain as LWIP_ASSERTs.

The LWIP_CHECK was suggested because I thought there was a wish to have
something akin to an assert that persisted in a release build.  I was keen
that this not actually be a fatal error, as such things in release builds are
bad.  I don't have a good example of what it would be used for (and agree that
error handlers would be in most cases better) but there seemed to be a demand
for this, although perhaps I've misunderstood.  Perhaps for those cases where
the programmer is assuming something to be true, and really thinks it will
always be true, but wants a little insurance that there will be some
notification if it turns out not to be true.  They could use LWIP_CHECK to
get this insurance.

I would also be OK with no LWIP_CHECK if people prefer that.  Or with
LWIP_ASSERT becoming just a warning in a release build.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #14, task #6792 (project lwip):

>Or with LWIP_ASSERT becoming just a warning in a release build.

If LWIP_ASSERT becomes a check in release, there still has to be error
handling code. So we can leave away the assert completely (to be faster) and
the user simply has to check the return values...

By now, I don't think that it's necessary to have release-asserts, since we
decided to write the code in a way that errors are handled by returning error
values.

Only the decision is not that easy. I think that e.g. NULL-pointer checks
should be checked in a release build also.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #15, task #6792 (project lwip):

I disagree about NULL pointer checks in a release build. That would be a
programmer error. Unless you are referring to return values from calls to
e.g. mem_malloc

Otherwise, code should be debugged until it works. In most cases in embedded
stuff, if it doesn't work in the field, there's nothing you can do anyway.
What's the point of having an assert in a TV? Or an air conditioning unit?
(Yes those are two examples where customers of ours are using TCP/IP
stacks!). Or even a printer. In the field there's no point - use a watchdog
instead and just reset. Or if it's that important to you, then leave asserts
on in your release build. But for those who write programs that work, these
sorts of checks are a pointless waste of code.


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Follow-up Comment #16, task #6792 (project lwip):

> - use a watchdog instead and just reset.

That's what I do on an ASSERT!

But to close this topic: I will remove the DEBUG_ASSERT again and we'll make
sure to change the code in such way that there are ASSERTs for programmer
errors and error return values for errors that can happen (that's not always
the case until now!).

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Update of task #6792 (project lwip):

                  Status:                    None => Cancelled              

    _______________________________________________________

Follow-up Comment #17:

I removed LWIP_DEBUG_ASSERT again.
Closing this as cancelled.

I'm opening a new task to review all uses of ASSERT to maybe return error
values.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[task #6792] Create ASSERTs for DEBUG compile

Ondrej Lufinka

Update of task #6792 (project lwip):

             Open/Closed:                    Open => Closed                


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6792>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel