Stack frame alignment bug on MacOS

[Laurent Michel]

Sorry for the stream of bug reports...

I have found another issue with stack alignment on Apple. Even in 32-bit mode, MacOS requires frames to be 16 byte aligned. I noticed that the code already contained some provision for this, but the prologue of a function is still not aligning correctly. 

The macros affected by the change are:

jit_base_prolog(), jit_prolog and jit_ret.

Here is the modified version:

#define jit_base_prolog() (PUSHLr(_EBX), PUSHLr(_ESI), PUSHLr(_EDI), PUSHLr(_EBP), MOVLrr(_ESP, _EBP),\

                           jit_subi_i(JIT_SP,JIT_SP,12))

#define jit_prolog(n) (_jitl.framesize = 20, _jitl.alloca_offset = 0,\

                       jit_base_prolog())

#define jit_ret() ((_jitl.alloca_offset < 0 ? LEAVE_() : jit_addi_i(JIT_SP,JIT_SP,12),POPLr(_EBP)), POPLr(_EDI), POPLr(_ESI), POPLr(_EBX), RET_())

The rationale is as follow:

The frame is aligned before the call (The SP (esp) is a multiple of 16). The call pushes the return address on the stack (4b). 

Then, the prolog pushes 4 registers, i.e., another 16 bytes. So the code correctly sets the framesize to 20 bytes. But that is not 16 bytes aligned. One must further increase esp by 12 byte to maintain its alignment (and decrease it correspondingly in the return).

So I added a jit_subi_i(esp,esp,12)   and modified jit_ret accordingly. 

The frame pointer (ebp) does not need to be aligned. Just esp. 

With this change my code works fine (it alternates call to C++ and lightning generated code, the bug would be invisible if C++ was calling lightning and lightning never called anything else --besides lightning code--. However since my C++ code call lightning code and the lightning code calls C++ and therefore ends up calling MacOS routine, the alignment is critical for correctness.

Based on how framesize is used in the rest of the code, I gather that it should not be changed as 20 is the offset from the new frame pointer to the first formal). 

I hope you consider wrapping this into the official version!

THanks!

PS/ My correction aligns on 16 bytes on all architecture. Note that x86_64 requires 16 bytes alignment anyway, so this "wastes" a little bit of space only on Linux i386 and nowhere else. In my case, I don't consider the 12 bytes anything I would loose sleep over, but that's your call.

PPS/ I can push the changes to git if you wish but I won't until you say it's ok (don't even know if i have write privileges).

--

  Laurent

_______________________________________________
Lightning mailing list
Lightning@...
http://lists.gnu.org/mailman/listinfo/lightning

[Paolo Bonzini-2]

> PS/ My correction aligns on 16 bytes on all architecture. Note that
> x86_64 requires 16 bytes alignment anyway, so this "wastes" a little bit
> of space only on Linux i386 and nowhere else. In my case, I don't
> consider the 12 bytes anything I would loose sleep over, but that's your
> call.

Can you try this patch instead?  (And maybe you have a testcase to
include?...)  If it works, feel free to write a ChangeLog and push it.

> PPS/ I can push the changes to git if you wish but I won't until you say
> it's ok (don't even know if i have write privileges).

I think so.  You had CVS, so you can access git too.

Paolo

diff --git a/lightning/i386/core-32.h b/lightning/i386/core-32.h
index eba34b9..8e2cc5a 100644
--- a/lightning/i386/core-32.h
+++ b/lightning/i386/core-32.h
@@ -46,8 +46,11 @@ struct jit_local_state {
   int alloca_slack;
 };
 
-#define jit_base_prolog() (PUSHLr(_EBX), PUSHLr(_ESI), PUSHLr(_EDI), PUSHLr(_EBP), MOVLrr(_ESP, _EBP))
-#define jit_prolog(n) (_jitl.framesize = 20, _jitl.alloca_offset = 0, jit_base_prolog())
+#define jit_base_prolog() (_jitl.framesize = 20, _jitl.alloca_offset = 0, \
+  PUSHLr(_EBX), PUSHLr(_ESI), PUSHLr(_EDI), PUSHLr(_EBP), MOVLrr(_ESP, _EBP))
+#define jit_base_ret(ofs)  \
+  (((ofs) < 0 ? LEAVE_() : POPLr(_EBP)),  \
+   POPLr(_EDI), POPLr(_ESI), POPLr(_EBX), RET_())
 
 /* Used internally.  SLACK is used by the Darwin ABI which keeps the stack
    aligned to 16-bytes.  */
@@ -78,11 +81,17 @@ struct jit_local_state {
 #define jit_allocai(n) \
   jit_allocai_internal ((n), (_jitl.alloca_slack - (n)) & 15)
 
+#define jit_prolog(n) (jit_base_prolog(), jit_subi (JIT_SP, JIT_SP, 12))
+#define jit_ret() jit_base_ret (-12)
+
 #else
 # define jit_prepare_i(ni) (_jitl.argssize += (ni))
 
 #define jit_allocai(n) \
   jit_allocai_internal ((n), 0)
+
+#define jit_prolog(n) jit_base_prolog()
+#define jit_ret() jit_base_ret (_jitl.alloca_offset)
 #endif
 
 #define jit_calli(label) (CALLm( ((unsigned long) (label))), _jit.x.pc)
@@ -105,7 +114,6 @@ struct jit_local_state {
 #define jit_movi_p(d, is)       (jit_movi_l(d, ((long)(is))), _jit.x.pc)
 #define jit_patch_long_at(jump_pc,v)  (*_PSL((jump_pc) - sizeof(long)) = _jit_SL((jit_insn *)(v) - (jump_pc)))
 #define jit_patch_at(jump_pc,v)  jit_patch_long_at(jump_pc, v)
-#define jit_ret() ((_jitl.alloca_offset < 0 ? LEAVE_() : POPLr(_EBP)), POPLr(_EDI), POPLr(_ESI), POPLr(_EBX), RET_())
 
 /* Memory */
 

_______________________________________________
Lightning mailing list
Lightning@...
http://lists.gnu.org/mailman/listinfo/lightning

[Laurent Michel]

On Jun 10, 2008, at 8:12 PM, Paolo Bonzini wrote:

PS/ My correction aligns on 16 bytes on all architecture. Note that x86_64 requires 16 bytes alignment anyway, so this "wastes" a little bit of space only on Linux i386 and nowhere else. In my case, I don't consider the 12 bytes anything I would loose sleep over, but that's your call.

Can you try this patch instead?  (And maybe you have a testcase to include?...)  If it works, feel free to write a ChangeLog and push it.

PPS/ I can push the changes to git if you wish but I won't until you say it's ok (don't even know if i have write privileges).

I think so.  You had CVS, so you can access git too.

Thanks Paolo. 

I'll try the patch for sure and will commit once I confirm everything's fine.  For the testcase, these are the comet programs I write :-(. I'll try to extract the essence of the test to make a standalone version though. I have ~300 (COMET)programs in my testsuite and right now many appear to be broken when I enable the JIT. I guess that one or two issues can wreak havoc. I'll work on those issues in the next couple of days. My objective is to get back to a fully functional code base with the head version from git in 32bit mode. Once that is done, I'll attack the x86_64 (that was the plan from the beginning). So chances are I'll send a few more emails ;-)

Did you get my message regarding how to cleanly compile the 64 bit version on MacOS? I had to hack a little to get it going, but I am not familiar at all with writing autoconf things, so I was hoping I missed something. 

--

  Laurent

_______________________________________________
Lightning mailing list
Lightning@...
http://lists.gnu.org/mailman/listinfo/lightning

[Paolo Bonzini-2]

> I guess that one or two  issues can wreak havoc.

Yes, that's my experience too.

> Did you get my message regarding how to cleanly compile the 64 bit
> version on MacOS? I had to hack a little to get it going, but I am not
> familiar at all with writing autoconf things, so I was hoping I missed
> something.

Yes, I hadn't really read it carefully yet, but it's on the todo list.

Paolo


_______________________________________________
Lightning mailing list
Lightning@...
http://lists.gnu.org/mailman/listinfo/lightning