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.