diff --git a/compile.go b/compile.go index e46e0d7..f9fbf57 100644 --- a/compile.go +++ b/compile.go @@ -236,12 +236,13 @@ 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 && (opGetArgB(last)+1) == a { - cd.SetB(cd.LastPC(), b) - } else { - cd.AddABC(OP_LOADNIL, a, b, 0, line) - } + // this method used to merge multiple consecutive LOADNIL instructions + // of consecutive registers into a single LOADNIL instruction, but it + // caused issues when the merged instructions were JMP targets, and so + // generated invalid code; so the merging functionality has been removed. + // It is safe to merge the LOADNIL instructions under certain conditions, + // but additional logic / complexity would be needed here. + cd.AddABC(OP_LOADNIL, a, b, 0, line) } func (cd *codeStore) SetOpCode(pc int, v int) { diff --git a/script_test.go b/script_test.go index 48f48b3..3358b36 100644 --- a/script_test.go +++ b/script_test.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "runtime" - "strings" "sync/atomic" "testing" "time" @@ -148,6 +147,33 @@ func TestLua(t *testing.T) { testScriptDir(t, luaTests, "_lua5.1-tests") } +func TestMergingLoadNilBug2(t *testing.T) { + // there was a bug where the LOADNIL merging optimisation would merge LOADNILs that were the targets of + // JMP instructions, causing the JMP to jump to the wrong location and breaking the logic and resulting in + // a panic. + s := ` + id = "foo" + + function get_def() + return {} + end + + function test() + local def = id ~= nil and get_def() or nil + if def ~= nil then + print("def is not nil") + end + end + + test() +` + L := NewState() + defer L.Close() + if err := L.DoString(s); err != nil { + t.Error(err) + } +} + 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 := ` @@ -185,48 +211,50 @@ func TestMergingLoadNilBug(t *testing.T) { } } -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) - } -} +// This test is disabled because the LOADNIL merging optimisation has been disabled. See the comment in the +// AddLoadNil() function for more information. +//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) +// } +//}