Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <max.kokryashkin@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] test: adapt test checking tail calls debug info
Date: Mon, 11 Oct 2021 18:03:51 +0300	[thread overview]
Message-ID: <YWRSVwrhha+hnfcw@root> (raw)
In-Reply-To: <20210930144056.1679061-1-m.kokryashkin@tarantool.org>

Hi, Maxim!

Thanks for the patch!
LGTM, except a few nits below.

On 30.09.21, Maxim Kokryashkin wrote:
> LuaJIT does not provide information about tail calls,
> unlike Lua does. getfenv() behavior for this test set is also different in
> LuaJIT, because tail calls do not provide additional call frame.
> 
> Closes tarantool/tarantool#5702
> Part of tarantool/tarantool#5845
> Part of tarantool/tarantool#4473

Looks like it should be 5870 instead 4473. Also, 5845 is already
closed.

> ---
> GitHub branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-5702-adapt-getfenv-getinfo-PUC-Rio
> Issue: https://github.com/tarantool/tarantool/issues/5702
> See also https://luajit.org/status.html
> 
>  test/PUC-Rio-Lua-5.1-tests/db.lua | 61 +++++++++++++------------------
>  1 file changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/test/PUC-Rio-Lua-5.1-tests/db.lua b/test/PUC-Rio-Lua-5.1-tests/db.lua
> index 56f59ea8..a94f07ab 100644
> --- a/test/PUC-Rio-Lua-5.1-tests/db.lua
> +++ b/test/PUC-Rio-Lua-5.1-tests/db.lua
> @@ -378,21 +378,22 @@ if jit_is_enabled then
>    jit.on()
>  end
>  
> -

Nit: this change is excess.

>  -- tests for tail calls
> +-- LuaJIT does not provide information about tail calls,
> +-- unlike Lua does. See also https://luajit.org/status.html.
> +-- getfenv() behavior is also different here, because tail calls
> +-- do not provide additional call frame for LuaJIT.
> +-- See also https://github.com/tarantool/tarantool/issues/5702.
> +-- This function is adapted to LuaJIT behavior.
>  local function f (x)
>    if x then
>      assert(debug.getinfo(1, "S").what == "Lua")
>      local tail = debug.getinfo(2)
> -    assert(not pcall(getfenv, 3))
> -    assert(tail.what == "tail" and tail.short_src == "(tail call)" and
> -           tail.linedefined == -1 and tail.func == nil)
> -    assert(debug.getinfo(3, "f").func == g1)
> +    assert(pcall(getfenv, 3))
> +    assert(tail.what == "Lua" and tail.linedefined == 403 and tail.func == g1)
> +    -- End of single assert

Don't get this comment.

>      assert(getfenv(3))
> -    assert(debug.getinfo(4, "S").what == "tail")
> -    assert(not pcall(getfenv, 5))
> -    assert(debug.getinfo(5, "S").what == "main")
> -    assert(getfenv(5))
> +    assert(debug.getinfo(3, "S").what == "main")
>      print"+"
>      end
>  end
> @@ -403,43 +404,33 @@ function g1(x) g(x) end
>  
>  local function h (x) local f=g1; return f(x) end
>  
> --- FIXME: LuaJIT does not provide information about tail calls,
> --- unlike Lua does. See also https://luajit.org/status.html.
> --- getfenv() behaviour is also different here, because tail calls
> --- do not provide additional call frame for LuaJIT and level
> --- number should be changed.
> --- Test is disabled for LuaJIT.
> --- See also https://github.com/tarantool/tarantool/issues/5702.
> --- h(true)
> +-- This fucntions is adapted to LuaJIT behavior. See the comment above.

Nit: This comment is excess IMHO: here nothing is changed.
Feel free to ignore.

> +h(true)
>  
>  local b = {}
> --- FIXME: Behavior is different for LuaJIT. See the comment above.
> --- Test is disabled for LuaJIT.
> --- debug.sethook(function (e) table.insert(b, e) end, "cr")
> --- h(false)
> --- debug.sethook()
> -local res = {"return",   -- first return (from sethook)
> -  "call", "call", "call", "call",
> -  "return", "tail return", "return", "tail return",
> -  "call",    -- last call (to sethook)
> +debug.sethook(function (e) table.insert(b, e) end, "cr")
> +-- This fucntions is adapted to LuaJIT behavior. See the comment above.
> +h(false)
> +debug.sethook()
> +-- This chunk is adapted to LuaJIT behavior. See the comment above.
> +local res = {"call",   -- first return (from sethook)
> +  "call", "call", "call", "return",
> +  "return", "call"
>  }
> --- FIXME: Behavior is different for LuaJIT. See the comment above.
> --- Test is disabled for LuaJIT.
> --- for _, k in ipairs(res) do assert(k == table.remove(b, 1)) end
> +for _, k in ipairs(res) do assert(k == table.remove(b, 1)) end
>  
>  
> -lim = 30000
> +-- This chunk is adapted to LuaJIT behavior. See the comment above.

Please mention the function nearby the comment as it was done before.
Here and above.

> +lim = 2
>  local function foo (x)
>    if x==0 then
> -    assert(debug.getinfo(lim+2).what == "main")
> -    for i=2,lim do assert(debug.getinfo(i, "S").what == "tail") end
> +    assert(debug.getinfo(lim + 1,"S").what == "main")
> +    assert(debug.getinfo(lim, "S").what == "main")
>    else return foo(x-1)
>    end
>  end
>  
> --- FIXME: Behavior is different for LuaJIT.
> --- See the comment to `h()` above. Test is disabled for LuaJIT.
> --- foo(lim)
> +foo(lim)
>  
>  
>  print"+"
> -- 
> 2.33.0
> 

-- 
Best regards,
Sergey Kaplun

  reply	other threads:[~2021-10-11 15:05 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-30 14:40 Maxim Kokryashkin via Tarantool-patches
2021-10-11 15:03 ` Sergey Kaplun via Tarantool-patches [this message]
2021-10-13 12:37   ` [Tarantool-patches] [PATCH luajit v2] " Maxim Kokryashkin via Tarantool-patches
2022-06-01 16:21     ` Igor Munkin via Tarantool-patches
2022-06-20 12:47 ` [Tarantool-patches] [PATCH luajit] " Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YWRSVwrhha+hnfcw@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] test: adapt test checking tail calls debug info' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox