Tarantool development patches archive
 help / color / mirror / Atom feed
From: sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key.
Date: Mon, 20 Sep 2021 12:37:56 +0300
Message-ID: <E8B5FDFC-0FEE-4D14-AE5B-2699D5BA342B@tarantool.org> (raw)
In-Reply-To: <YUhIk2zNYcf7+dAg@root>

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

      reply	other threads:[~2021-09-20  9:38 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-09  7:03 [Tarantool-patches] [PATCH luajit 0/3] Follow-up fixes for full 64-bit lightuserdata interning Sergey Kaplun via Tarantool-patches
2021-09-09  7:03 ` [Tarantool-patches] [PATCH luajit 1/3] test: fix path storage for non-concatable objects Sergey Kaplun via Tarantool-patches
2021-09-15 15:30   ` sergos via Tarantool-patches
2021-09-20  8:28     ` Sergey Kaplun via Tarantool-patches
2021-09-20  9:37       ` sergos via Tarantool-patches
2021-09-09  7:03 ` [Tarantool-patches] [PATCH luajit 2/3] Reorganize lightuserdata interning code Sergey Kaplun via Tarantool-patches
2021-09-15 15:30   ` sergos via Tarantool-patches
2021-09-20  8:32     ` Sergey Kaplun via Tarantool-patches
2021-09-20  9:37       ` sergos via Tarantool-patches
2021-09-09  7:03 ` [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key Sergey Kaplun via Tarantool-patches
2021-09-15 15:31   ` sergos via Tarantool-patches
2021-09-20  8:38     ` Sergey Kaplun via Tarantool-patches
2021-09-20  9:37       ` sergos via Tarantool-patches [this message]

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=E8B5FDFC-0FEE-4D14-AE5B-2699D5BA342B@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergos@tarantool.org \
    --cc=skaplun@tarantool.org \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git