Re: gdb code review, pointer madness

NZG
2006-01-24T17:19:23Z

Intro:
This is a continuation of a thread attempting to patch gdb 61 to make it work
correctly with Eclipse's MI interface. The problem is that the Eclipse CDT
queries the frame list before it begins execution, which triggers a bug in
the m68k port of gdb and smashes it.

> I don't think there's anything wrong with the C code. That's a
> completely fundamental function in GDB, used everywhere, all the time.
> And it's hard for me to believe that both endiannesses aren't being
> exercised. If it were broken, we surely would have found out by now.
>
> If the function is behaving as you say, then I'd say your compiler has
> miscompiled GDB. Why don't you look through the assembly code for the
> function, and see if it's right?
It appears the assembly code for this function is correct (pasted below) but I
am no wizard at x68 assembly. If somebody else wants to take a gander it
would be appreciated.

I built it with
CC=gcc-3.3 ../gdb-6.1/configure --target=m68k-bdm-elf
make CC=ggc-3.3

after applying the bdm patch;
http://www.uclinux.org/pub/uClinux/m68k-elf-tools/gcc-3/uclinux-tools-20040603/m68k-bdm-1.3.0.tar.bz2
and replacing 2 files as detailed in:
http://mailman.uclinux.org/pipermail/uclinux-dev/2004-March/024885.html

I think the most likely culprit is some form of stack corruption (such as an
improperly dereferenced pointer) above it.

The faulty call is being made directly after a call to
frame_unwind_register (next_frame, M68K_FP_REGNUM, buf);
is made on the sentinal frame.

I highly suspect something is not being initialized correctly in the m68k
sentinal frame and that it's causing a dereference off into space from the
linked list of frames.
Tracking that down though.... has been a challenge.

Call leading up to insane extract_unsigned_integer call
*************************************************************
static struct m68k_frame_cache *
m68k_frame_cache (struct frame_info *next_frame, void **this_cache)
{
struct m68k_frame_cache *cache;
char buf[4];
int i;

if (*this_cache)
return *this_cache;

cache = m68k_alloc_frame_cache ();
*this_cache = cache;

/* In principle, for normal frames, %fp holds the frame pointer,
which holds the base address for the current stack frame.
However, for functions that don't need it, the frame pointer is
optional. For these "frameless" functions the frame pointer is
actually the frame pointer of the calling frame. Signal
trampolines are just a special case of a "frameless" function.
They (usually) share their frame pointer with the frame that was
in progress when the signal occurred. */

frame_unwind_register (next_frame, M68K_FP_REGNUM, buf);
cache->base = extract_unsigned_integer (buf, 4);
(returns with some crazy random number obtained from shifting and adding 8 buf
elements)

disassembly of extract_unsigned_integer
*************************************************************
00000110 <extract_unsigned_integer>:

ULONGEST
extract_unsigned_integer (const void *addr, int len)
{
110: 55 push %ebp
111: 89 e5 mov %esp,%ebp
113: 57 push %edi
114: 56 push %esi
115: 53 push %ebx
116: 83 ec 1c sub $0x1c,%esp
119: 8b 45 0c mov 0xc(%ebp),%eax
ULONGEST retval;
const unsigned char *p;
const unsigned char *startaddr = addr;
const unsigned char *endaddr = startaddr + len;
11c: 8b 55 08 mov 0x8(%ebp),%edx
11f: 01 c2 add %eax,%edx

if (len > (int) sizeof (ULONGEST))
121: 83 f8 08 cmp $0x8,%eax
124: 89 55 ec mov %edx,0xffffffec(%ebp)
127: 0f 8f 9b 00 00 00 jg 1c8 <extract_unsigned_integer+0xb8>
error ("\
That operation is not available on integers of more than %d bytes.",
(int) sizeof (ULONGEST));

/* Start at the most significant end of the integer, and work towards
the least significant. */
retval = 0;
if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
12d: a1 00 00 00 00 mov 0x0,%eax
132: 31 f6 xor %esi,%esi
134: 31 ff xor %edi,%edi
136: 89 04 24 mov %eax,(%esp)
139: e8 fc ff ff ff call 13a <extract_unsigned_integer+0x2a>
13e: 85 c0 test %eax,%eax
140: 75 4e jne 190 <extract_unsigned_integer+0x80>
{
for (p = startaddr; p < endaddr; ++p)
142: 8b 45 08 mov 0x8(%ebp),%eax
145: 8b 55 ec mov 0xffffffec(%ebp),%edx
148: 89 45 f0 mov %eax,0xfffffff0(%ebp)
14b: 39 d0 cmp %edx,%eax
14d: 73 31 jae 180 <extract_unsigned_integer+0x70>
14f: 90 nop
retval = (retval << 8) | *p;
150: 89 fb mov %edi,%ebx
152: 89 f1 mov %esi,%ecx
154: c1 e1 08 shl $0x8,%ecx
157: 0f a4 f3 08 shld $0x8,%esi,%ebx
15b: 31 d2 xor %edx,%edx
15d: 8b 75 f0 mov 0xfffffff0(%ebp),%esi
160: 89 df mov %ebx,%edi
162: 09 d7 or %edx,%edi
164: 0f b6 06 movzbl (%esi),%eax
167: 89 ce mov %ecx,%esi
169: ff 45 f0 incl 0xfffffff0(%ebp)
16c: 09 c6 or %eax,%esi
16e: 8b 45 ec mov 0xffffffec(%ebp),%eax
171: 39 45 f0 cmp %eax,0xfffffff0(%ebp)
174: 72 da jb 150 <extract_unsigned_integer+0x40>
176: 8d 76 00 lea 0x0(%esi),%esi
179: 8d bc 27 00 00 00 00 lea 0x0(%edi),%edi
}
else
{
for (p = endaddr - 1; p >= startaddr; --p)
retval = (retval << 8) | *p;
}
return retval;
}
180: 83 c4 1c add $0x1c,%esp
183: 89 f0 mov %esi,%eax
185: 89 fa mov %edi,%edx
187: 5b pop %ebx
188: 5e pop %esi
189: 5f pop %edi
18a: 5d pop %ebp
18b: c3 ret
18c: 8d 74 26 00 lea 0x0(%esi),%esi
190: 8b 55 ec mov 0xffffffec(%ebp),%edx
193: 8b 45 08 mov 0x8(%ebp),%eax
196: 4a dec %edx
197: 89 55 f0 mov %edx,0xfffffff0(%ebp)
19a: 39 c2 cmp %eax,%edx
19c: 72 e2 jb 180 <extract_unsigned_integer+0x70>
19e: 89 f6 mov %esi,%esi
1a0: 89 fb mov %edi,%ebx
1a2: 89 f1 mov %esi,%ecx
1a4: c1 e1 08 shl $0x8,%ecx
1a7: 0f a4 f3 08 shld $0x8,%esi,%ebx
1ab: 31 d2 xor %edx,%edx
1ad: 8b 75 f0 mov 0xfffffff0(%ebp),%esi
1b0: 89 df mov %ebx,%edi
1b2: 09 d7 or %edx,%edi
1b4: 0f b6 06 movzbl (%esi),%eax
1b7: 89 ce mov %ecx,%esi
1b9: ff 4d f0 decl 0xfffffff0(%ebp)
1bc: 09 c6 or %eax,%esi
1be: 8b 45 08 mov 0x8(%ebp),%eax
1c1: 39 45 f0 cmp %eax,0xfffffff0(%ebp)
1c4: 73 da jae 1a0 <extract_unsigned_integer+0x90>
1c6: eb b8 jmp 180 <extract_unsigned_integer+0x70>
1c8: c7 04 24 00 00 00 00 movl $0x0,(%esp)
1cf: ba 08 00 00 00 mov $0x8,%edx
1d4: 89 54 24 04 mov %edx,0x4(%esp)
1d8: e8 fc ff ff ff call 1d9 <extract_unsigned_integer+0xc9>
1dd: 8d 76 00 lea 0x0(%esi),%esi