Mon 16 Jul 2007 05:13:00 PM UTC, comment #19:
Since I'm pretty sure this is fixed, I'll close it.
|
Mon 16 Jul 2007 05:01:44 PM UTC, comment #18:
Ok for me to close
|
Mon 16 Jul 2007 04:59:45 PM UTC, comment #17:
I already took out the next=NULL. I only let it in for MEMP_OVERFLOW_CHECK=1. That way if someone is really debugging the application, the structure is easier to read. I don't think this is so bad because MEMP_OVERFLOW_CHECK=1 wouldn't be used in realease builds anyway.
If you're OK with that, I think we can close this bug.
|
Sun 15 Jul 2007 03:13:05 PM UTC, comment #16:
I would say you could take the next=NULL out. The user never uses it, and memp_* dont need it unless it's on the list.
|
Sun 15 Jul 2007 12:29:02 PM UTC, comment #15:
Ok for me. Just a thing: if the "memp->next = NULL;" really useful? I think we could remove it (since, with MEMP_OVERFLOW_CHECK=0, it next will be overwritten, and the memp_free doesn't check it , but just set it)...
|
Sun 15 Jul 2007 05:52:37 AM UTC, comment #14:
Yeah, put it inside the if. That is the best I think.
|
Sat 14 Jul 2007 05:15:18 PM UTC, comment #13:
It would be even better to have the memp += MEM_SIZE inside the body of if(memp!=NULL). That way we totally save redundant code:
...
if (memp != NULL) {
memp_tab[type] = memp->next;
#if MEMP_OVERFLOW_CHECK
memp->next = NULL;
memp->file = file;
memp->line = line;
#endif /* MEMP_OVERFLOW_CHECK */
#if MEMP_STATS
...
#endif /* MEMP_STATS */
LWIP_ASSERT(...);
memp = (struct memp)((u8_t)memp + MEMP_SIZE);
} else {
LWIP_DEBUGF(...);
#if MEMP_STATS
...
#endif /* MEMP_STATS */
}
SYS_ARCH_UNPROTECT(old_level);
return memp;
}
OK?
|
Sat 14 Jul 2007 04:49:39 PM UTC, comment #12:
re comment #11:
Exactly, that's how I'd think it would be better.
> But it increase a little bit the footprint ("more" if SYS_LIGHTWEIGHT_PROT=1).
Does it? It's one 'if' statement less, which means less code.
We could argue here if the if statement is more code than the 'return NULL' or the 'SYS_ARCH_UNPROTECT'... For a good compiler, this probably wouldn't make a difference, but why not help the other compilers?
After all, I think the code is better to read that way.
|
Sat 14 Jul 2007 03:28:39 PM UTC, comment #11:
Yes, ok, that's the point I miss (explain at the beginning of the file): "We don't need to preserve the struct memp while not allocated, so we can save a little space and set MEMP_SIZE to 0."
Sorry Simon. So, if we want to avoid the "extra" if, we could come to the original patch with a little change:
+++ memp.c 2007-07-11 11:41:49.105656200 -0700
@@ -355,6 +355,7 @@
#if MEMP_STATS
++lwip_stats.memp[type].err;
#endif /* MEMP_STATS */
+ SYS_ARCH_UNPROTECT(old_level);
+ return NULL;
}
SYS_ARCH_UNPROTECT(old_level);
But it increase a little bit the footprint ("more" if SYS_LIGHTWEIGHT_PROT=1). Footprint vs Speed, for this specific case, I don't have any preference...
|
Sat 14 Jul 2007 03:08:05 PM UTC, comment #10:
> return memp:?memp:NULL;
I would think a compiler would do that. I'm not near my computer now, so I cant look to see right now. I can check on Monday, but this seems like a straightforward optimization.
> should return an "invalid" address (MEMP_SIZE)
Yes, it would. If MEMP_OVERFLOW_CHECK=0, then MEMP_SIZE=0, so no error.
|
Sat 14 Jul 2007 02:51:32 PM UTC, comment #9:
I not sure to understand why with MEMP_OVERFLOW_CHECK=0, the case should not appears. If there is not memp in a pool, I think the memp[tab]==NULL, right? In this case, even with MEMP_OVERFLOW_CHECK=0, the case where memp_tab[type]==NULL at the beginning of memp_malloc should happen when we are out of memory, and in this case ...
return (void)((u8_t)memp + MEMP_SIZE);
should return an "invalid" address (MEMP_SIZE). Or there is something I don't understand... Else, how should be the fix for you?
|
Sat 14 Jul 2007 08:43:41 AM UTC, comment #8:
I don't know about the fix:
This bug only appears if MEMP_OVERFLOW_CHECK=1. With the current patch, we always have an 'if' statement (..?..:..) at the end of memp_malloc, which is not necessary for MEMP_OVERFLOW_CHECK=0.
But then again, good compilers might optimize that away since it gets
return memp:?memp:NULL;
for MEMP_OVERFLOW_CHECK=0, which should be pretty easy to optimize, shouldn't it?
|
Sat 14 Jul 2007 12:08:28 AM UTC, comment #7:
Change is commited. Added a brief comment, per what Thomas was suggesting.
How's it look Thomas? If you're happy, we'll close this bug.
|
Fri 13 Jul 2007 10:51:32 PM UTC, comment #6:
Ok, the last patch file is good. Check in. About comments, add them if you are sure...
|
Fri 13 Jul 2007 06:59:35 PM UTC, comment #5:
Another way of thinking about it: When creating the linked list, we are pushing new elements onto the front of list, like a stack. So, the first element processed gets next=NULL, but it is still the last one in the list (stacks are FILO).
This may seem backwards, but it can keep you from having some weird behavior in threaded environments.
|
Fri 13 Jul 2007 06:50:29 PM UTC, comment #4:
You're right, my wording is off. Maybe I should just take a nap. The first memp->next, not memp, is set to NULL, to indicate the end of the list. The fact that the next points back in my mind, and not forward in the list throws me off.
|
Fri 13 Jul 2007 06:41:16 PM UTC, comment #3:
One of the comments is not correct, I think. You wrote:
> create a linked list of memp elements, marking the first element in the pool to be NULL, to allow us to detect pool exhaustion
If there are no elements for table i, then the memp_tab[i]=NULL. Otherwise, the for(j) loop runs, and memp_tab[i] points to a linked list of elements of size (MEMP_SIZE+memp_sizes[i]), where the last element in the list has its "next" set to NULL.
Otherwise, more comments are always good!
|
Fri 13 Jul 2007 06:24:27 PM UTC, comment #2:
Good catch. I attached the fix, with some commentary added to memp_init.
(file #13333)
|
Fri 13 Jul 2007 05:36:00 PM UTC, comment #1:
It returns NULL as long as MEMP_SIZE==0. But the return adds MEMP_SIZE to NULL and that is a bug. We can't add the "return NULL;" where you have it, because then SYS_ARCH_UNPROTECT doesn't happen.
/* Need to check for NULL in case MEMP_SIZE>0 */
return (memp ? ((void)((u8_t)memp + MEMP_SIZE)) : NULL);
}
|
Fri 13 Jul 2007 05:25:50 PM UTC, original submission:
If malloc fails, memp_malloc needs to return a NULL value, as many places in the code rely on this function.
Contents of the patch - also attached
+++ memp.c 2007-07-11 11:41:49.105656200 -0700
@@ -355,6 +355,7 @@
#if MEMP_STATS
++lwip_stats.memp[type].err;
#endif /* MEMP_STATS */
+ return NULL;
}
SYS_ARCH_UNPROTECT(old_level);
|