Re: [1/2] richedit: Created initial tests for windowless richedit controls. (try 2)

[Peter Oberndorfer]

On Samstag 08 November 2008, Dylan Smith wrote:
> (Ensured compiler independence by modifying byte code templates)
>
> The initial tests added simply call CreateTextServices, and query for
> the ITextServices interface, then cleans up.
>
> Since CreateTextServices needs an implementation of ITextHost to be
> given, this patch needed a stub implementation.  This required wrappers
> for converting thiscall to stdcall which are now implementing by
> adapting a byte code wrapper template.  Although byte code isn't portable
> across processors, this is only needed for x86 platforms so it is only
> meant to be compiler independent.
>
> In order to investigate what is happening in the tests, it is helpful
> to trace the calls to ITextHost methods by the richedit controls.  I
> realized it would be too verbose to trace all these calls by default,
> so I wrapped them all with a check for winedebug > 1, since normally
> winedebug is 0 or 1.
> ---
>  dlls/riched20/tests/Makefile.in |    3 +-
>  dlls/riched20/tests/txtsrv.c    |  593 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 595 insertions(+), 1 deletions(-)
>  create mode 100644 dlls/riched20/tests/txtsrv.c

Hi,

This page [1] seems to suggest memory returned from HeapAlloc
is marked as not executable.
Other tests (ntdll exception) are using VirtualAlloc()
for dynamically created code.

patch 1
> +/* The following x86 code string converts between the thiscall and stdcall
> + * calling convetions.  The thiscall calling convention places the This
> + * pointer in ecx on the x86 platform, and the stdcall calling convention
> + * pushes the This pointer on the stack as the first argument.
> + *
> + * The wrapper's code finishes by jumping to the real function.
> + *
> + * Byte codes are used so that a copy of it can be modified to use
> + * for each method in ITextHost. */

patch 2
> -/* The following x86 code string converts between the thiscall and stdcall
> +/* The following x86 code strings convert between the thiscall and stdcall
>   * calling convetions.  The thiscall calling convention places the This

Here you introduce comments that you seem to change in the second patch.
Maybe use the right text in the first patch?
Additionally the term code string sounds odd.
Maybe replace that with "The following x86 assembler code..."
or something similar?
> #define ITextServices_QueryInterface(p,a,b) (p)->lpVtbl->QueryInterface(p,a,b)
>  #define ITextServices_AddRef(p) (p)->lpVtbl->AddRef(p)
>  #define ITextServices_Release(p) (p)->lpVtbl->Release(p)
> -/*** ITextServices methods ***/
> -#define ITextServices_TxSendMessage(p,a,b,c,d) (p)->lpVtbl->TxSendMessage(p,a,b,c,d)
> -#define ITextServices_TxDraw(p,a,b,c,d,e,f,g,h,i,j,k,l) (p)->lpVtbl->TxDraw(p,a,b,c,d,e,f,g,h,i,j,k,l)

Why do you only move some of those #defines part of ITextServices
and not the one part of IUnknown ?
I do not know much about those #defines but this seems odd


Maybe i am missing something but why do you need those thiscall wrappers?
Wouldn't it be possible to use
the DEFINE_THISCALL_WRAPPER and THISCALL macros like in txtserv.c?

Greetings Peter

[1] http://www.unixwiz.net/techtips/xp-sp2.html

[1/2] richedit: Created initial tests for windowless richedit controls. (try 2)

[Dylan Smith]

On Sat, Nov 8, 2008 at 10:40 AM, Peter Oberndorfer <kumbayo84(a)arcor.de>wrote:
> This page [1] seems to suggest memory returned from HeapAlloc
> is marked as not executable.
> Other tests (ntdll exception) are using VirtualAlloc()
> for dynamically created code.
>
> patch 1
> > +/* The following x86 code string converts between the thiscall and
> > stdcall
> > + * calling convetions.  The thiscall calling convention places the This
> > + * pointer in ecx on the x86 platform, and the stdcall calling
> > convention
> > + * pushes the This pointer on the stack as the first argument.
> > + *
> > + * The wrapper's code finishes by jumping to the real function.
> > + *
> > + * Byte codes are used so that a copy of it can be modified to use
> > + * for each method in ITextHost. */

Thanks, I forgot to address that issue because I was too busy trying to get
it to run correctly.  I used HeapAlloc initially because I wasn't familiar
with VirtualAlloc, so thanks for letting me know what to use.
> patch 2
> > -/* The following x86 code string converts between the thiscall and
> > stdcall
> > +/* The following x86 code strings convert between the thiscall and
> > stdcall
> >   * calling convetions.  The thiscall calling convention places the This
>
> Here you introduce comments that you seem to change in the second patch.
> Maybe use the right text in the first patch?
> Additionally the term code string sounds odd.
> Maybe replace that with "The following x86 assembler code..."
> or something similar?

All I changed in the comments between the patches was singular to plural
word changes.  Using the same comments for both patches would make the
comment confusing if only one of the patches were accepted.

I could change code string, but probably to "The following x86 byte
code...", since it is just the comments that are assembly.
> > #define ITextServices_QueryInterface(p,a,b)
> > (p)->lpVtbl->QueryInterface(p,a,b)
> >  #define ITextServices_AddRef(p) (p)->lpVtbl->AddRef(p)
> >  #define ITextServices_Release(p) (p)->lpVtbl->Release(p)
> > -/*** ITextServices methods ***/
> > -#define ITextServices_TxSendMessage(p,a,b,c,d)
> > (p)->lpVtbl->TxSendMessage(p,a,b,c,d)
> > -#define ITextServices_TxDraw(p,a,b,c,d,e,f,g,h,i,j,k,l)
> > (p)->lpVtbl->TxDraw(p,a,b,c,d,e,f,g,h,i,j,k,l)
>
> Why do you only move some of those #defines part of ITextServices
> and not the one part of IUnknown ?
> I do not know much about those #defines but this seems odd

The QueryInterface, AddRef, and Release methods were actually defined using
the stdcall calling convention, so it doesn't have the same problems as the
other methods which were defined using the C++ thiscall calling convention.
> Maybe i am missing something but why do you need those thiscall wrappers?
> Wouldn't it be possible to use
> the DEFINE_THISCALL_WRAPPER and THISCALL macros like in txtserv.c?

I can't use DEFINE_THISCALL_WRAPPER  because it uses __ASM_GLOBAL_FUNC,
__ASM_FUNC, and __ASM_NAME, which are defined in include/wine/port.h and
include/config.h.  These cannot be included in the tests since the tests
must be compiled and crosscompiled with the same configuration (and
include/config.h is generated by the configure script).  Previously I tried
to do it that way, but it wasn't commited I tried to define these macros in
the test without making it portable enough to other compilers.
> Greetings Peter

Thanks for all the helpful comments Peter!