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

[Sebastian Lackner]

On 23.01.2016 15:59, Paul Gofman wrote:
> 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.

In theory stack unwinding should not work correct with both solutions (yet),
The main reason I suggested it was that its probably faster, and that we
could implement stack alignment fixup easier later if it turns out to be
necessary.

> 
>>> +    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.

No problem, thats what the review is for ;)

>> 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 the function is called with wrong alignment, it would only bring us a tiny
bit further. The compiler feature for automatic x64 stack alignment fixup is
still pretty new, so we would have to implement our own version in assembler
anyway.

> 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.

I do not really have a preference, both opcodes are used in the Wine source.

>>
>>> +                    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.
> 
>