Re: winedbg: resend: analyse far calls in be_i386_is_func_call

[Eric Pouech]

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 */

winedbg: resend: analyse far calls in be_i386_is_func_call

[Jeff Latimer]

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

winedbg: resend: analyse far calls in be_i386_is_func_call

[Eric Pouech]

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.