Re: [Tarantool-patches] [PATCH luajit 4/4] ARM64: Fix xpcall() error case (really).

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi!

Thanks for the review!

On 02.06.21, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> 
> Just some updates to the message, LGTM.
> 
> Sergos
> 
> > On 24 May 2021, at 16:27, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > Thanks to François Perrad and Stefan Pejic.
> > 
> > (cherry picked from commit d417ded17945b4211608d497d50b509e0274f5e0)
> > 
> > Premature decrementing VM's RC register before switch to fff_fallback
> 	    decrement of
> 
> > handler during processing `xpcall()` fast function leads to incorrect
> > stack layout (not enough arguments on stack), when `xpcall()` calls
> > without a second argument or if it is not a function (see <301-basic.t>
> > test in lua-Harness test suite). While further error processing it leads
> > to incorrect error message, due to stack inconsistency.
> 
> Mention this test verifies the patch behavior.
> 
> > 
> > This patch stores intermediate result into TMP1 register (it does not
> > determine fallback's behaviour and there is no way to return from
> > fallback back to xpcall processing with spoiled TMP1) and moves RC
> > setting after possible switching to fallback handler.
> 		the switch            the
> 
> > 
> > Sergey Kaplun:
> > * added the description for the problem
> > 
> > Resolves tarantool/tarantool#6093
> > Part of tarantool/tarantool#5629
> > ---

The new commit message is:
Branch is force-pushed.

===================================================================
ARM64: Fix xpcall() error case (really).

Thanks to François Perrad and Stefan Pejic.

(cherry picked from commit d417ded17945b4211608d497d50b509e0274f5e0)

Premature decrement of VM's RC register before switch to fff_fallback
handler during processing `xpcall()` fast function leads to incorrect
stack layout (not enough arguments on stack), when `xpcall()` calls
without a second argument or if it is not a function (see
<test/lua-Harness-tests/301-basic.t:832>
test in lua-Harness test suite). While further error processing it leads
to incorrect error message, due to stack inconsistency.

This patch stores intermediate result into TMP1 register (it does not
determine fallback's behaviour and there is no way to return from
fallback back to xpcall processing with spoiled TMP1) and moves RC
setting after the switch to the fallback handler.

Sergey Kaplun:
* added the description for the problem

Resolves tarantool/tarantool#6093
Part of tarantool/tarantool#5629
===================================================================

> > src/vm_arm64.dasc | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
> > index e16a77ab..6e298255 100644
> > --- a/src/vm_arm64.dasc
> > +++ b/src/vm_arm64.dasc
> > @@ -1183,7 +1183,7 @@ static void build_subroutines(BuildCtx *ctx)
> >   |.ffunc xpcall
> >   |     ldp CARG1, CARG2, [BASE]
> >   |  ldrb TMP0w, GL->hookmask
> > -  |   subs NARGS8:RC, NARGS8:RC, #16
> > +  |   subs NARGS8:TMP1, NARGS8:RC, #16
> >   |   blo ->fff_fallback
> >   |    mov RB, BASE
> >   |     asr ITYPE, CARG2, #47
> > @@ -1191,6 +1191,7 @@ static void build_subroutines(BuildCtx *ctx)
> >   |     cmn ITYPE, #-LJ_TFUNC
> >   |  add PC, TMP0, #24+FRAME_PCALL
> >   |     bne ->fff_fallback		// Traceback must be a function.
> > +  |   mov NARGS8:RC, NARGS8:TMP1
> >   |    add BASE, BASE, #24
> >   |     stp CARG2, CARG1, [RB]		// Swap function and traceback.
> >   |   cbz NARGS8:RC, ->vm_call_dispatch
> > -- 
> > 2.31.0
> > 
> 

-- 
Best regards,
Sergey Kaplun