[PATCH v3] mscoree: Implement VTable fixup for x86_64 architecture.

[Paul Gofman]

On 01/23/2016 04:52 AM, Sebastian Lackner wrote:
> On 22.01.2016 23:24, Paul Gofman wrote:
>> +    /* push %rcx; push %rdx; push %r8; push %r9 */
> I think it would be better to allocate a proper stack frame (pushq %rbp, movq %rsp, %rbp, subq ...,%rsp)
> and then use movq instructions.
I will change pushq to movq (copying regs over esp the same way as for
XMM). But why do you prefer to have a stack frame ('pushq %ebp; movq
%esp, %ebp')? I probably missing something but I do not see why it will
make anything better. Even without it I can see the thunk in a call
stack unwind on crash from the inside.

>> +    BYTE i1[6];
>> +    /*  sub $0x68, %rsp ; 0x10*4 + 0x20 + 0x8 (align stack for subsequent func call)
>> +        movups %xmm0,0x28(%esp); ...; movups %xmm5,0x58(%esp)
> Shouldn't it be %rsp ? Same for the precompiled assembler code below.
Shame on me, I shuffled around and stared into these bytes 100 times and
did not notice. Thank you for catching this and sorry you had to do it.
> Also, its theoretically safe to use movdqa like done in various other wine functions.
I could use aligned move (additionally taking care for moves alignment),
but I thought it would be just easier to debug if on initially unaligned
stack call it will crash in ReallyFixupVTable (or even not crash if it
will be aligning stack sometime in future). I can make aligned moves if
you think it is better, but we are really not gaining a performance from
it, it is not something that called often, but complicating logic a bit
requiring both aligned moves in a thunk and aligned stack on call to
ReallyFixupVTable. If to move aligned, do you have any specific prefence
to movdqa over movaps? I personally prefer movaps as it is what all the
compilers do nowadays (and mnemonic looks nicer) but of course can
change to movdqa.
>
>> +                    image, tokens[i], fixup->fixup->type);
> I would suggest to move those changes and some of the changes below into a separate patch.
> They can easily be reviewed separately and using PtrToUint() instead of pointer types wasn't
> even a good idea on 32-bit. ;)
 I can move 2 vars type changes, removing PtrToUint and TRACE away into
separate (but linked) patch.