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.
Patrick
On Wed, Feb 29, 2012 at 12:39 AM, Patrick Georgi patrick@georgi-clan.de wrote:
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 you expand on this? Do you mean that we should change do something like this?
hdrtype_t *ptr = hstart;
Marc
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
On Wed, Feb 29, 2012 at 5:31 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net 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
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
-
Carl-Daniel Hailfinger -
Marc Jones -
Patrick Georgi