On Sat, Nov 8, 2008 at 10:40 AM, Peter Oberndorfer <kumbayo84@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!