llvm dev - Feb 2010 - [LLVMdev] jit X86 target compilation callback bug

Hello again.

I still think that you are wrong. Realignement with and esp,-16 not
always changes stack poiner. If esp is already aligned to 16 byte
boundary, it will not change! Take a look at following example.
Assume esp has value 0x000001000 at start of X86CompilationCallback
function. Then execution of it will yield following esp values:

0x000000FFC - after push ebp
0x000000FFC - after mov ebp, esp
0x000000FF8 - after push eax
0x000000FF4 - after push edx // edx is pushed to address 0x000000FF4
0x000000FF0 - after push ecx // ecx is pushed to address 0x000000FF0
0x000000FF0 - after and esp, -16 // not changed!!!!

now next three instructions will corrupt pushed ecx and edx registers:

mov   eax, dword ptr [ebp+4]
mov   dword ptr [esp+4], eax
mov   dword ptr [esp], ebp

because [esp+4] - this is where pushed edx resides (0x000000FF0+4 = 0x000000FF4)
and [esp] is where pushed ecx resides (0x000000FF0)

instead of "and esp,-16" there would be "sub esp,8" then in
stack
there would be 8 bytes free to use (with [esp] and [esp+4]).
>
> What is "this" function?
>
By this I meant X86CompilationCallback function.

We are using 2.6 version. But same problem is in trunk also.


Kristaps.

On Tue, Feb 2, 2010 at 7:12 PM, Anton Korobeynikov
<anton at korobeynikov.info> wrote:
> Hello
>
>> We are running llvm jit x86 on MS Visual Studio 2005. It seems there
>> is a bug in asm code in function X86CompilationCallback in file
>> X86JITInfo.cpp. Current code sets stack pointer to invalid value in
>> instruction "and   esp,  16". Depending on current stack
pointer value
>> it sometimes overwrites ecx and edx registers with next three lines.
> How so? The stack grows downwards, thus this realignment just equivalent
> to "sub esp, some_value" with some_value being less than 16.
> We don't use register save area inside the compilation callback,
> however, since we're generating stubs "by hands".
>
>> We have fixed this problem by changing this instruction to "sub
 esp,
>> 8" (8 because this function needs 2 temp 32bit variables).
> What is "this" function?
>
> --
> With best regards, Anton Korobeynikov
> Faculty of Mathematics and Mechanics, Saint Petersburg State University
>

On 02/03/2010 01:08 PM, Kristaps Straupe wrote:
> Hello again.
>
> I still think that you are wrong. Realignement with and esp,-16 not
> always changes stack poiner. If esp is already aligned to 16 byte
> boundary, it will not change! Take a look at following example.
> Assume esp has value 0x000001000 at start of X86CompilationCallback
> function. Then execution of it will yield following esp values:
>
> 0x000000FFC - after push ebp
> 0x000000FFC - after mov ebp, esp
> 0x000000FF8 - after push eax
> 0x000000FF4 - after push edx // edx is pushed to address 0x000000FF4
> 0x000000FF0 - after push ecx // ecx is pushed to address 0x000000FF0
> 0x000000FF0 - after and esp, -16 // not changed!!!!
>
> now next three instructions will corrupt pushed ecx and edx registers:
>
> mov   eax, dword ptr [ebp+4]
> mov   dword ptr [esp+4], eax
> mov   dword ptr [esp], ebp
>
> because [esp+4] - this is where pushed edx resides (0x000000FF0+4 =
0x000000FF4)
> and [esp] is where pushed ecx resides (0x000000FF0)
>
> instead of "and esp,-16" there would be "sub esp,8"
then in stack
> there would be 8 bytes free to use (with [esp] and [esp+4]).
>   
The non-MSVC version of the code does a "sub esp, 16" (after "and
esp,
-16"):
#  if defined(__APPLE__)
    "andl    $-16, %esp\n"    // Align ESP on 16-byte boundary
#  endif
    "subl    $16, %esp\n"


That would be the correct thing to do on the MSVC version too I think.
"and esp, -16" aligns the stack, and "sub esp, -16" makes
room for 8
bytes + padding to keep the 16 byte alignment of
the stack.

This patch should fix the issue:
diff --git a/lib/Target/X86/X86JITInfo.cpp b/lib/Target/X86/X86JITInfo.cpp
index f363903..d297d24 100644
--- a/lib/Target/X86/X86JITInfo.cpp
+++ b/lib/Target/X86/X86JITInfo.cpp
@@ -297,6 +297,7 @@ extern "C" {
       push  edx
       push  ecx
       and   esp, -16
+      sub   esp, 16
       mov   eax, dword ptr [ebp+4]
       mov   dword ptr [esp+4], eax
       mov   dword ptr [esp], ebp


Best regards,
--Edwin

Hello, Everyone
> That would be the correct thing to do on the MSVC version too I think.
> "and esp, -16" aligns the stack, and "sub esp, -16"
makes room for 8
> bytes + padding to keep the 16 byte alignment of
> the stack.
Correct, it's funny why never saw this previously. I originally
thought about 64-bit stub. Kristaps, next time, please either submit a
diff, or tell the precise location info :)
> This patch should fix the issue:
> diff --git a/lib/Target/X86/X86JITInfo.cpp b/lib/Target/X86/X86JITInfo.cpp
> index f363903..d297d24 100644
Looks ok, please commit.

Thanks!
-- 
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University