[Lightning] 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

smime.p7s
Description: S/MIME cryptographic signature