

<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16880379231397807765_BODY">Hi, Maxim!<br>Thanks for the review!<br>Fixed your comments and force-pushed the branch.<br><br>On 29.06.23, Maxim Kokryashkin wrote:<br>><br>> Hi, Sergey!<br>> LGTM, except for a few comments and the single note below.<br>> > <br>> >>From: Mike Pall <mike><br>> >><br>> >>Reported by Jason Carr.<br>> >><br>> >>(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)<br>> >><br>> >>This commit is the follow up for the previous commit ("Fix lua_yield()<br>> >>from C hook."). In GC64 mode stack slot for a GC thread object is still<br>> >>miscalculated during creating a continuation frame for `lua_yield()`<br>> >Typo: s/during creating/during the creation of/<br><br>Fixed.<br><br>> >>.This happens due to tricky usage of the previous slot instead of the<br>> >Typo: s/due to/due to the/<br><br>Fixed.<br><br>> >>given one in `setframe_gc()` macro.<br>> >><br>> >>This patch changes the semantics of `setframe_gc()` macro to use the<br>> >>given as argument slot as the destination to store GC value. Also, it<br>> >Typo: s/the given as argument slot/the slot given as argument/<br><br>Fixed.<br><br>> >>fixups all usages of this macro to match new semantics.<br>> >Typo: s/new/the new/<br><br>Fixed.<br><br>> > <br>> >Also, I strongly suggest to distinct the changes that directly relate<br>> >to the issue, from the semantic fixups related to the macro update.<br>> >An additional sentence or a list in the commit message would be enough.<br><br>I mention all places with changed semantics to avoid misunderstanding.<br>The commit message looks like the following:<br><br>===================================================================<br>Another fix for lua_yield() from C hook.<br><br>Reported by Jason Carr.<br><br>(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)<br><br>This commit is the follow up for the previous commit ("Fix lua_yield()<br>from C hook."). In GC64 mode stack slot for a GC thread object is still<br>miscalculated during the creation of a continuation frame for<br>`lua_yield()`. This happens due to the tricky usage of the previous slot<br>instead of the given one in `setframe_gc()` macro.<br><br>This patch changes the semantics of `setframe_gc()` macro to use the<br>slot given as argument as the destination to store GC value. Also, it<br>fixups all usages of this macro to match the new semantics, i.e. in:<br>* <src/lj_ccallback.c><br>* <src/lj_err.c><br>* <src/lj_meta.c><br><br>Sergey Kaplun:<br>* added the description for the problem<br><br>Part of tarantool/tarantool#8516<br>===================================================================<br><br>> >><br>> >>Sergey Kaplun:<br>> >>* added the description for the problem<br>> >><br>> >>Part of tarantool/tarantool#8516<br>> >>---<br>> >> src/lj_ccallback.c | 2 +-<br>> >> src/lj_err.c | 2 +-<br>> >> src/lj_frame.h | 2 +-<br>> >> src/lj_meta.c | 2 +-<br>> >> test/tarantool-c-tests/fix-yield-c-hook.test.c | 4 ----<br>> >> 5 files changed, 4 insertions(+), 8 deletions(-)<br>> >><br><br><snipped><br><br>> >--<br>> >Best regards,<br>> >Maxim Kokryashkin<br>> > <br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>