jit X86 target compilation callback bug

Hi!

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.
We have fixed this problem by changing this instruction to "sub esp,
8" (8 because this function needs 2 temp 32bit variables). Are we
correct?

Thanks and let us know.

Hello

We are running llvm jit x86 on MS Visual Studio 2005.

Which version of llvm you're using?

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?

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.

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 :slight_smile:

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!