[Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Alexander Turenko]

It seems we look at this code from some very different positions. I have
my patterns in the mind and you have your ones.

WBR, Alexander Turenko.

On Sun, Oct 11, 2020 at 08:47:55PM +0300, Igor Munkin wrote:
> Sasha,
> 
> It is not full review, but Vlad asked me offline regarding a passage
> below. So I dump our discussion results.
> 
> On 11.10.20, Alexander Turenko wrote:
> 
> <snipped>
> 
> > +	int top = lua_gettop(L);
> 
> This value can't be used, since <lua_rawgeti> can trigger Lua stack
> reallocation, ergo this value will be invalidated.

AFAIU, you rejected this comment in the next email?

 | This is not the address, but offset, so nevermind.

> 
> > +
> > +	/* Calculate absolute value in the stack. */
> 
> At first, it was tough to me to understand the reason you transform the
> given relative index to an absolute one, since there is everything
> within <lua_pushvalue> for it. I finally got the issue after Vlad's
> comments and another (more thorough) look to the sources. I believe it's
> nice to drop a few words regarding it. Here are the key points (IMHO):
> * whether index is less than zero, it is considered relative to the top
>   Lua stack slot
> * when you obtain the function object to be called, top pointer is
>   incremented, so index ought to be adjusted respectively
> 
> > +	if (idx < 0)
> > +		idx = top + idx + 1;
> 
> Well, is this math even correct? AFAICS, you copy the <idx> slot on the
> top as a first argument for <luaT_tuple_encode_table_ref>, right? So,
> this is the original guest stack layout:
> | nil | <- L->top
> | ... |
> | val | <- idx
> And this is the resulting one:
> | nil | <- L->top
> | val |
> | fun | <- old L->top
> | ... |
> | val | <- idx
> 
> So, it looks like you need to subtract 1 instead of adding it, since
> <idx> is negative. Feel free to correct me if I'm bad in this math.
> 
> Anyway, technically, you don't need to calculate the absolute value by
> yourself, just adjust the offset to the given slot. I guess the
> following line is enough (with the verbose comment I mentioned above):
> | idx -= idx < 0;

Hm. Hmmm.

The math is correct. We have the linear dependency, so it seems we can
just verify one negative idx and others should be good too for any valid
composition of idx and top.

top: 4; idx: -1 -> 4 -- ok
top: 4; idx: -2 -> 3 -- ok

This snippet is used several times across tarantool code base: say, in
luaL_checkcdata().

Let's show the sketchy code:

 | int top = gettop(L);
 | <some stack manipulations>
 | int rc = lua_pcall(<...>);
 | lua_settop(L, top);
 | <handle rc>

It is much, MUCH better than doing all those lua_pop(L, 1) or
lua_pop(L, 2) depending on lua_pcall() return value.

Now, look at another schetchy code:

 | int idx = absolute(idx);
 | <doing some stack manipulations>
 | lua_pushvalue(L, idx);

It again much, MUCH better than doing all those idx + 1 or -1 or even -2
depending on how top is changed.

> 
> > +
> > +	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
> > +	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
> > +	assert(lua_isfunction(L, -1));
> > +
> > +	lua_pushvalue(L, idx);
> 
> There is also another way: simply leave the comment prior to
> <lua_pushvalue> call and pass the proper index as an argument
> | lua_pushvalue(L, idx - (idx < 0));

It'll be <idx - 2 * (idx < 0)> in the next commit. I don't want to play
this game and still think that it is much better to just use an
absolute index.