Re: winedbg: resend: analyse far calls in be_i386_is_func_call
Jeff L wrote:
> Fixes some things Eric pointed out. Have not tested the far calls as I
> don't have anything that generates them.
>
> This patch came about when I was looking at why single stepping seemed
> to stuff up after a call. It breaks down the calls for 32 bit mode
> calls but not necessarily 16 and not 64 bit calls. It is a fairly messy
> area of knowledge and I could do with assistance as to how the 16/32/64
> bit modes work.
>
> Change log: Add code to analyse far calls in be_i386_is_func_call
> instead of only near calls.
>
> Jeff Latimer
a lot of things still look wrong to me...
- first of all, a lot of code should be factorized
- the callee should really be filled with the way the address of the
call is stored in the insn (for example, you return a modeflat for a
relative call from a 16 bit segment...)
- lots of computation for negative displacements are wrong
- segment is always expressed as an unsigned short (even in ADDRESS
structure), so you shouldn't convert it to an int...
- ...
A+
>
> Index: programs/winedbg/be_i386.c
> ===================================================================
> RCS file: /home/wine/wine/programs/winedbg/be_i386.c,v
> retrieving revision 1.6
> diff -u -r1.6 be_i386.c
> --- programs/winedbg/be_i386.c 21 Mar 2006 19:22:51 -0000 1.6
> +++ programs/winedbg/be_i386.c 8 Apr 2006 14:30:45 -0000
> @@ -380,28 +380,243 @@
>
> static unsigned be_i386_is_func_call(const void* insn, ADDRESS* callee)
> {
> +#define f_mod(byte) ((byte)>>6)
> +#define f_reg(byte) (((byte)>>3)&0x7)
> +#define f_rm(byte) ((byte)&0x7)
> BYTE ch;
> - int delta;
> + BYTE mod, reg, rm;
> + BYTE delta8;
> + short delta16;
> + int delta = 0;
> + int segment = 0;
> + unsigned short segment16;
>
> if (!dbg_read_memory(insn, &ch, sizeof(ch))) return FALSE;
> switch (ch)
> {
> - case 0xe8:
> - dbg_read_memory((const char*)insn + 1, &delta, sizeof(delta));
> -
> + case 0xe8: /* Call near, relative to next instruction */
> + callee->Mode = get_selector_type(dbg_curr_thread->handle, &dbg_context, dbg_context.SegCs);
> + if (callee->Mode == AddrMode1616) {
> + if (!dbg_read_memory((const char*)insn + 1, &delta16, sizeof(delta16)))
> + return FALSE;
> + delta = delta16; /* Align to 32 bits */
> + delta &= 0xffff;
in this case, the callee should be kept as AddrMode1616, not in flat mode.
you shouldn't either crop delta to a 16 bit value, especially it's wrong
when delta is negative
> + } else {
> + if (!dbg_read_memory((const char*)insn + 1, &delta, sizeof(delta)))
> + return FALSE;
> + }
> callee->Mode = AddrModeFlat;
> callee->Offset = (DWORD)insn;
> - be_i386_disasm_one_insn(callee, FALSE);
> + be_i386_disasm_one_insn(callee, FALSE);
> callee->Offset += delta;
>
> return TRUE;
> - case 0xff:
> +
> + case 0xff: /* Call far, absolute address */
> if (!dbg_read_memory((const char*)insn + 1, &ch, sizeof(ch)))
> return FALSE;
> - ch &= 0x38;
> - if (ch != 0x10 && ch != 0x18) return FALSE;
> - /* fall through */
> - case 0x9a:
> + rm = f_rm(ch); /* Extract ModR/M byte */
> + reg = f_reg(ch);
> + mod = f_mod(ch);
> + if (reg != 0x02 && reg != 0x03) return FALSE;
> + if (reg == 0x02) { /* Same segment */
> + switch(mod)
> + {
> + case 0x00: /* Register operand */
> + callee->Mode = AddrModeFlat;
> + switch (rm)
> + {
> + case 0x00:
> + delta = dbg_context.Ebx + dbg_context.Esi;
> + break;
> + case 0x01:
> + delta = dbg_context.Ebx + dbg_context.Edi;
> + break;
> + case 0x02:
> + delta = dbg_context.Ebp + dbg_context.Esi;
> + break;
> + case 0x03:
> + delta = dbg_context.Ebp + dbg_context.Edi;
> + break;
> + case 0x04:
> + delta = dbg_context.Esi;
> + break;
> + case 0x05:
> + delta = dbg_context.Edi;
> + break;
> + case 0x06:
> + if (!dbg_read_memory((const char*)insn + 2, &delta, sizeof(2))) /* disp16 */
> + return FALSE;
this is ugly. it'll work but, it would be cleaner to make it with
reading delta16 (as a short), then cast the short to an int
> + break;
> + case 0x07:
> + delta = dbg_context.Ebx;
> + break;
> + }
> + break;
> + case 0x01: /* Disp8 operand */
> + callee->Mode = AddrModeFlat;
> + if (!dbg_read_memory((const char*)insn + 2, &delta8, sizeof(delta8)))
> + return FALSE;
> + delta = delta8;
> + switch (rm)
> + {
> + case 0x00:
> + delta += dbg_context.Ebx + dbg_context.Esi;
> + break;
> + case 0x01:
> + delta += dbg_context.Ebx + dbg_context.Edi;
> + break;
> + case 0x02:
> + delta += dbg_context.Ebp + dbg_context.Esi;
> + break;
> + case 0x03:
> + delta += dbg_context.Ebp + dbg_context.Edi;
> + break;
> + case 0x04:
> + delta += dbg_context.Esi;
> + break;
> + case 0x05:
> + delta += dbg_context.Edi;
> + break;
> + case 0x06:
> + delta += dbg_context.Ebp;
> + break;
> + case 0x07:
> + delta += dbg_context.Ebx;
> + break;
> + }
> + break;
> + case 0x02: /* Disp16 or Disp32 bit operand */
> + callee->Mode = get_selector_type(dbg_curr_thread->handle, &dbg_context, dbg_context.SegCs);
> + if (callee->Mode == AddrMode1616) {
> + if (!dbg_read_memory((const char*)insn + 2, &delta16, sizeof(delta16)))
> + return FALSE;
> + delta = delta16; /* Align to 32 bits */
> + delta &= 0xffff;
> + } else {
> + if (!dbg_read_memory((const char*)insn + 2, &delta, sizeof(delta)))
> + return FALSE;
> + }
> + callee->Mode = AddrModeFlat;
> + switch (rm)
> + {
> + case 0x00:
> + delta += dbg_context.Ebx + dbg_context.Esi;
> + break;
> + case 0x01:
> + delta += dbg_context.Ebx + dbg_context.Edi;
> + break;
> + case 0x02:
> + delta += dbg_context.Ebp + dbg_context.Esi;
> + break;
> + case 0x03:
> + delta += dbg_context.Ebp + dbg_context.Edi;
> + break;
> + case 0x04:
> + delta += dbg_context.Esi;
> + break;
> + case 0x05:
> + delta += dbg_context.Edi;
> + break;
> + case 0x06:
> + delta += dbg_context.Ebp;
> + break;
> + case 0x07:
> + delta += dbg_context.Ebx;
> + break;
> + }
> + break;
> + case 0x03: /* Register operand */
> + callee->Mode = AddrModeFlat;
> + switch (rm)
> + {
> + case 0x00:
> + delta = dbg_context.Eax;
> + break;
> + case 0x01:
> + delta = dbg_context.Ecx;
> + break;
> + case 0x02:
> + delta = dbg_context.Edx;
> + break;
> + case 0x03:
> + delta = dbg_context.Ebx;
> + break;
> + case 0x04:
> + delta = dbg_context.Esp;
> + break;
> + case 0x05:
> + delta = dbg_context.Ebp;
> + break;
> + case 0x06:
> + delta = dbg_context.Esi;
> + break;
> + case 0x07:
> + delta = dbg_context.Edi;
> + break;
> + }
> + break;
> + default: /* Disp32 bit operand */
> + if (!dbg_read_memory((const char*)insn + 2, &delta, sizeof(delta)))
> + return FALSE;
> + break;
> + }
> +
> + callee->Offset = delta;
> +
> + return TRUE;
> + }
> + else if (reg == 0x03) /* Indirect Far call into other segment */
> + {
> + far char * faraddr;
you shouldn't need the far here (it brings nothing)
> + /* Extract the far address of the indirect address */
> + if (dbg_read_memory((const char*)insn + 2, &faraddr, sizeof(faraddr)))
> + return FALSE;
> + /* Extract the far address of the callee */
> + if (dbg_read_memory((const char*)faraddr + sizeof(delta), &segment16, sizeof(segment16)))
> + return FALSE;
> + segment = segment16;
> + callee->Mode = get_selector_type(dbg_curr_thread->handle, &dbg_context, (int) &segment);
> + if (callee->Mode == AddrMode1616) {
> + if (dbg_read_memory((const char*)faraddr, &delta16, sizeof(delta16)))
> + return FALSE;
> + delta = delta16; /* Align to 32 bits */
> + delta &= 0xffff;
> + } else {
> + if (dbg_read_memory((const char*)faraddr, &delta, sizeof(delta)))
> + return FALSE;
> + }
> + callee->Mode = AddrMode1632; /* We have made a 32 address so say so */
> + callee->Segment = segment;
> + callee->Offset = delta; /* absolute address not an offset */
> +
> + return TRUE;
> + }
> +
> + WINE_FIXME("Unsupported yet call insn (0x%02x) at %p\n", ch, insn);
> + return FALSE;
> +
> + case 0x9a: /* Call far, absolute address in operand */
> + if (dbg_read_memory((const char*)insn + 1 + sizeof(delta), &segment16, sizeof(segment16)))
> + return FALSE;
> + segment = segment16;
> + callee->Mode = get_selector_type(dbg_curr_thread->handle, &dbg_context, dbg_context.SegCs);
> + if (callee->Mode == AddrMode1616) {
> + if (!dbg_read_memory((const char*)insn + 1, &delta16, sizeof(delta16)))
> + return FALSE;
> + delta = delta16; /* Align to 32 bits */
> + delta &= 0xffff;
> + } else {
> + if (!dbg_read_memory((const char*)insn + 1, &delta, sizeof(delta)))
> + return FALSE;
> + }
> +
> + callee->Mode = AddrMode1632; /* We have made a 32 address so say so */
> + callee->Segment = segment;
> + callee->Offset = delta; /* absolute address not an offset */
> +
> + return TRUE;
> +
> case 0xCD:
> WINE_FIXME("Unsupported yet call insn (0x%02x) at %p\n", ch, insn);
> /* fall through */
>
>
> ------------------------------------------------------------------------
>
>
--
Eric Pouech
Eric Pouech wrote:
- first of all, a lot of code should be factorized
Can you give me an example of what factorised means here? I assume that you would like to see the common register code for mod 0, 1, 2 reduced. If that is the case, I thought that the variation in mod 0 rm 6 made combining it somewhat messy and that the code would execute faster by keeping sections separate. If that is not what you meant or I am wrong about the performance aspects, let me know.
- segment is always expressed as an unsigned short (even in ADDRESS structure), so you shouldn't convert it to an int...
My mistake, is it better to use unsigned short or WORD when specifying this?
- ...
A+
+ case 0xe8: /* Call near, relative to next instruction */ + callee->Mode = get_selector_type(dbg_curr_thread->handle, &dbg_context, dbg_context.SegCs);+ if (callee->Mode == AddrMode1616) { + if (!dbg_read_memory((const char*)insn + 1, &delta16, sizeof(delta16))) + return FALSE; + delta = delta16; /* Align to 32 bits */
in this case, the callee should be kept as AddrMode1616, not in flat mode.
What happens given that there is no segment filled in the callee structure? Happy to fix it but I thought that as the segment is not filled in it would be a flat mode.
+ return TRUE; + } + else if (reg == 0x03) /* Indirect Far call into other segment */ + { + far char * faraddr;
you shouldn't need the far here (it brings nothing)
My reading of the m16:32 was that I needed 6 bytes bytes for the address, ie it was a far address. Hence the far. Are you saying that the indirect address is actually a near address?
+ /* Extract the far address of the indirect address */ + if (dbg_read_memory((const char*)insn + 2, &faraddr, sizeof(faraddr))) + return FALSE; + /* Extract the far address of the callee */ + if (dbg_read_memory((const char*)faraddr + sizeof(delta), &segment, sizeof(segment))) + return FALSE;
Jeff Latimer
Jeff Latimer wrote:
Eric Pouech wrote:
- first of all, a lot of code should be factorized
Can you give me an example of what factorised means here? I assume that you would like to see the common register code for mod 0, 1, 2 reduced. If that is the case, I thought that the variation in mod 0 rm 6 made combining it somewhat messy and that the code would execute faster by keeping sections separate. If that is not what you meant or I am wrong about the performance aspects, let me know. yes
- segment is always expressed as an unsigned short (even in ADDRESS structure), so you shouldn't convert it to an int... My mistake, is it better to use unsigned short or WORD when specifying this? WORD
in this case, the callee should be kept as AddrMode1616, not in flat mode. What happens given that there is no segment filled in the callee structure? Happy to fix it but I thought that as the segment is not filled in it would be a flat mode. the callee must be fully filled by the function. if you're in 16bit mode, then the segment is current cs, and the offset if current IP (not EIP) + displacement
+ return TRUE; + } + else if (reg == 0x03) /* Indirect Far call into other segment */ + { + far char * faraddr;
you shouldn't need the far here (it brings nothing)
My reading of the m16:32 was that I needed 6 bytes bytes for the address, ie it was a far address. Hence the far. Are you saying that the indirect address is actually a near address?
yes, but you will never manipulate it directly as a far address (in sense of 16:32 entity). You'll do the job yourself to read the 16 and the 32 bit parts. -- Eric Pouech
participants (2)
-
Eric Pouech -
Jeff Latimer