dibyendumajumdar / ravi

Ravi is a dialect of Lua, featuring limited optional static typing, JIT and AOT compilers
http://ravilang.github.io/
Other
1.16k stars 62 forks source link

Wrong info in docs on OP_TEST and OP_TESTSET #184

Closed Kristopher38 closed 4 years ago

Kristopher38 commented 4 years ago

This issue is regarding Lua bytecode reference docs. In the section on OP_TEST and OP_TESTSET there is a line that states:

For TESTSET, register R(B) is coerced into a boolean and compared to the boolean field C. If R(B) matches C, the next instruction is skipped, otherwise R(B) is assigned to R(A) and the VM continues with the next instruction.

And a pseudocode:

TEST        A C     if not (R(A) <=> C) then pc++
TESTSET     A B C   if (R(B) <=> C) then R(A) := R(B) else pc++

But closely reading the Lua source for version 5.3.5, where the handling of OP_TEST and OP_TESTSET happens, we can conclude that the info in the docs is the wrong way around. Let me explain that with OP_TESTSET (the same applies to OP_TEST as well, just without R(A) = R(B) and the argument slots are different as stated in the docs). In the lvm.c the code for handling OP_TESTSET is as follows:

if (GETARG_C(i) ? l_isfalse(rb) : !l_isfalse(rb))
  ci->u.l.savedpc++;
else {
  setobjs2s(L, ra, rb);
  donextjump(ci);
}

Now let me rewrite that in pseudocode (value in R(B) is coerced into a Lua boolean):

if C == false then
  if R(B) == true then
    skip next instruction
  else -- R(B) == false
    R(A) = R(B)
  end
else -- C == true
  if R(B) == true then
    R(A) = R(B)
  else -- R(B) == false
    skip next instruction
  end
end

Which essentially means:

if C is different than R(B) then
  skip next instruction
else -- C is the same as R(B)
  R(A) = R(B)
end

But the docs clearly state the opposite:

If R(B) matches C, the next instruction is skipped, otherwise R(B) is assigned to R(A)

Paragraphs on OP_TEST and OP_TESTSET should be corrected to not be misleading.

dibyendumajumdar commented 4 years ago

Thank you @Kristopher38 appreciate the feedback. I will check and correct.

dibyendumajumdar commented 4 years ago

Hi @Kristopher38, strange as the comment in lopcodes.h must be wrong then? You may be right about this, I need to go through some examples to verify.

dibyendumajumdar commented 4 years ago

Hi @Kristopher38 Please would you review https://github.com/dibyendumajumdar/ravi/blob/master/readthedocs/lua_bytecode_reference.rst? I am still working through the various examples of bytecode

Kristopher38 commented 4 years ago

Hi, I've read through the commits and they look correct to me. The updated bytecode explanations will especially help at getting a better grasp of what exactly is the execution flow of those opcodes.

Also thanks for creating this reference manual and keeping it up to date :)

dibyendumajumdar commented 4 years ago

@Kristopher38 Thank you for reporting the issue and also for reviewing the changes. Really appreciated.