On Wed, Feb 29, 2012 at 5:31 PM, Carl-Daniel Hailfinger <[email protected]> wrote: > Am 29.02.2012 08:39 schrieb Patrick Georgi: >> Am 28.02.2012 23:06, schrieb Marc Jones: >>> I found this bug building tint with libpayload. libpayload is built >>> with defconfig and using the same coreboot crosstools gcc. The bug >>> happens in the first call to alloc() when the first header of the >>> first region is installed. The header memory location is checked, >>> found to be 0, and then loaded with the header. The bug is that the >>> original value of the location is used after the memory was updated. >>> It should have been reloaded. It is pretty easy to see in the >>> disassembly below. >> workaround: mark setup() __attribute__((noinline)) >> >> The proper fix is to clean up the various casts so the aliasing based >> optimizations in gcc do the right thing. > > Can't you use __attribute__((may_alias)) for the affected variables? > > Regards, > Carl-Daniel > > -- > http://www.hailfinger.org/ >
i think that I have resolved the issue by making the pointers volatile. I wanted to get some comments here before I push it to gerrit. The asm generated: 10b7d7: 8b 15 a0 5f 11 00 mov 0x115fa0,%edx 10b7dd: 81 e2 00 00 00 a8 and $0xa8000000,%edx 10b7e3: 81 fa 00 00 00 a8 cmp $0xa8000000,%edx 10b7e9: 74 0f je 10b7fa <alloc+0x3a> 10b7eb: 25 fc ff ff 00 and $0xfffffc,%eax 10b7f0: 0d 00 00 00 aa or $0xaa000000,%eax 10b7f5: a3 a0 5f 11 00 mov %eax,0x115fa0 10b7fa: b8 a0 5f 11 00 mov $0x115fa0,%eax 10b7ff: 90 nop 10b800: 8b 10 mov (%eax),%edx 10b802: 89 d1 mov %edx,%ecx Marc -- http://se-eng.com
From 74a08410bf0c4f1c595928ec679cd7f11c90310c Mon Sep 17 00:00:00 2001 From: Marc Jones <[email protected]> Date: Thu, 1 Mar 2012 16:12:11 -0700 Subject: [PATCH] Make libpaylod alloc() memory pointers volatile gcc4.6.2 was optimizing the libpayload alloc() function and failing to reload a pointer after the memory had been manipulated by a pointer in the inlined function setup(). I changes the pointer type to volatile and now pass it to the setup() function. I also cleaned up the declaration so that it wasn't cast 10 times in the function. Change-Id: I1637bd7bd5d9cf82ac88925cbfe76d319aa3cd82 Signed-off-by: Marc Jones <[email protected]> --- payloads/libpayload/libc/malloc.c | 25 +++++++++++-------------- 1 files changed, 11 insertions(+), 14 deletions(-) diff --git a/payloads/libpayload/libc/malloc.c b/payloads/libpayload/libc/malloc.c index 6389fc9..9412cab 100644 --- a/payloads/libpayload/libc/malloc.c +++ b/payloads/libpayload/libc/malloc.c @@ -73,11 +73,9 @@ static int heap_initialized = 0; static int minimal_free = 0; #endif -static void setup(void) +static void setup(hdrtype_t volatile *start, int size) { - int size = (unsigned int)(&_eheap - &_heap) - HDRSIZE; - - *((hdrtype_t *) hstart) = FREE_BLOCK(size); + *start = FREE_BLOCK(size); #ifdef CONFIG_DEBUG_MALLOC heap_initialized = 1; @@ -88,7 +86,7 @@ static void setup(void) static void *alloc(int len) { hdrtype_t header; - void *ptr = hstart; + hdrtype_t volatile *ptr = (hdrtype_t volatile *) hstart; /* Align the size. */ len = (len + 3) & ~3; @@ -97,12 +95,12 @@ static void *alloc(int len) return (void *)NULL; /* Make sure the region is setup correctly. */ - if (!HAS_MAGIC(*((hdrtype_t *) ptr))) - setup(); + if (!HAS_MAGIC(*ptr)) + setup(ptr, len); /* Find some free space. */ do { - header = *((hdrtype_t *) ptr); + header = *ptr; int size = SIZE(header); if (!HAS_MAGIC(header) || size == 0) { @@ -114,7 +112,7 @@ static void *alloc(int len) if (header & FLAG_FREE) { if (len <= size) { - void *nptr = ptr + (HDRSIZE + len); + hdrtype_t volatile *nptr = ptr + (HDRSIZE + len); int nsize = size - (HDRSIZE + len); /* If there is still room in this block, @@ -124,14 +122,13 @@ static void *alloc(int len) if (nsize > 0) { /* Mark the block as used. */ - *((hdrtype_t *) ptr) = USED_BLOCK(len); + *ptr = USED_BLOCK(len); /* Create a new free block. */ - *((hdrtype_t *) nptr) = - FREE_BLOCK(nsize); + *nptr = FREE_BLOCK(nsize); } else { /* Mark the block as used. */ - *((hdrtype_t *) ptr) = USED_BLOCK(size); + *ptr = USED_BLOCK(size); } return (void *)(ptr + HDRSIZE); @@ -140,7 +137,7 @@ static void *alloc(int len) ptr += HDRSIZE + size; - } while (ptr < hend); + } while (ptr < (hdrtype_t *) hend); /* Nothing available. */ return (void *)NULL; -- 1.7.1
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
