malloc(), realloc().. the dark side powerful it is

Confirmed here. On Andy's page it looks OK.

Looks like he has a fixed library, huh?

Message to more senior guys .... bug in memory allocation! Alert! How about releasing a fixed library? This happens on both 0022 and 1.0 of the IDE.

Actually, looking more closely, the test crashes the processor (that is, it doesn't complete it) on IDE 1.0.

On version 0022 it completes, like this:

0000 1594 1598 : used/free/large
0022 1572 1576 : used/free/large
...
0418 1176 1180 : used/free/large
0440 1154 1158 : used/free/large
freeing
0418 1176 1158 : used/free/large
0396 1198 1158 : used/free/large
...
0022 1572 1158 : used/free/large
0000 1594 1158 : used/free/large
allocating
0000 1594 1158 : used/free/large
0022 1572 1158 : used/free/large
...
0418 1176 1158 : used/free/large
0440 1154 1158 : used/free/large
freeing
0418 1176 1158 : used/free/large
0396 1198 1158 : used/free/large
...
0022 1572 1158 : used/free/large
0000 1594 1158 : used/free/large

The last figure(s) should be the same as the first, but isn't.

Oh, and it shouldn't crash.

Nick, come back to the light. Fix it with hardware I say.

Lefty

On my UNO with IDE 1.0 it finishes OK without crashing.

Tested under AVR Studio using WinAVR-20100110 (avr-libc 1.6.7) and the same output happens, the largest block is not grown at the end. Tested with native libc from Atmel (using AVR studio) and the output is correct. Maybe this is the bug fixed on avr-libc 1.7 ?

allocating
0000 1476 1476 : used/free/large
0022 1454 1454 : used/free/large
0044 1432 1432 : used/free/large
0066 1410 1410 : used/free/large
0088 1388 1388 : used/free/large
0110 1366 1366 : used/free/large
0132 1344 1344 : used/free/large
0154 1322 1322 : used/free/large
0176 1300 1300 : used/free/large
0198 1278 1278 : used/free/large
0220 1256 1256 : used/free/large
0242 1234 1234 : used/free/large
0264 1212 1212 : used/free/large
0286 1190 1190 : used/free/large
0308 1168 1168 : used/free/large
0330 1146 1146 : used/free/large
0352 1124 1124 : used/free/large
0374 1102 1102 : used/free/large
0396 1080 1080 : used/free/large
0418 1058 1058 : used/free/large
0440 1036 1036 : used/free/large
freeing
0418 1058 1036 : used/free/large
0396 1080 1036 : used/free/large
0374 1102 1036 : used/free/large
0352 1124 1036 : used/free/large
0330 1146 1036 : used/free/large
0308 1168 1036 : used/free/large
0286 1190 1036 : used/free/large
0264 1212 1036 : used/free/large
0242 1234 1036 : used/free/large
0220 1256 1036 : used/free/large
0198 1278 1036 : used/free/large
0176 1300 1036 : used/free/large
0154 1322 1036 : used/free/large
0132 1344 1036 : used/free/large
0110 1366 1036 : used/free/large
0088 1388 1036 : used/free/large
0066 1410 1036 : used/free/large
0044 1432 1036 : used/free/large
0022 1454 1036 : used/free/large
0000 1476 1476 : used/free/large
allocating
0000 1476 1476 : used/free/large
0022 1454 1454 : used/free/large
0044 1432 1432 : used/free/large
0066 1410 1410 : used/free/large
0088 1388 1388 : used/free/large
0110 1366 1366 : used/free/large
0132 1344 1344 : used/free/large
0154 1322 1322 : used/free/large
0176 1300 1300 : used/free/large
0198 1278 1278 : used/free/large
0220 1256 1256 : used/free/large
0242 1234 1234 : used/free/large
0264 1212 1212 : used/free/large
0286 1190 1190 : used/free/large
0308 1168 1168 : used/free/large
0330 1146 1146 : used/free/large
0352 1124 1124 : used/free/large
0374 1102 1102 : used/free/large
0396 1080 1080 : used/free/large
0418 1058 1058 : used/free/large
0440 1036 1036 : used/free/large
freeing
0418 1058 1036 : used/free/large
0396 1080 1036 : used/free/large
0374 1102 1058 : used/free/large
0352 1124 1058 : used/free/large
0330 1146 1058 : used/free/large
0308 1168 1058 : used/free/large
0286 1190 1058 : used/free/large
0264 1212 1058 : used/free/large
0242 1234 1058 : used/free/large
0220 1256 1058 : used/free/large
0198 1278 1058 : used/free/large
0176 1300 1058 : used/free/large
0154 1322 1058 : used/free/large
0132 1344 1058 : used/free/large
0110 1366 1058 : used/free/large
0088 1388 1058 : used/free/large
0066 1410 1124 : used/free/large
0044 1432 1300 : used/free/large
0022 1454 1300 : used/free/large
0000 1476 1476 : used/free/large

Well I got rid of the "crash". I added a gasp delay!

delay (1000);
  
// freeze the mcu until reset
 
  exit(0);

It appears that "freeze the MCU" was taken a bit literally, and the final few bytes were not output to the serial port.

retrolefty:
Nick, come back to the light. Fix it with hardware I say.

Lefty

In a moment, Lefty. :wink:

Right then.

Since I don't know how to fix the precompiled libraries, I've simply made my own versions of malloc/free/realloc from the avr-libc-1.7.1 source.

This can be downloaded from:

http://gammon.com.au/Arduino/mymalloc.c

Just incorporate that into your project (make a new tab in the IDE called mymalloc.c - NOT .cpp) and paste the file into it.

Then, add this to the start of your project:

extern "C" {
  void * mymalloc(size_t len);
  void myfree(void *p);
  void * myrealloc(void *ptr, size_t len);
}

Now change (or #define) malloc to mymalloc and so on. Then the test runs with everything going back to normal at the end.

Possibly all that needs changing is free, since I think that is the thing with the bug.

A caveat is that things like the String library will probably still pull in the old malloc/free. There is probably a smarter way of achieving this.

Another caveat. Taking a closer look, this version (taken from 1.7.1) does not seem to contain the patch alluded to in the previous page. Interesting.

So it seems that free() was rewritten somewhere in time.

I'm wondering which avr-libc wiring uses.. Andy had his uC to correctly free the memory blocks.

Personally I would prefer if the Arduino administrators/owners (whatever they are called) would roll out 1.1 of the IDE with this fix in it. Since the String library, supplied, uses malloc, and we keep hearing from people that their programs crash when using String, it might solve a few problems. And while they are at it, they could include placement new in new.cpp.

Just for fun, wiring uses avr-libc-1.7.1, libc.a dated from 2011/05/27.

I'll open a bug report for this topic at Arduino's Google Code page.

Bug report #857 open.

Active you have been. :slight_smile:

The source is strong in you.

The source in you is strong.

Well this is interesting. Adding this test function to the test:

void stest ()
  {
   String foo = "Hi there"; 
  }

And calling it like this:

 Serial.println ("String test ...");
 showmem();
 stest ();
 showmem ();

I get the results:

String test ...
0000 1619 1623 : used/free/large
0000 1619 1612 : used/free/large

So a simple usage of a single string has reduced the largest block.

Now with the new library:

String test ...
0000 1607 1611 : used/free/large
0000 1607 1611 : used/free/large

(Different base figures because one was tested under v0022 and one under 1.0 of the IDE).

I've worked out a reasonably simple way of retrofitting it. The bug seems to be in free, so we'll concentrate on that. I copied this:

struct __freelist {
	size_t          sz;
	struct __freelist *nx;
};

extern char     __heap_start;
extern char     __heap_end;
#define STACK_POINTER() ((char *)AVR_STACK_POINTER_REG)

extern char    *__brkval;	/* first location not yet allocated */
extern struct __freelist *__flp;/* freelist pointer (head of freelist) */
extern size_t   __malloc_margin;/* user-changeable before the first malloc() */
extern char    *__malloc_heap_start;
extern char    *__malloc_heap_end;

char           *__brkval;
struct __freelist *__flp;

void
myfree(void *p)
{
	struct __freelist *fp1, *fp2, *fpnew;
	char           *cp1, *cp2, *cpnew;
  
	/* ISO C says free(NULL) must be a no-op */
	if (p == 0)
		return;
  
	cpnew = p;
	cpnew -= sizeof(size_t);
	fpnew = (struct __freelist *) cpnew;
	fpnew->nx = 0;
  
	/*
	 * Trivial case first: if there's no freelist yet, our entry
	 * will be the only one on it.  If this is the last entry, we
	 * can reduce __brkval instead.
	 */
	if (__flp == 0) {
		if ((char *) p + fpnew->sz == __brkval)
			__brkval = cpnew;
		else
			__flp = fpnew;
		return;
	}
	/*
	 * Now, find the position where our new entry belongs onto the
	 * freelist.  Try to aggregate the chunk with adjacent chunks
	 * if possible.
	 */
	for (fp1 = __flp, fp2 = 0;
	     fp1;
	     fp2 = fp1, fp1 = fp1->nx) {
		if (fp1 < fpnew)
			continue;
		cp1 = (char *) fp1;
		fpnew->nx = fp1;
		if ((char *) &(fpnew->nx) + fpnew->sz == cp1) {
			/* upper chunk adjacent, assimilate it */
			fpnew->sz += fp1->sz + sizeof(size_t);
			fpnew->nx = fp1->nx;
		}
		if (fp2 == 0) {
			/* new head of freelist */
			__flp = fpnew;
			return;
		}
		break;
	}
	/*
	 * Note that we get here either if we hit the "break" above,
	 * or if we fell off the end of the loop.  The latter means
	 * we've got a new topmost chunk.  Either way, try aggregating
	 * with the lower chunk if possible.
	 */
	fp2->nx = fpnew;
	cp2 = (char *) &(fp2->nx);
	if (cp2 + fp2->sz == cpnew) {
		/* lower junk adjacent, merge */
		fp2->sz += fpnew->sz + sizeof(size_t);
		fp2->nx = fpnew->nx;
	}
	/*
	 * If there's a new topmost chunk, lower __brkval instead.
	 */
	for (fp1 = __flp, fp2 = 0;
	     fp1->nx != 0;
	     fp2 = fp1, fp1 = fp1->nx)
    /* advance to entry just before end of list */ ;
	cp2 = (char *) &(fp1->nx);
	if (cp2 + fp1->sz == __brkval) {
		if (fp2 == NULL)
			/* Freelist is empty now. */
			__flp = NULL;
		else
			fp2->nx = NULL;
		__brkval = cp2 - sizeof(size_t);
	}
}

into the tail-end of wiring.c (where else?).

Then since String (and presumably most things, such as Arduino.h) include stdlib.h I edited stdlib.h and added:

#define free myfree

This redirects String (and other places where you call free) to use myfree, which is now implemented inside wiring.c.

It's a bit of a hack, but this lets you get on with having a fixed memory allocator, until such time as a more proper way of fixing it is released.

Great work Nick, I'll use that workaround in the meanwhile.
Personally I would prefer to see the avr-libc updated rather than hacking on top of it to fix issues that were already corrected at mainstream.

Interesting, this could explain some memory boundary errors and/or allocation failures I can't find, but only when I send data to a terminal (which i don't need to do in my app). Still, it seems hard to believe that such a serious bug made it this far. Thanks for your work Nick...

It turns out that the existing Arduino tool chain (the compiler, linker and so on, but not including AVRDude) also has a bug that prevents it from properly generating code bigger than 128K bytes, which is a problem for the Mega Arduinos.

It looks like both this memory allocation issue and the flash size issue can be resolved by upgrading the Arduino tool chain to the latest. Instructions on how to do that are here:

I have not tried it yet, but its high on my priority list. I'll report back after I try it and run this test.

Nice work everyone, particularly Nick. I upgraded my libc version from 1.6.x to 1.7.x a long time ago when I recompiled avr-gcc up to 4.5.1 to get some other bug fixes and enhancements so yes, on my website you are seeing the results of 1.7.x free().

Thanks, Andy. For my part, thanks very much for porting the STL. I was starting to do it when I heard about your version. You saved me a lot of work and almost certainly did a better job. :slight_smile:

I love the STL, admittedly it doesn't get used every day on the Arduino, but that's a bit of a pity. I found recently that the string class uses somewhat less program memory than the String class, so it's a shame it isn't shipped with the system.