tasklwIP - A Lightweight TCP/IP stack - Tasks: task #6792, Create ASSERTs for DEBUG compile

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #6792: Create ASSERTs for DEBUG compile

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Fri 20 Apr 2007 07:06:55 PM UTC
   
 
Category:  None Should Start On:  Fri 20 Apr 2007 12:00:00 AM UTC
Should be Finished on:  Fri 20 Apr 2007 12:00:00 AM UTC Priority:  1 - Later
Status:  Cancelled Privacy:  Public
Assigned to:  None Percent Complete:  0%
Open/Closed:  Closed Planned Release:  None
Effort:  0.00

Jump to the original submission

Thu 24 May 2007 07:20:42 AM UTC, 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.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 23 May 2007 05:20:28 PM UTC, comment #16: 


> - 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!).

Simon Goldschmidt <goldsimon>
Group administrator
Wed 23 May 2007 04:37:49 PM UTC, comment #15: 

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.

Jonathan Larmour <jifl>
Group Member
Wed 23 May 2007 04:07:45 PM UTC, comment #14: 


>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.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 23 May 2007 03:13:07 PM UTC, comment #13: 

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.

Kieran Mansley <kieranm>
Group Member
Wed 23 May 2007 03:05:44 PM UTC, comment #12: 


>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?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 23 May 2007 03:01:03 PM UTC, comment #11: 

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?



Jonathan Larmour <jifl>
Group Member
Wed 23 May 2007 02:54:02 PM UTC, comment #10: 

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

Kieran Mansley <kieranm>
Group Member
Mon 14 May 2007 10:34:01 AM UTC, comment #9: 


>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.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 30 Apr 2007 01:56:40 PM UTC, comment #8: 

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


Kieran Mansley <kieranm>
Group Member
Mon 30 Apr 2007 01:48:26 PM UTC, comment #7: 

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?

Jonathan Larmour <jifl>
Group Member
Mon 30 Apr 2007 01:32:04 PM UTC, comment #6: 

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.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 30 Apr 2007 01:21:30 PM UTC, comment #5: 

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.

Jonathan Larmour <jifl>
Group Member
Mon 30 Apr 2007 11:11:25 AM UTC, comment #4: 

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.

Kieran Mansley <kieranm>
Group Member
Sun 29 Apr 2007 04:35:02 PM UTC, comment #3: 

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)?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 27 Apr 2007 07:32:26 PM UTC, comment #2: 


>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!

Simon Goldschmidt <goldsimon>
Group administrator
Mon 23 Apr 2007 11:54:33 AM UTC, comment #1: 

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.

Kieran Mansley <kieranm>
Group Member
Fri 20 Apr 2007 07:06:55 PM UTC, original submission:  

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.

Simon Goldschmidt <goldsimon>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-24 goldsimon Open/ClosedOpen Closed
    2007-05-24 goldsimon StatusNone Cancelled

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code