[Tarantool-patches] [PATCH luajit 0/3] Follow-up fixes for full 64-bit lightuserdata interning

[Tarantool-patches] [PATCH luajit 0/3] Follow-up fixes for full 64-bit lightuserdata interning

[Sergey Kaplun via Tarantool-patches]

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-727-lightuserdata-itern
Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/lj-727-lightuserdata-itern
Issue: https://github.com/LuaJIT/LuaJIT/issues/727

The first commit (test-related) is needed to fix test behaviour for
is_deeply function for userdata object.
The second commit (refactoring) is needed to simplify cherry-pick of the
next one (and the last one) without conflicts.

Mike Pall (2):
  Reorganize lightuserdata interning code.
  Avoid conflict between 64 bit lightuserdata and ITERN key.

Sergey Kaplun (1):
  test: fix path storage for non-concatable objects

 src/lj_api.c                                  | 30 +--------
 src/lj_udata.c                                | 28 +++++++++
 src/lj_udata.h                                |  3 +
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../lj-727-lightuserdata-itern.test.lua       | 48 ++++++++++++++
 .../lj-727-lightuserdata-itern/CMakeLists.txt |  1 +
 .../lightuserdata.c                           | 63 +++++++++++++++++++
 test/tarantool-tests/tap.lua                  |  4 +-
 8 files changed, 148 insertions(+), 30 deletions(-)
 create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
 create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c

-- 
2.31.0

[Tarantool-patches] [PATCH luajit 1/3] test: fix path storage for non-concatable objects

[Sergey Kaplun via Tarantool-patches]

When the key of a table to compare via `tap.test:is_deeply()` is
non-concatable object (i.e. lightuserdata) concatenation with path
raises an error.

This patch converts object to string to avoid this error.

Needed for tarantool/tarantool#5629
---
 test/tarantool-tests/tap.lua | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
index 44fcac3d..a1f54d20 100644
--- a/test/tarantool-tests/tap.lua
+++ b/test/tarantool-tests/tap.lua
@@ -166,7 +166,7 @@ local function is_deeply(test, got, expected, message, extra)
 
     for k, v in pairs(got) do
       has[k] = true
-      extra.path = path .. "." .. k
+      extra.path = path .. "." .. tostring(k)
       if not cmpdeeply(v, expected[k], extra) then
         return false
       end
@@ -175,7 +175,7 @@ local function is_deeply(test, got, expected, message, extra)
     -- Check if expected contains more keys then got.
     for k, v in pairs(expected) do
       if has[k] ~= true and (extra.strict or v ~= NULL) then
-        extra.path = path .. "." .. k
+        extra.path = path .. "." .. tostring(k)
         extra.expected = "key (exists): " ..  tostring(k)
         extra.got = "key (missing): " .. tostring(k)
         return false
-- 
2.31.0

Re: [Tarantool-patches] [PATCH luajit 1/3] test: fix path storage for non-concatable objects

[sergos via Tarantool-patches]

Hi!
Thanks for the patch!

> On 9 Sep 2021, at 10:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> When the key of a table to compare via `tap.test:is_deeply()` is
> non-concatable object (i.e. lightuserdata) concatenation with path
  ^^^^ does it mean the object has no __concat, while has __tostring?

As a result: is there a case an object has no __tostring?

> raises an error.
> 
> This patch converts object to string to avoid this error.
> 
> Needed for tarantool/tarantool#5629
> ---
> test/tarantool-tests/tap.lua | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
> index 44fcac3d..a1f54d20 100644
> --- a/test/tarantool-tests/tap.lua
> +++ b/test/tarantool-tests/tap.lua
> @@ -166,7 +166,7 @@ local function is_deeply(test, got, expected, message, extra)
> 
>     for k, v in pairs(got) do
>       has[k] = true
> -      extra.path = path .. "." .. k
> +      extra.path = path .. "." .. tostring(k)
>       if not cmpdeeply(v, expected[k], extra) then
>         return false
>       end
> @@ -175,7 +175,7 @@ local function is_deeply(test, got, expected, message, extra)
>     -- Check if expected contains more keys then got.
>     for k, v in pairs(expected) do
>       if has[k] ~= true and (extra.strict or v ~= NULL) then
> -        extra.path = path .. "." .. k
> +        extra.path = path .. "." .. tostring(k)
>         extra.expected = "key (exists): " ..  tostring(k)
>         extra.got = "key (missing): " .. tostring(k)
>         return false
> -- 
> 2.31.0
> 

Re: [Tarantool-patches] [PATCH luajit 1/3] test: fix path storage for non-concatable objects

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 15.09.21, sergos wrote:
> Hi!
> Thanks for the patch!
> 
> > On 9 Sep 2021, at 10:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > When the key of a table to compare via `tap.test:is_deeply()` is
> > non-concatable object (i.e. lightuserdata) concatenation with path
>   ^^^^ does it mean the object has no __concat, while has __tostring?
> 
> As a result: is there a case an object has no __tostring?

Yes, for example, userdata may have no neither __tostring, nor __concat
metamethod, but tostring() will show us its value and prevent raising an
error.

| luajit -e 'print(getmetatable(newproxy(true)).__tostring, getmetatable(newproxy(true)).__concat); local r = "ud: "..tostring(newproxy()) print(r)'
| nil     nil
| ud: userdata: 0x41edce48

> 
> > raises an error.
> > 
> > This patch converts object to string to avoid this error.
> > 
> > Needed for tarantool/tarantool#5629
> > ---
> > test/tarantool-tests/tap.lua | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
> > index 44fcac3d..a1f54d20 100644
> > --- a/test/tarantool-tests/tap.lua
> > +++ b/test/tarantool-tests/tap.lua
> > @@ -166,7 +166,7 @@ local function is_deeply(test, got, expected, message, extra)
> > 
> >     for k, v in pairs(got) do
> >       has[k] = true
> > -      extra.path = path .. "." .. k
> > +      extra.path = path .. "." .. tostring(k)
> >       if not cmpdeeply(v, expected[k], extra) then
> >         return false
> >       end
> > @@ -175,7 +175,7 @@ local function is_deeply(test, got, expected, message, extra)
> >     -- Check if expected contains more keys then got.
> >     for k, v in pairs(expected) do
> >       if has[k] ~= true and (extra.strict or v ~= NULL) then
> > -        extra.path = path .. "." .. k
> > +        extra.path = path .. "." .. tostring(k)
> >         extra.expected = "key (exists): " ..  tostring(k)
> >         extra.got = "key (missing): " .. tostring(k)
> >         return false
> > -- 
> > 2.31.0
> > 
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/3] test: fix path storage for non-concatable objects

[sergos via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

Thanks for explanation! 

LGTM.

Sergos

> On 20 Sep 2021, at 11:28, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> On 15.09.21, sergos wrote:
>> Hi!
>> Thanks for the patch!
>> 
>>> On 9 Sep 2021, at 10:03, Sergey Kaplun <skaplun@tarantool.org <mailto:skaplun@tarantool.org>> wrote:
>>> 
>>> When the key of a table to compare via `tap.test:is_deeply()` is
>>> non-concatable object (i.e. lightuserdata) concatenation with path
>>  ^^^^ does it mean the object has no __concat, while has __tostring?
>> 
>> As a result: is there a case an object has no __tostring?
> 
> Yes, for example, userdata may have no neither __tostring, nor __concat
> metamethod, but tostring() will show us its value and prevent raising an
> error.
> 
> | luajit -e 'print(getmetatable(newproxy(true)).__tostring, getmetatable(newproxy(true)).__concat); local r = "ud: "..tostring(newproxy()) print(r)'
> | nil     nil
> | ud: userdata: 0x41edce48
> 
>> 
>>> raises an error.
>>> 
>>> This patch converts object to string to avoid this error.
>>> 
>>> Needed for tarantool/tarantool#5629
>>> ---
>>> test/tarantool-tests/tap.lua | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
>>> index 44fcac3d..a1f54d20 100644
>>> --- a/test/tarantool-tests/tap.lua
>>> +++ b/test/tarantool-tests/tap.lua
>>> @@ -166,7 +166,7 @@ local function is_deeply(test, got, expected, message, extra)
>>> 
>>>    for k, v in pairs(got) do
>>>      has[k] = true
>>> -      extra.path = path .. "." .. k
>>> +      extra.path = path .. "." .. tostring(k)
>>>      if not cmpdeeply(v, expected[k], extra) then
>>>        return false
>>>      end
>>> @@ -175,7 +175,7 @@ local function is_deeply(test, got, expected, message, extra)
>>>    -- Check if expected contains more keys then got.
>>>    for k, v in pairs(expected) do
>>>      if has[k] ~= true and (extra.strict or v ~= NULL) then
>>> -        extra.path = path .. "." .. k
>>> +        extra.path = path .. "." .. tostring(k)
>>>        extra.expected = "key (exists): " ..  tostring(k)
>>>        extra.got = "key (missing): " .. tostring(k)
>>>        return false
>>> -- 
>>> 2.31.0
>>> 
>> 
> 
> -- 
> Best regards,
> Sergey Kaplun


[-- Attachment #2: Type: text/html, Size: 15076 bytes --]

Re: [Tarantool-patches] [PATCH luajit 1/3] test: fix path storage for non-concatable objects

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, as trivial. I've only made some tweaks in
commit message:
| test: fix path storage for non-concatable objects
|
| When a key of the table to be compared via `tap.test:is_deeply()` is a
| non-concatable object (i.e. lightuserdata) its concatenation with the
| path prefix raises an error. As a result of this patch the object is
| explicitly converted to string to avoid this error.

-- 
Best regards,
IM

[Tarantool-patches] [PATCH luajit 2/3] Reorganize lightuserdata interning code.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

This patch only performs a code movement of lightuserdata interning to
<lj_udata.c> file and does nothing else.

Sergey Kaplun:
* added the description for the patch

Needed for tarantool/tarantool#5629
---
 src/lj_api.c   | 30 ++----------------------------
 src/lj_udata.c | 27 +++++++++++++++++++++++++++
 src/lj_udata.h |  3 +++
 3 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/src/lj_api.c b/src/lj_api.c
index c7a0b327..b6655e5a 100644
--- a/src/lj_api.c
+++ b/src/lj_api.c
@@ -716,36 +716,10 @@ LUA_API void lua_pushboolean(lua_State *L, int b)
   incr_top(L);
 }
 
-#if LJ_64
-static void *lightud_intern(lua_State *L, void *p)
-{
-  global_State *g = G(L);
-  uint64_t u = (uint64_t)p;
-  uint32_t up = lightudup(u);
-  uint32_t *segmap = mref(g->gc.lightudseg, uint32_t);
-  MSize segnum = g->gc.lightudnum;
-  if (segmap) {
-    MSize seg;
-    for (seg = 0; seg <= segnum; seg++)
-      if (segmap[seg] == up)  /* Fast path. */
-	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
-    segnum++;
-  }
-  if (!((segnum-1) & segnum) && segnum != 1) {
-    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
-    lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
-    setmref(g->gc.lightudseg, segmap);
-  }
-  g->gc.lightudnum = segnum;
-  segmap[segnum] = up;
-  return (void *)(((uint64_t)segnum << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
-}
-#endif
-
 LUA_API void lua_pushlightuserdata(lua_State *L, void *p)
 {
 #if LJ_64
-  p = lightud_intern(L, p);
+  p = lj_lightud_intern(L, p);
 #endif
   setrawlightudV(L->top, p);
   incr_top(L);
@@ -1197,7 +1171,7 @@ static TValue *cpcall(lua_State *L, lua_CFunction func, void *ud)
   setfuncV(L, top++, fn);
   if (LJ_FR2) setnilV(top++);
 #if LJ_64
-  ud = lightud_intern(L, ud);
+  ud = lj_lightud_intern(L, ud);
 #endif
   setrawlightudV(top++, ud);
   cframe_nres(L->cframe) = 1+0;  /* Zero results. */
diff --git a/src/lj_udata.c b/src/lj_udata.c
index 70c722a3..6808b1bc 100644
--- a/src/lj_udata.c
+++ b/src/lj_udata.c
@@ -8,6 +8,7 @@
 
 #include "lj_obj.h"
 #include "lj_gc.h"
+#include "lj_err.h"
 #include "lj_udata.h"
 
 GCudata *lj_udata_new(lua_State *L, MSize sz, GCtab *env)
@@ -34,3 +35,29 @@ void LJ_FASTCALL lj_udata_free(global_State *g, GCudata *ud)
   lj_mem_free(g, ud, sizeudata(ud));
 }
 
+#if LJ_64
+void *lj_lightud_intern(lua_State *L, void *p)
+{
+  global_State *g = G(L);
+  uint64_t u = (uint64_t)p;
+  uint32_t up = lightudup(u);
+  uint32_t *segmap = mref(g->gc.lightudseg, uint32_t);
+  MSize segnum = g->gc.lightudnum;
+  if (segmap) {
+    MSize seg;
+    for (seg = 0; seg <= segnum; seg++)
+      if (segmap[seg] == up)  /* Fast path. */
+	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
+    segnum++;
+  }
+  if (!((segnum-1) & segnum) && segnum != 1) {
+    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
+    lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
+    setmref(g->gc.lightudseg, segmap);
+  }
+  g->gc.lightudnum = segnum;
+  segmap[segnum] = up;
+  return (void *)(((uint64_t)segnum << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
+}
+#endif
+
diff --git a/src/lj_udata.h b/src/lj_udata.h
index f271a42d..d97936d4 100644
--- a/src/lj_udata.h
+++ b/src/lj_udata.h
@@ -10,5 +10,8 @@
 
 LJ_FUNC GCudata *lj_udata_new(lua_State *L, MSize sz, GCtab *env);
 LJ_FUNC void LJ_FASTCALL lj_udata_free(global_State *g, GCudata *ud);
+#if LJ_64
+LJ_FUNC void * LJ_FASTCALL lj_lightud_intern(lua_State *L, void *p);
+#endif
 
 #endif
-- 
2.31.0

Re: [Tarantool-patches] [PATCH luajit 2/3] Reorganize lightuserdata interning code.

[sergos via Tarantool-patches]

Hi! Thanks for the patch!

I would put something in description about the fact it’s pulled in just
to ease following the upstream - in particular, the following 3/3 is done
in this very moved code.

LGTM with above.

Sergos


> On 9 Sep 2021, at 10:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> This patch only performs a code movement of lightuserdata interning to
> <lj_udata.c> file and does nothing else.
> 
> Sergey Kaplun:
> * added the description for the patch
> 
> Needed for tarantool/tarantool#5629
> ---
> src/lj_api.c   | 30 ++----------------------------
> src/lj_udata.c | 27 +++++++++++++++++++++++++++
> src/lj_udata.h |  3 +++
> 3 files changed, 32 insertions(+), 28 deletions(-)
> 
> diff --git a/src/lj_api.c b/src/lj_api.c
> index c7a0b327..b6655e5a 100644
> --- a/src/lj_api.c
> +++ b/src/lj_api.c
> @@ -716,36 +716,10 @@ LUA_API void lua_pushboolean(lua_State *L, int b)
>   incr_top(L);
> }
> 
> -#if LJ_64
> -static void *lightud_intern(lua_State *L, void *p)
> -{
> -  global_State *g = G(L);
> -  uint64_t u = (uint64_t)p;
> -  uint32_t up = lightudup(u);
> -  uint32_t *segmap = mref(g->gc.lightudseg, uint32_t);
> -  MSize segnum = g->gc.lightudnum;
> -  if (segmap) {
> -    MSize seg;
> -    for (seg = 0; seg <= segnum; seg++)
> -      if (segmap[seg] == up)  /* Fast path. */
> -	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
> -    segnum++;
> -  }
> -  if (!((segnum-1) & segnum) && segnum != 1) {
> -    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
> -    lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
> -    setmref(g->gc.lightudseg, segmap);
> -  }
> -  g->gc.lightudnum = segnum;
> -  segmap[segnum] = up;
> -  return (void *)(((uint64_t)segnum << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
> -}
> -#endif
> -
> LUA_API void lua_pushlightuserdata(lua_State *L, void *p)
> {
> #if LJ_64
> -  p = lightud_intern(L, p);
> +  p = lj_lightud_intern(L, p);
> #endif
>   setrawlightudV(L->top, p);
>   incr_top(L);
> @@ -1197,7 +1171,7 @@ static TValue *cpcall(lua_State *L, lua_CFunction func, void *ud)
>   setfuncV(L, top++, fn);
>   if (LJ_FR2) setnilV(top++);
> #if LJ_64
> -  ud = lightud_intern(L, ud);
> +  ud = lj_lightud_intern(L, ud);
> #endif
>   setrawlightudV(top++, ud);
>   cframe_nres(L->cframe) = 1+0;  /* Zero results. */
> diff --git a/src/lj_udata.c b/src/lj_udata.c
> index 70c722a3..6808b1bc 100644
> --- a/src/lj_udata.c
> +++ b/src/lj_udata.c
> @@ -8,6 +8,7 @@
> 
> #include "lj_obj.h"
> #include "lj_gc.h"
> +#include "lj_err.h"
> #include "lj_udata.h"
> 
> GCudata *lj_udata_new(lua_State *L, MSize sz, GCtab *env)
> @@ -34,3 +35,29 @@ void LJ_FASTCALL lj_udata_free(global_State *g, GCudata *ud)
>   lj_mem_free(g, ud, sizeudata(ud));
> }
> 
> +#if LJ_64
> +void *lj_lightud_intern(lua_State *L, void *p)
> +{
> +  global_State *g = G(L);
> +  uint64_t u = (uint64_t)p;
> +  uint32_t up = lightudup(u);
> +  uint32_t *segmap = mref(g->gc.lightudseg, uint32_t);
> +  MSize segnum = g->gc.lightudnum;
> +  if (segmap) {
> +    MSize seg;
> +    for (seg = 0; seg <= segnum; seg++)
> +      if (segmap[seg] == up)  /* Fast path. */
> +	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
> +    segnum++;
> +  }
> +  if (!((segnum-1) & segnum) && segnum != 1) {
> +    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
> +    lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
> +    setmref(g->gc.lightudseg, segmap);
> +  }
> +  g->gc.lightudnum = segnum;
> +  segmap[segnum] = up;
> +  return (void *)(((uint64_t)segnum << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
> +}
> +#endif
> +
> diff --git a/src/lj_udata.h b/src/lj_udata.h
> index f271a42d..d97936d4 100644
> --- a/src/lj_udata.h
> +++ b/src/lj_udata.h
> @@ -10,5 +10,8 @@
> 
> LJ_FUNC GCudata *lj_udata_new(lua_State *L, MSize sz, GCtab *env);
> LJ_FUNC void LJ_FASTCALL lj_udata_free(global_State *g, GCudata *ud);
> +#if LJ_64
> +LJ_FUNC void * LJ_FASTCALL lj_lightud_intern(lua_State *L, void *p);
> +#endif
> 
> #endif
> -- 
> 2.31.0
> 

Re: [Tarantool-patches] [PATCH luajit 2/3] Reorganize lightuserdata interning code.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 15.09.21, sergos wrote:
> Hi! Thanks for the patch!
> 
> I would put something in description about the fact it’s pulled in just
> to ease following the upstream - in particular, the following 3/3 is done
> in this very moved code.
> 
> LGTM with above.

Updated the commit message to the following:
===================================================================
Reorganize lightuserdata interning code.

This patch only performs a code movement of lightuserdata interning to
<lj_udata.c> file and does nothing else. This patch is backported to
simplify syncing with the upstream.

Sergey Kaplun:
* added the description for the patch

Needed for tarantool/tarantool#5629
===================================================================

Branch is force pushed.

> 
> Sergos
> 
> 
> > On 9 Sep 2021, at 10:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > This patch only performs a code movement of lightuserdata interning to
> > <lj_udata.c> file and does nothing else.
> > 
> > Sergey Kaplun:
> > * added the description for the patch
> > 
> > Needed for tarantool/tarantool#5629
> > ---
> > src/lj_api.c   | 30 ++----------------------------
> > src/lj_udata.c | 27 +++++++++++++++++++++++++++
> > src/lj_udata.h |  3 +++
> > 3 files changed, 32 insertions(+), 28 deletions(-)
> > 
> > diff --git a/src/lj_api.c b/src/lj_api.c
> > index c7a0b327..b6655e5a 100644
> > --- a/src/lj_api.c
> > +++ b/src/lj_api.c
> > @@ -716,36 +716,10 @@ LUA_API void lua_pushboolean(lua_State *L, int b)
> >   incr_top(L);
> > }
> > 
> > -#if LJ_64
> > -static void *lightud_intern(lua_State *L, void *p)
> > -{
> > -  global_State *g = G(L);
> > -  uint64_t u = (uint64_t)p;
> > -  uint32_t up = lightudup(u);
> > -  uint32_t *segmap = mref(g->gc.lightudseg, uint32_t);
> > -  MSize segnum = g->gc.lightudnum;
> > -  if (segmap) {
> > -    MSize seg;
> > -    for (seg = 0; seg <= segnum; seg++)
> > -      if (segmap[seg] == up)  /* Fast path. */
> > -	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
> > -    segnum++;
> > -  }
> > -  if (!((segnum-1) & segnum) && segnum != 1) {
> > -    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
> > -    lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
> > -    setmref(g->gc.lightudseg, segmap);
> > -  }
> > -  g->gc.lightudnum = segnum;
> > -  segmap[segnum] = up;
> > -  return (void *)(((uint64_t)segnum << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
> > -}
> > -#endif
> > -
> > LUA_API void lua_pushlightuserdata(lua_State *L, void *p)
> > {
> > #if LJ_64
> > -  p = lightud_intern(L, p);
> > +  p = lj_lightud_intern(L, p);
> > #endif
> >   setrawlightudV(L->top, p);
> >   incr_top(L);
> > @@ -1197,7 +1171,7 @@ static TValue *cpcall(lua_State *L, lua_CFunction func, void *ud)
> >   setfuncV(L, top++, fn);
> >   if (LJ_FR2) setnilV(top++);
> > #if LJ_64
> > -  ud = lightud_intern(L, ud);
> > +  ud = lj_lightud_intern(L, ud);
> > #endif
> >   setrawlightudV(top++, ud);
> >   cframe_nres(L->cframe) = 1+0;  /* Zero results. */
> > diff --git a/src/lj_udata.c b/src/lj_udata.c
> > index 70c722a3..6808b1bc 100644
> > --- a/src/lj_udata.c
> > +++ b/src/lj_udata.c
> > @@ -8,6 +8,7 @@
> > 
> > #include "lj_obj.h"
> > #include "lj_gc.h"
> > +#include "lj_err.h"
> > #include "lj_udata.h"
> > 
> > GCudata *lj_udata_new(lua_State *L, MSize sz, GCtab *env)
> > @@ -34,3 +35,29 @@ void LJ_FASTCALL lj_udata_free(global_State *g, GCudata *ud)
> >   lj_mem_free(g, ud, sizeudata(ud));
> > }
> > 
> > +#if LJ_64
> > +void *lj_lightud_intern(lua_State *L, void *p)
> > +{
> > +  global_State *g = G(L);
> > +  uint64_t u = (uint64_t)p;
> > +  uint32_t up = lightudup(u);
> > +  uint32_t *segmap = mref(g->gc.lightudseg, uint32_t);
> > +  MSize segnum = g->gc.lightudnum;
> > +  if (segmap) {
> > +    MSize seg;
> > +    for (seg = 0; seg <= segnum; seg++)
> > +      if (segmap[seg] == up)  /* Fast path. */
> > +	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
> > +    segnum++;
> > +  }
> > +  if (!((segnum-1) & segnum) && segnum != 1) {
> > +    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
> > +    lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
> > +    setmref(g->gc.lightudseg, segmap);
> > +  }
> > +  g->gc.lightudnum = segnum;
> > +  segmap[segnum] = up;
> > +  return (void *)(((uint64_t)segnum << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
> > +}
> > +#endif
> > +
> > diff --git a/src/lj_udata.h b/src/lj_udata.h
> > index f271a42d..d97936d4 100644
> > --- a/src/lj_udata.h
> > +++ b/src/lj_udata.h
> > @@ -10,5 +10,8 @@
> > 
> > LJ_FUNC GCudata *lj_udata_new(lua_State *L, MSize sz, GCtab *env);
> > LJ_FUNC void LJ_FASTCALL lj_udata_free(global_State *g, GCudata *ud);
> > +#if LJ_64
> > +LJ_FUNC void * LJ_FASTCALL lj_lightud_intern(lua_State *L, void *p);
> > +#endif
> > 
> > #endif
> > -- 
> > 2.31.0
> > 
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/3] Reorganize lightuserdata interning code.

[sergos via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 5346 bytes --]

Thanks!

LGTM.

Sergos

> On 20 Sep 2021, at 11:32, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> On 15.09.21, sergos wrote:
>> Hi! Thanks for the patch!
>> 
>> I would put something in description about the fact it’s pulled in just
>> to ease following the upstream - in particular, the following 3/3 is done
>> in this very moved code.
>> 
>> LGTM with above.
> 
> Updated the commit message to the following:
> ===================================================================
> Reorganize lightuserdata interning code.
> 
> This patch only performs a code movement of lightuserdata interning to
> <lj_udata.c> file and does nothing else. This patch is backported to
> simplify syncing with the upstream.
> 
> Sergey Kaplun:
> * added the description for the patch
> 
> Needed for tarantool/tarantool#5629
> ===================================================================
> 
> Branch is force pushed.
> 
>> 
>> Sergos
>> 
>> 
>>> On 9 Sep 2021, at 10:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
>>> 
>>> From: Mike Pall <mike>
>>> 
>>> This patch only performs a code movement of lightuserdata interning to
>>> <lj_udata.c> file and does nothing else.
>>> 
>>> Sergey Kaplun:
>>> * added the description for the patch
>>> 
>>> Needed for tarantool/tarantool#5629
>>> ---
>>> src/lj_api.c   | 30 ++----------------------------
>>> src/lj_udata.c | 27 +++++++++++++++++++++++++++
>>> src/lj_udata.h |  3 +++
>>> 3 files changed, 32 insertions(+), 28 deletions(-)
>>> 
>>> diff --git a/src/lj_api.c b/src/lj_api.c
>>> index c7a0b327..b6655e5a 100644
>>> --- a/src/lj_api.c
>>> +++ b/src/lj_api.c
>>> @@ -716,36 +716,10 @@ LUA_API void lua_pushboolean(lua_State *L, int b)
>>>  incr_top(L);
>>> }
>>> 
>>> -#if LJ_64
>>> -static void *lightud_intern(lua_State *L, void *p)
>>> -{
>>> -  global_State *g = G(L);
>>> -  uint64_t u = (uint64_t)p;
>>> -  uint32_t up = lightudup(u);
>>> -  uint32_t *segmap = mref(g->gc.lightudseg, uint32_t);
>>> -  MSize segnum = g->gc.lightudnum;
>>> -  if (segmap) {
>>> -    MSize seg;
>>> -    for (seg = 0; seg <= segnum; seg++)
>>> -      if (segmap[seg] == up)  /* Fast path. */
>>> -	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
>>> -    segnum++;
>>> -  }
>>> -  if (!((segnum-1) & segnum) && segnum != 1) {
>>> -    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
>>> -    lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
>>> -    setmref(g->gc.lightudseg, segmap);
>>> -  }
>>> -  g->gc.lightudnum = segnum;
>>> -  segmap[segnum] = up;
>>> -  return (void *)(((uint64_t)segnum << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
>>> -}
>>> -#endif
>>> -
>>> LUA_API void lua_pushlightuserdata(lua_State *L, void *p)
>>> {
>>> #if LJ_64
>>> -  p = lightud_intern(L, p);
>>> +  p = lj_lightud_intern(L, p);
>>> #endif
>>>  setrawlightudV(L->top, p);
>>>  incr_top(L);
>>> @@ -1197,7 +1171,7 @@ static TValue *cpcall(lua_State *L, lua_CFunction func, void *ud)
>>>  setfuncV(L, top++, fn);
>>>  if (LJ_FR2) setnilV(top++);
>>> #if LJ_64
>>> -  ud = lightud_intern(L, ud);
>>> +  ud = lj_lightud_intern(L, ud);
>>> #endif
>>>  setrawlightudV(top++, ud);
>>>  cframe_nres(L->cframe) = 1+0;  /* Zero results. */
>>> diff --git a/src/lj_udata.c b/src/lj_udata.c
>>> index 70c722a3..6808b1bc 100644
>>> --- a/src/lj_udata.c
>>> +++ b/src/lj_udata.c
>>> @@ -8,6 +8,7 @@
>>> 
>>> #include "lj_obj.h"
>>> #include "lj_gc.h"
>>> +#include "lj_err.h"
>>> #include "lj_udata.h"
>>> 
>>> GCudata *lj_udata_new(lua_State *L, MSize sz, GCtab *env)
>>> @@ -34,3 +35,29 @@ void LJ_FASTCALL lj_udata_free(global_State *g, GCudata *ud)
>>>  lj_mem_free(g, ud, sizeudata(ud));
>>> }
>>> 
>>> +#if LJ_64
>>> +void *lj_lightud_intern(lua_State *L, void *p)
>>> +{
>>> +  global_State *g = G(L);
>>> +  uint64_t u = (uint64_t)p;
>>> +  uint32_t up = lightudup(u);
>>> +  uint32_t *segmap = mref(g->gc.lightudseg, uint32_t);
>>> +  MSize segnum = g->gc.lightudnum;
>>> +  if (segmap) {
>>> +    MSize seg;
>>> +    for (seg = 0; seg <= segnum; seg++)
>>> +      if (segmap[seg] == up)  /* Fast path. */
>>> +	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
>>> +    segnum++;
>>> +  }
>>> +  if (!((segnum-1) & segnum) && segnum != 1) {
>>> +    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
>>> +    lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
>>> +    setmref(g->gc.lightudseg, segmap);
>>> +  }
>>> +  g->gc.lightudnum = segnum;
>>> +  segmap[segnum] = up;
>>> +  return (void *)(((uint64_t)segnum << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
>>> +}
>>> +#endif
>>> +
>>> diff --git a/src/lj_udata.h b/src/lj_udata.h
>>> index f271a42d..d97936d4 100644
>>> --- a/src/lj_udata.h
>>> +++ b/src/lj_udata.h
>>> @@ -10,5 +10,8 @@
>>> 
>>> LJ_FUNC GCudata *lj_udata_new(lua_State *L, MSize sz, GCtab *env);
>>> LJ_FUNC void LJ_FASTCALL lj_udata_free(global_State *g, GCudata *ud);
>>> +#if LJ_64
>>> +LJ_FUNC void * LJ_FASTCALL lj_lightud_intern(lua_State *L, void *p);
>>> +#endif
>>> 
>>> #endif
>>> -- 
>>> 2.31.0
>>> 
>> 
> 
> -- 
> Best regards,
> Sergey Kaplun


[-- Attachment #2: Type: text/html, Size: 23554 bytes --]

Re: [Tarantool-patches] [PATCH luajit 2/3] Reorganize lightuserdata interning code.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, as trivial.

-- 
Best regards,
IM

[Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by XmiliaH.

(cherry picked from commit 16d38a4b214e8b8a20be554be77bd20286072365)

This patch fixes the regression introduced in scope of
fa8e7ffefb715abf55dc5b0c708c63251868 ('Add support for full-range
64 bit lightuserdata.').

The maximum available number of lightuserdata segment is 255. So the
high bits of this lightuserdata TValue are 0xfffe7fff. The same high
bits are set for special control variable on the stack for ITERN/ITERC
bytecodes via ISNEXT bytecode. When ITERN bytecode is despecialize to
ITERC bytecode and a table has the lightuserdata with the maximum
available segment number as a key, the special control variable is
considered as this key and iteration is broken.

This patch forbids to use more than 254 lightuserdata segments to
avoid clashing with the aforementioned control variable. In case when
user tries to create lightuserdata with 255th segment number an error
"bad light userdata pointer" is raised.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#5629
---
 src/lj_udata.c                                |  3 +-
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../lj-727-lightuserdata-itern.test.lua       | 48 ++++++++++++++
 .../lj-727-lightuserdata-itern/CMakeLists.txt |  1 +
 .../lightuserdata.c                           | 63 +++++++++++++++++++
 5 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
 create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c

diff --git a/src/lj_udata.c b/src/lj_udata.c
index 6808b1bc..1b7841fa 100644
--- a/src/lj_udata.c
+++ b/src/lj_udata.c
@@ -49,9 +49,10 @@ void *lj_lightud_intern(lua_State *L, void *p)
       if (segmap[seg] == up)  /* Fast path. */
 	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
     segnum++;
+    /* Leave last segment unused to avoid clash with ITERN key. */
+    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)-1) lj_err_msg(L, LJ_ERR_BADLU);
   }
   if (!((segnum-1) & segnum) && segnum != 1) {
-    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
     lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
     setmref(g->gc.lightudseg, segmap);
   }
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index a872fa5e..c933cc3f 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -60,6 +60,7 @@ add_subdirectory(gh-4427-ffi-sandwich)
 add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64)
 add_subdirectory(gh-6189-cur_L)
 add_subdirectory(lj-49-bad-lightuserdata)
+add_subdirectory(lj-727-lightuserdata-itern)
 add_subdirectory(lj-flush-on-trace)
 add_subdirectory(misclib-getmetrics-capi)
 
diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
new file mode 100644
index 00000000..ebe885bf
--- /dev/null
+++ b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
@@ -0,0 +1,48 @@
+local tap = require('tap')
+
+-- Test file to demonstrate next FF incorrect behaviour on LJ_64.
+-- See also, https://github.com/LuaJIT/LuaJIT/issues/727.
+
+local test = tap.test('lj-727-lightuserdata-itern')
+test:plan(1)
+
+local ud = require('lightuserdata').craft_ptr_wp()
+
+-- We now have the tagged lightuuserdata pointer
+-- 0xFFFE7FFF00000002 in the up before this patch (after the patch
+-- the maximum available lightuserdata segment is 0xffe).
+
+-- These pointers are special in for loop keys as they are used in
+-- the INTERN control variable to denote the current index in the
+-- array.
+-- If the ITERN is then patched to ITERC because of
+-- despecialization via the ISNEXT bytecode, the control variable
+-- is considered as the existing key in the table and some
+-- elements are skipped during iteration.
+
+local real_next = next
+local next = next
+
+local function itern_test(t, f)
+  for k, v in next, t do
+    f(k, v)
+  end
+end
+
+local visited = {}
+local t = {1, [ud] = 2345}
+itern_test(t, function(k, v)
+  visited[k] = v
+  if next == real_next then
+    next = function(tab, key)
+      return real_next(tab, key)
+    end
+    -- Despecialize bytecode.
+    itern_test({}, function() end)
+  end
+end)
+
+test.strict = true
+test:is_deeply(visited, t, 'userdata node is visited')
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt b/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
new file mode 100644
index 00000000..564b688b
--- /dev/null
+++ b/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
@@ -0,0 +1 @@
+BuildTestCLib(lightuserdata lightuserdata.c)
diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c b/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c
new file mode 100644
index 00000000..0fe6441c
--- /dev/null
+++ b/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c
@@ -0,0 +1,63 @@
+#include <lua.h>
+#include <lauxlib.h>
+
+#undef NDEBUG
+#include <assert.h>
+
+/* To stay within 47 bits, lightuserdata is segmented. */
+#define LJ_LIGHTUD_BITS_SEG 8
+#define NSEGMENTS ((1 << LJ_LIGHTUD_BITS_SEG)-1)
+
+/*
+ * The function to wrap: get a number to form lightuserdata to
+ * return with the 0xXXXXXfff00000002 format.
+ * It may raise an error, when the available lightuserdata
+ * segments are run out.
+ */
+static int craft_ptr(lua_State *L)
+{
+	const unsigned long long i = lua_tonumber(L, 1);
+	lua_pushlightuserdata(L, (void *)((i << 44) + 0xfff00000002));
+	return 1;
+}
+
+/*
+ * The function to generate bunch of lightuserdata of the
+ * 0xXXXXXfff00000002 format and push the last one on the stack.
+ */
+static int craft_ptr_wp(lua_State *L)
+{
+	void *ptr = NULL;
+	/*
+	 * There are only 255 available lightuserdata segments.
+	 * Generate a bunch of pointers to take them all.
+	 * XXX: After this patch the last userdata segment is
+	 * reserved for ISNEXT/ITERC/ITERN control variable, so
+	 * `craft_ptr()` function will raise an error at the last
+	 * iteration.
+	 */
+	unsigned long long i = 0;
+	for (; i < NSEGMENTS; i++) {
+		lua_pushcfunction(L, craft_ptr);
+		lua_pushnumber(L, i);
+		if (lua_pcall(L, 1, 1, 0) == LUA_OK)
+			ptr = (void *)lua_topointer(L, -1);
+		else
+			break;
+	}
+	assert(ptr != NULL);
+	/* Overwrite possible error message. */
+	lua_pushlightuserdata(L, ptr);
+	return 1;
+}
+
+static const struct luaL_Reg lightuserdata[] = {
+	{"craft_ptr_wp", craft_ptr_wp},
+	{NULL, NULL}
+};
+
+LUA_API int luaopen_lightuserdata(lua_State *L)
+{
+	luaL_register(L, "lightuserdata", lightuserdata);
+	return 1;
+}
-- 
2.31.0

Re: [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key.

[sergos via Tarantool-patches]

Hi! Thanks for the patch!

Just some test comment.

Regards,
Sergos

> On 9 Sep 2021, at 10:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> Reported by XmiliaH.
> 
> (cherry picked from commit 16d38a4b214e8b8a20be554be77bd20286072365)
> 
> This patch fixes the regression introduced in scope of
> fa8e7ffefb715abf55dc5b0c708c63251868 ('Add support for full-range
> 64 bit lightuserdata.').
> 
> The maximum available number of lightuserdata segment is 255. So the
> high bits of this lightuserdata TValue are 0xfffe7fff. The same high
> bits are set for special control variable on the stack for ITERN/ITERC
> bytecodes via ISNEXT bytecode. When ITERN bytecode is despecialize to
> ITERC bytecode and a table has the lightuserdata with the maximum
> available segment number as a key, the special control variable is
> considered as this key and iteration is broken.
> 
> This patch forbids to use more than 254 lightuserdata segments to
> avoid clashing with the aforementioned control variable. In case when
> user tries to create lightuserdata with 255th segment number an error
> "bad light userdata pointer" is raised.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#5629
> ---
> src/lj_udata.c                                |  3 +-
> test/tarantool-tests/CMakeLists.txt           |  1 +
> .../lj-727-lightuserdata-itern.test.lua       | 48 ++++++++++++++
> .../lj-727-lightuserdata-itern/CMakeLists.txt |  1 +
> .../lightuserdata.c                           | 63 +++++++++++++++++++
> 5 files changed, 115 insertions(+), 1 deletion(-)
> create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
> create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
> create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c
> 
> diff --git a/src/lj_udata.c b/src/lj_udata.c
> index 6808b1bc..1b7841fa 100644
> --- a/src/lj_udata.c
> +++ b/src/lj_udata.c
> @@ -49,9 +49,10 @@ void *lj_lightud_intern(lua_State *L, void *p)
>       if (segmap[seg] == up)  /* Fast path. */
> 	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
>     segnum++;
> +    /* Leave last segment unused to avoid clash with ITERN key. */
> +    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)-1) lj_err_msg(L, LJ_ERR_BADLU);
>   }
>   if (!((segnum-1) & segnum) && segnum != 1) {
> -    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
>     lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
>     setmref(g->gc.lightudseg, segmap);
>   }
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index a872fa5e..c933cc3f 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -60,6 +60,7 @@ add_subdirectory(gh-4427-ffi-sandwich)
> add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64)
> add_subdirectory(gh-6189-cur_L)
> add_subdirectory(lj-49-bad-lightuserdata)
> +add_subdirectory(lj-727-lightuserdata-itern)
> add_subdirectory(lj-flush-on-trace)
> add_subdirectory(misclib-getmetrics-capi)
> 
> diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
> new file mode 100644
> index 00000000..ebe885bf
> --- /dev/null
> +++ b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
> @@ -0,0 +1,48 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate next FF incorrect behaviour on LJ_64.
> +-- See also, https://github.com/LuaJIT/LuaJIT/issues/727.
> +
> +local test = tap.test('lj-727-lightuserdata-itern')
> +test:plan(1)
> +
> +local ud = require('lightuserdata').craft_ptr_wp()
> +
> +-- We now have the tagged lightuuserdata pointer
> +-- 0xFFFE7FFF00000002 in the up before this patch (after the patch
> +-- the maximum available lightuserdata segment is 0xffe).

Shall we end the test here with just an expectation of an error?
I believe you can make a way simpler test: pcall(craft_ptr()) should work
successfully 254 times and error on an 255th one, isn’t it?


> +
> +-- These pointers are special in for loop keys as they are used in
> +-- the INTERN control variable to denote the current index in the
> +-- array.
> +-- If the ITERN is then patched to ITERC because of
> +-- despecialization via the ISNEXT bytecode, the control variable
> +-- is considered as the existing key in the table and some
> +-- elements are skipped during iteration.
> +
> +local real_next = next
> +local next = next
> +
> +local function itern_test(t, f)
> +  for k, v in next, t do
> +    f(k, v)
> +  end
> +end
> +
> +local visited = {}
> +local t = {1, [ud] = 2345}
> +itern_test(t, function(k, v)
> +  visited[k] = v
> +  if next == real_next then
> +    next = function(tab, key)
> +      return real_next(tab, key)
> +    end
> +    -- Despecialize bytecode.
> +    itern_test({}, function() end)
> +  end
> +end)
> +
> +test.strict = true
> +test:is_deeply(visited, t, 'userdata node is visited')
> +
> +os.exit(test:check() and 0 or 1)
> diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt b/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
> new file mode 100644
> index 00000000..564b688b
> --- /dev/null
> +++ b/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
> @@ -0,0 +1 @@
> +BuildTestCLib(lightuserdata lightuserdata.c)
> diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c b/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c
> new file mode 100644
> index 00000000..0fe6441c
> --- /dev/null
> +++ b/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c
> @@ -0,0 +1,63 @@
> +#include <lua.h>
> +#include <lauxlib.h>
> +
> +#undef NDEBUG
> +#include <assert.h>
> +
> +/* To stay within 47 bits, lightuserdata is segmented. */
> +#define LJ_LIGHTUD_BITS_SEG 8
> +#define NSEGMENTS ((1 << LJ_LIGHTUD_BITS_SEG)-1)
> +
> +/*
> + * The function to wrap: get a number to form lightuserdata to
> + * return with the 0xXXXXXfff00000002 format.
> + * It may raise an error, when the available lightuserdata
> + * segments are run out.
> + */
> +static int craft_ptr(lua_State *L)
> +{
> +	const unsigned long long i = lua_tonumber(L, 1);
> +	lua_pushlightuserdata(L, (void *)((i << 44) + 0xfff00000002));
> +	return 1;
> +}
> +
> +/*
> + * The function to generate bunch of lightuserdata of the
> + * 0xXXXXXfff00000002 format and push the last one on the stack.
> + */
> +static int craft_ptr_wp(lua_State *L)
> +{
> +	void *ptr = NULL;
> +	/*
> +	 * There are only 255 available lightuserdata segments.
> +	 * Generate a bunch of pointers to take them all.
> +	 * XXX: After this patch the last userdata segment is
> +	 * reserved for ISNEXT/ITERC/ITERN control variable, so
> +	 * `craft_ptr()` function will raise an error at the last
> +	 * iteration.
> +	 */
> +	unsigned long long i = 0;
> +	for (; i < NSEGMENTS; i++) {
> +		lua_pushcfunction(L, craft_ptr);
> +		lua_pushnumber(L, i);
> +		if (lua_pcall(L, 1, 1, 0) == LUA_OK)
> +			ptr = (void *)lua_topointer(L, -1);
> +		else
> +			break;
> +	}
> +	assert(ptr != NULL);
> +	/* Overwrite possible error message. */
> +	lua_pushlightuserdata(L, ptr);
> +	return 1;
> +}
> +
> +static const struct luaL_Reg lightuserdata[] = {
> +	{"craft_ptr_wp", craft_ptr_wp},
> +	{NULL, NULL}
> +};
> +
> +LUA_API int luaopen_lightuserdata(lua_State *L)
> +{
> +	luaL_register(L, "lightuserdata", lightuserdata);
> +	return 1;
> +}
> -- 
> 2.31.0
> 

Re: [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 15.09.21, sergos wrote:
> Hi! Thanks for the patch!
> 
> Just some test comment.
> 
> Regards,
> Sergos
> 
> > On 9 Sep 2021, at 10:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > Reported by XmiliaH.
> > 
> > (cherry picked from commit 16d38a4b214e8b8a20be554be77bd20286072365)
> > 
> > This patch fixes the regression introduced in scope of
> > fa8e7ffefb715abf55dc5b0c708c63251868 ('Add support for full-range
> > 64 bit lightuserdata.').
> > 
> > The maximum available number of lightuserdata segment is 255. So the
> > high bits of this lightuserdata TValue are 0xfffe7fff. The same high
> > bits are set for special control variable on the stack for ITERN/ITERC
> > bytecodes via ISNEXT bytecode. When ITERN bytecode is despecialize to
> > ITERC bytecode and a table has the lightuserdata with the maximum
> > available segment number as a key, the special control variable is
> > considered as this key and iteration is broken.
> > 
> > This patch forbids to use more than 254 lightuserdata segments to
> > avoid clashing with the aforementioned control variable. In case when
> > user tries to create lightuserdata with 255th segment number an error
> > "bad light userdata pointer" is raised.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#5629
> > ---
> > src/lj_udata.c                                |  3 +-
> > test/tarantool-tests/CMakeLists.txt           |  1 +
> > .../lj-727-lightuserdata-itern.test.lua       | 48 ++++++++++++++
> > .../lj-727-lightuserdata-itern/CMakeLists.txt |  1 +
> > .../lightuserdata.c                           | 63 +++++++++++++++++++
> > 5 files changed, 115 insertions(+), 1 deletion(-)
> > create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
> > create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
> > create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c
> > 
> > diff --git a/src/lj_udata.c b/src/lj_udata.c
> > index 6808b1bc..1b7841fa 100644
> > --- a/src/lj_udata.c
> > +++ b/src/lj_udata.c
> > @@ -49,9 +49,10 @@ void *lj_lightud_intern(lua_State *L, void *p)
> >       if (segmap[seg] == up)  /* Fast path. */
> > 	return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u));
> >     segnum++;
> > +    /* Leave last segment unused to avoid clash with ITERN key. */
> > +    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)-1) lj_err_msg(L, LJ_ERR_BADLU);
> >   }
> >   if (!((segnum-1) & segnum) && segnum != 1) {
> > -    if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU);
> >     lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t);
> >     setmref(g->gc.lightudseg, segmap);
> >   }
> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> > index a872fa5e..c933cc3f 100644
> > --- a/test/tarantool-tests/CMakeLists.txt
> > +++ b/test/tarantool-tests/CMakeLists.txt
> > @@ -60,6 +60,7 @@ add_subdirectory(gh-4427-ffi-sandwich)
> > add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64)
> > add_subdirectory(gh-6189-cur_L)
> > add_subdirectory(lj-49-bad-lightuserdata)
> > +add_subdirectory(lj-727-lightuserdata-itern)
> > add_subdirectory(lj-flush-on-trace)
> > add_subdirectory(misclib-getmetrics-capi)
> > 
> > diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
> > new file mode 100644
> > index 00000000..ebe885bf
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
> > @@ -0,0 +1,48 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate next FF incorrect behaviour on LJ_64.
> > +-- See also, https://github.com/LuaJIT/LuaJIT/issues/727.
> > +
> > +local test = tap.test('lj-727-lightuserdata-itern')
> > +test:plan(1)
> > +
> > +local ud = require('lightuserdata').craft_ptr_wp()
> > +
> > +-- We now have the tagged lightuuserdata pointer
> > +-- 0xFFFE7FFF00000002 in the up before this patch (after the patch
> > +-- the maximum available lightuserdata segment is 0xffe).
> 
> Shall we end the test here with just an expectation of an error?
> I believe you can make a way simpler test: pcall(craft_ptr()) should work
> successfully 254 times and error on an 255th one, isn’t it?

Not exactly, I think.
The main idea of the test -- generate as much lightuserdata objects as
we can, and save them in the same table. After that we check that
iteration by them is correct.

Test you suggested doesn't show up the possible issue with ITERN, does
it?

> 
> 
> > +
> > +-- These pointers are special in for loop keys as they are used in
> > +-- the INTERN control variable to denote the current index in the
> > +-- array.
> > +-- If the ITERN is then patched to ITERC because of
> > +-- despecialization via the ISNEXT bytecode, the control variable
> > +-- is considered as the existing key in the table and some
> > +-- elements are skipped during iteration.
> > +
> > +local real_next = next
> > +local next = next
> > +
> > +local function itern_test(t, f)
> > +  for k, v in next, t do
> > +    f(k, v)
> > +  end
> > +end
> > +
> > +local visited = {}
> > +local t = {1, [ud] = 2345}
> > +itern_test(t, function(k, v)
> > +  visited[k] = v
> > +  if next == real_next then
> > +    next = function(tab, key)
> > +      return real_next(tab, key)
> > +    end
> > +    -- Despecialize bytecode.
> > +    itern_test({}, function() end)
> > +  end
> > +end)
> > +
> > +test.strict = true
> > +test:is_deeply(visited, t, 'userdata node is visited')
> > +
> > +os.exit(test:check() and 0 or 1)
> > diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt b/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
> > new file mode 100644
> > index 00000000..564b688b
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
> > @@ -0,0 +1 @@
> > +BuildTestCLib(lightuserdata lightuserdata.c)
> > diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c b/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c
> > new file mode 100644
> > index 00000000..0fe6441c
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c
> > @@ -0,0 +1,63 @@
> > +#include <lua.h>
> > +#include <lauxlib.h>
> > +
> > +#undef NDEBUG
> > +#include <assert.h>
> > +
> > +/* To stay within 47 bits, lightuserdata is segmented. */
> > +#define LJ_LIGHTUD_BITS_SEG 8
> > +#define NSEGMENTS ((1 << LJ_LIGHTUD_BITS_SEG)-1)
> > +
> > +/*
> > + * The function to wrap: get a number to form lightuserdata to
> > + * return with the 0xXXXXXfff00000002 format.
> > + * It may raise an error, when the available lightuserdata
> > + * segments are run out.
> > + */
> > +static int craft_ptr(lua_State *L)
> > +{
> > +	const unsigned long long i = lua_tonumber(L, 1);
> > +	lua_pushlightuserdata(L, (void *)((i << 44) + 0xfff00000002));
> > +	return 1;
> > +}
> > +
> > +/*
> > + * The function to generate bunch of lightuserdata of the
> > + * 0xXXXXXfff00000002 format and push the last one on the stack.
> > + */
> > +static int craft_ptr_wp(lua_State *L)
> > +{
> > +	void *ptr = NULL;
> > +	/*
> > +	 * There are only 255 available lightuserdata segments.
> > +	 * Generate a bunch of pointers to take them all.
> > +	 * XXX: After this patch the last userdata segment is
> > +	 * reserved for ISNEXT/ITERC/ITERN control variable, so
> > +	 * `craft_ptr()` function will raise an error at the last
> > +	 * iteration.
> > +	 */
> > +	unsigned long long i = 0;
> > +	for (; i < NSEGMENTS; i++) {
> > +		lua_pushcfunction(L, craft_ptr);
> > +		lua_pushnumber(L, i);
> > +		if (lua_pcall(L, 1, 1, 0) == LUA_OK)
> > +			ptr = (void *)lua_topointer(L, -1);
> > +		else
> > +			break;
> > +	}
> > +	assert(ptr != NULL);
> > +	/* Overwrite possible error message. */
> > +	lua_pushlightuserdata(L, ptr);
> > +	return 1;
> > +}
> > +
> > +static const struct luaL_Reg lightuserdata[] = {
> > +	{"craft_ptr_wp", craft_ptr_wp},
> > +	{NULL, NULL}
> > +};
> > +
> > +LUA_API int luaopen_lightuserdata(lua_State *L)
> > +{
> > +	luaL_register(L, "lightuserdata", lightuserdata);
> > +	return 1;
> > +}
> > -- 
> > 2.31.0
> > 
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key.

[sergos via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2048 bytes --]

Hi!

> On 20 Sep 2021, at 11:38, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> On 15.09.21, sergos wrote:

[...]

>>> +++ b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
>>> @@ -0,0 +1,48 @@
>>> +local tap = require('tap')
>>> +
>>> +-- Test file to demonstrate next FF incorrect behaviour on LJ_64.
>>> +-- See also, https://github.com/LuaJIT/LuaJIT/issues/727.
>>> +
>>> +local test = tap.test('lj-727-lightuserdata-itern')
>>> +test:plan(1)
>>> +
>>> +local ud = require('lightuserdata').craft_ptr_wp()
>>> +
>>> +-- We now have the tagged lightuuserdata pointer
>>> +-- 0xFFFE7FFF00000002 in the up before this patch (after the patch
>>> +-- the maximum available lightuserdata segment is 0xffe).
>> 
>> Shall we end the test here with just an expectation of an error?
>> I believe you can make a way simpler test: pcall(craft_ptr()) should work
>> successfully 254 times and error on an 255th one, isn’t it?
> 
> Not exactly, I think.
> The main idea of the test -- generate as much lightuserdata objects as
> we can, and save them in the same table. After that we check that
> iteration by them is correct.
> 
> Test you suggested doesn't show up the possible issue with ITERN, does
> it?

Exactly. I don’t see any reason to force the situation showing that you
can’t use the LUD segment beyond particular value. The test can be that
simple showing the max segment is 254, not 255 - exactly the functionality
that is added to the code. So, it should fail at creation of 255th segment
and it will be the positive outcome of the test. If there’s no error -
the test fails.
It will simplify the test considerably. Also, you should not have such
long explanation of ITERN/ITERC - just say "the 255th segment is forbidden,
since its encoding is overlapped with control variable used by ISNEXT”.

I would recommend to wait for the 2nd reviewer here - especially if you
discussed the patch before submit.

Regards,
Sergos


[-- Attachment #2: Type: text/html, Size: 11108 bytes --]

Re: [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! As a result of the fixes discussed offline, LGTM.

On 20.09.21, sergos wrote:
> Hi!
> 
> > On 20 Sep 2021, at 11:38, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > Hi, Sergos!
> > 
> > Thanks for the review!
> > 
> > On 15.09.21, sergos wrote:
> 
> [...]
> 
> >>> +++ b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
> >>> @@ -0,0 +1,48 @@
> >>> +local tap = require('tap')
> >>> +
> >>> +-- Test file to demonstrate next FF incorrect behaviour on LJ_64.
> >>> +-- See also, https://github.com/LuaJIT/LuaJIT/issues/727.
> >>> +
> >>> +local test = tap.test('lj-727-lightuserdata-itern')
> >>> +test:plan(1)
> >>> +
> >>> +local ud = require('lightuserdata').craft_ptr_wp()
> >>> +
> >>> +-- We now have the tagged lightuuserdata pointer
> >>> +-- 0xFFFE7FFF00000002 in the up before this patch (after the patch
> >>> +-- the maximum available lightuserdata segment is 0xffe).
> >> 
> >> Shall we end the test here with just an expectation of an error?
> >> I believe you can make a way simpler test: pcall(craft_ptr()) should work
> >> successfully 254 times and error on an 255th one, isn’t it?
> > 
> > Not exactly, I think.
> > The main idea of the test -- generate as much lightuserdata objects as
> > we can, and save them in the same table. After that we check that
> > iteration by them is correct.
> > 
> > Test you suggested doesn't show up the possible issue with ITERN, does
> > it?
> 
> Exactly. I don’t see any reason to force the situation showing that you
> can’t use the LUD segment beyond particular value. The test can be that
> simple showing the max segment is 254, not 255 - exactly the functionality
> that is added to the code. So, it should fail at creation of 255th segment
> and it will be the positive outcome of the test. If there’s no error -
> the test fails.
> It will simplify the test considerably. Also, you should not have such
> long explanation of ITERN/ITERC - just say "the 255th segment is forbidden,
> since its encoding is overlapped with control variable used by ISNEXT”.

Sergos, I'm partially agree with you: we can just check that the last
lightuserdata segment is reserved for LuaJIT internal usage -- this is
the case. However, Sergey wants to check that after this patch ITERN
despecialization doesn't lead to table misiteration (since this is the
symptom being reported in LuaJIT queue).

Unfortunately I can't figure out two issues:
1. How to properly check the BADLU is raised for the *last* segment
(consider the comment in the commit from the trunk[1]).
2. What is more important: how does this error stops us from using
"ITERN-magic-range" pointers outside of the last segment? I might be
missing some related macro-magic, but AFAIU a new segment is created for
any lightudup value, hence 0xfffe7fff... pointers can be mapped into the
second segment, can't they?

Anyway, I guess we can proceed with this series and report an issue/PR
to the vanilla trunk when the issue bothers us again.

P.S. What if we create "craft pointers" in a reverse order? I'll check
this a bit later.

> 
> I would recommend to wait for the 2nd reviewer here - especially if you
> discussed the patch before submit.
> 
> Regards,
> Sergos
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/3] Follow-up fixes for full 64-bit lightuserdata interning

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patches into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10 (only the first one).

On 09.09.21, Sergey Kaplun wrote:
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-727-lightuserdata-itern
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/lj-727-lightuserdata-itern
> Issue: https://github.com/LuaJIT/LuaJIT/issues/727
> 
> The first commit (test-related) is needed to fix test behaviour for
> is_deeply function for userdata object.
> The second commit (refactoring) is needed to simplify cherry-pick of the
> next one (and the last one) without conflicts.
> 
> Mike Pall (2):
>   Reorganize lightuserdata interning code.
>   Avoid conflict between 64 bit lightuserdata and ITERN key.
> 
> Sergey Kaplun (1):
>   test: fix path storage for non-concatable objects
> 
>  src/lj_api.c                                  | 30 +--------
>  src/lj_udata.c                                | 28 +++++++++
>  src/lj_udata.h                                |  3 +
>  test/tarantool-tests/CMakeLists.txt           |  1 +
>  .../lj-727-lightuserdata-itern.test.lua       | 48 ++++++++++++++
>  .../lj-727-lightuserdata-itern/CMakeLists.txt |  1 +
>  .../lightuserdata.c                           | 63 +++++++++++++++++++
>  test/tarantool-tests/tap.lua                  |  4 +-
>  8 files changed, 148 insertions(+), 30 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern.test.lua
>  create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt
>  create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c
> 
> -- 
> 2.31.0
> 

-- 
Best regards,
IM