Repository: yuin/gopher-lua (go)

Issue:
Incorrect code generation for `LOADNIL`
- [X] GopherLua is a Lua5.1 implementation. You should be familiar with Lua programming language. Have you read [Lua 5.1 reference manual](http://www.lua.org/manual/5.1/) carefully?
- [X] GopherLua is a Lua5.1 implementation.  In Lua, to keep it simple, **it is more important to remove functionalities rather than to add functionalities unlike other languages** . If you are going to introduce some new cool functionalities into the GopherLua code base and the functionalities can be implemented by existing APIs, It should be implemented as a library. 

Please answer the following before submitting your issue:

1. What version of GopherLua are you using? : `2348fd042596f47bf9a1018a01f365ca3bf19134`
2. What version of Go are you using? : 1.22.2
3. What operating system and processor architecture are you using? : Mac M1 ARM 64
4. What did you do? : See below
5. What did you expect to see? : See below
6. What did you see instead? : See below

There is a bug in Gopher Lua `master` latest `2348fd042596f47bf9a1018a01f365ca3bf19134`

Under certain conditions, additional registers can be erroneously cleared by a single `nil` assignment.

Run the following through glua on the cmd line to reproduce:

    function test()
        local a = 0
        local b = 1
        local c = 2
        local d = 3
        local e = 4		-- reg 4
        local f = 5
        local g = 6
        local h = 7

        if e == 4 then
            e = nil		-- should clear reg 4, but clears regs 4-8 by mistake
        end
        if f == nil then
            print("bad f")
        end
        if g == nil then
            print("bad g")
        end
        if h == nil then
            print("bad h")
        end
    end

    test()


it will print:

	bad f
	bad g
	bad h

Indicating several variables have been incorrectly cleared.

The bug occurs due to an incorrect `LOADNIL` being generated for the assignment in the if block. According to the [lua 5.1 byte code spec](https://www.lua.org/source/5.1/lopcodes.h.html), it should generate

    LOADNIL 4, 4, 0

Because in Lua 5.1, A should be the first register to be cleared, and B should be the final register to be cleared.

But Gopher Lua generates:

	LOADNIL 4, 8, 0

This is due to code in compile.go which merges `LOADNIL` instructions, see:

    func (cd *codeStore) AddLoadNil(a, b, line int) {
      last := cd.Last()
      if opGetOpCode(last) == OP_LOADNIL && (opGetArgA(last)+opGetArgB(last)) == a {
        cd.SetB(cd.LastPC(), b)
      } else {
        cd.AddABC(OP_LOADNIL, a, b, 0, line)
      }
    }

This code is written with the expectation that the final register to be overwritten is A+B, which is not correct for Lua 5.1 (but _is_ correct for Lua 5.2+, where the `LOADNIL` [byte code spec changed](https://www.lua.org/source/5.2/lopcodes.h.html)).

I will prepare a PR to propose a fix to make `AddLoadNil()` merge correctly.

Pull request:
fix compiler producing incorrect LOADNIL byte code
Fixes #495 

Proposed fix and tests to fix `AddLoadNil()` optimisation, which was incorrectly merging `LOADNIL` instructions under certain conditions.

Patch:
diff --git a/compile.go b/compile.go
index c373677..e46e0d7 100644
--- a/compile.go
+++ b/compile.go
@@ -237,7 +237,7 @@ func (cd *codeStore) PropagateMV(top int, save *int, reg *int, inc int) {
 
 func (cd *codeStore) AddLoadNil(a, b, line int) {
 	last := cd.Last()
-	if opGetOpCode(last) == OP_LOADNIL && (opGetArgA(last)+opGetArgB(last)) == a {
+	if opGetOpCode(last) == OP_LOADNIL && (opGetArgB(last)+1) == a {
 		cd.SetB(cd.LastPC(), b)
 	} else {
 		cd.AddABC(OP_LOADNIL, a, b, 0, line)


Test patch:
diff --git a/script_test.go b/script_test.go
index 3c0675d..48f48b3 100644
--- a/script_test.go
+++ b/script_test.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"runtime"
+	"strings"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -146,3 +147,86 @@ func TestGlua(t *testing.T) {
 func TestLua(t *testing.T) {
 	testScriptDir(t, luaTests, "_lua5.1-tests")
 }
+
+func TestMergingLoadNilBug(t *testing.T) {
+	// there was a bug where a multiple load nils were being incorrectly merged, and the following code exposed it
+	s := `
+    function test()
+        local a = 0
+        local b = 1
+        local c = 2
+        local d = 3
+        local e = 4		-- reg 4
+        local f = 5
+        local g = 6
+        local h = 7
+
+        if e == 4 then
+            e = nil		-- should clear reg 4, but clears regs 4-8 by mistake
+        end
+        if f == nil then
+            error("bad f")
+        end
+        if g == nil then
+            error("bad g")
+        end
+        if h == nil then
+            error("bad h")
+        end
+    end
+
+    test()
+`
+
+	L := NewState()
+	defer L.Close()
+	if err := L.DoString(s); err != nil {
+		t.Error(err)
+	}
+}
+
+func TestMergingLoadNil(t *testing.T) {
+	// multiple nil assignments to consecutive registers should be merged
+	s := `
+		function test()
+			local a = 0
+			local b = 1
+			local c = 2
+
+			-- this should generate just one LOADNIL byte code instruction
+			a = nil
+			b = nil
+			c = nil
+
+			print(a,b,c)
+		end
+
+		test()`
+
+	chunk, err := parse.Parse(strings.NewReader(s), "test")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	compiled, err := Compile(chunk, "test")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(compiled.FunctionPrototypes) != 1 {
+		t.Fatal("expected 1 function prototype")
+	}
+
+	// there should be exactly 1 LOADNIL instruction in the byte code generated for the above
+	// anymore, and the LOADNIL merging is not working correctly
+	count := 0
+	for _, instr := range compiled.FunctionPrototypes[0].Code {
+		if opGetOpCode(instr) == OP_LOADNIL {
+			count++
+		}
+	}
+
+	if count != 1 {
+		t.Fatalf("expected 1 LOADNIL instruction, found %d", count)
+	}
+}