Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LOADNIL merging optimisation (still) produces invalid code #522

Open
2 tasks done
tul opened this issue Jan 10, 2025 · 0 comments · May be fixed by #523
Open
2 tasks done

LOADNIL merging optimisation (still) produces invalid code #522

tul opened this issue Jan 10, 2025 · 0 comments · May be fixed by #523

Comments

@tul
Copy link
Contributor

tul commented Jan 10, 2025

You must post issues only here. Questions, ideas must be posted in discussions.

  • GopherLua is a Lua5.1 implementation. You should be familiar with Lua programming language. Have you read Lua 5.1 reference manual carefully?
  • 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? : ccacf662c9d24821ea6cb1abaa515cc3b6aad7c6
  2. What version of Go are you using? : 1.22.3
  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 lastest ccacf662c9d24821ea6cb1abaa515cc3b6aad7c6.

Under certain conditions, the compiler can produce invalid code, resulting in a nil pointer deference panic.

The following code will reproduce the bug:

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()

Invoke with:

> ./glua  test3.lua
runtime error: invalid memory address or nil pointer dereference
stack traceback:
test3.lua:9: in function 'test'
test3.lua:14: in main chunk
[G]: ?

The issue is caused by incorrect byte code generation, due to another issue with the LOADNIL merging optimisation (see previously resolved issue #495).

The micro code generated by the above should generate:

[001] GETGLOBAL      |  0, 0; R(0) := Gbl[Kst(0)] (line:8)
[002] LOADNIL      |  1, 1, 0; R(1) := ... := R(1) := nil (line:8)
[003] EQ      |  1, 0, 1; if ((RK(0) == RK(1)) ~= 1) then pc++ (line:8)
[004] JMP      |  0, 4; pc+=4 (line:8)
[005] GETGLOBAL      |  0, 1; R(0) := Gbl[Kst(1)] (line:8)
[006] CALL      |  0, 1, 2; R(0) ... R(0+2-2) := R(0)(R(0+1) ... R(0+1-1)) (line:8)
[007] TESTSET      |  0, 0, 1; if (R(0) <=> 1) then R(0) := R(0) else pc++ (line:8)
[008] JMP      |  0, 1; pc+=1 (line:8)                                                  >>>>v   CORRECT JMP TARGET
[009] LOADNIL      |  0, 0, 0; R(0) := ... := R(0) := nil (line:8)                          v
[010] LOADNIL      |  1, 1, 0; R(1) := ... := R(1) := nil (line:9)                      <<<<<
[011] EQ      |  1, 0, 1; if ((RK(0) == RK(1)) ~= 1) then pc++ (line:9)
[012] JMP      |  0, 3; pc+=3 (line:9)
[013] GETGLOBAL      |  1, 2; R(1) := Gbl[Kst(2)] (line:10)
[014] LOADK      |  2, 3; R(2) := Kst(3) (line:10)
[015] CALL      |  1, 2, 1; R(1) ... R(1+1-2) := R(1)(R(1+1) ... R(1+2-1)) (line:10)
[016] RETURN      |  0, 1, 0; return R(0) ... R(0+1-2) (line:12)
; end of function

But instead, it generates:

[001] GETGLOBAL      |  0, 0; R(0) := Gbl[Kst(0)] (line:8)
[002] LOADNIL      |  1, 1, 0; R(1) := ... := R(1) := nil (line:8)
[003] EQ      |  1, 0, 1; if ((RK(0) == RK(1)) ~= 1) then pc++ (line:8)
[004] JMP      |  0, 4; pc+=4 (line:8)
[005] GETGLOBAL      |  0, 1; R(0) := Gbl[Kst(1)] (line:8)
[006] CALL      |  0, 1, 2; R(0) ... R(0+2-2) := R(0)(R(0+1) ... R(0+1-1)) (line:8)
[007] TESTSET      |  0, 0, 1; if (R(0) <=> 1) then R(0) := R(0) else pc++ (line:8)
[008] JMP      |  0, 1; pc+=1 (line:8)                                              >>>>v   INCORRECT JMP TARGET
[009] LOADNIL      |  0, 1, 0; R(0) := ... := R(1) := nil (line:8)                      v
[010] EQ      |  1, 0, 1; if ((RK(0) == RK(1)) ~= 1) then pc++ (line:9)             <<<<<
[011] JMP      |  0, 3; pc+=3 (line:9)
[012] GETGLOBAL      |  1, 2; R(1) := Gbl[Kst(2)] (line:10)
[013] LOADK      |  2, 3; R(2) := Kst(3) (line:10)
[014] CALL      |  1, 2, 1; R(1) ... R(1+1-2) := R(1)(R(1+1) ... R(1+2-1)) (line:10)
[015] RETURN      |  0, 1, 0; return R(0) ... R(0+1-2) (line:12)
; end of function

Note, the LOADNIL instructions on lines 9 and 10 have been merged by the optimiser, but this is incorrect. The
2nd LOADNIL instruction was a jump target for the JMP on line 8, and merging it into the prior instruction meant
the JMP missed its target, and that the the LOADNIL for reg 1 was skipped. This resulted in an uninitialised
register value being used in subsequent operations, causing the panic.

Summary

The LOADNIL merging optimisation merges multiple LOADNIL instructions of consecutive registers into a single
instruction, however it does not take into account if any of the LOADNIL instructions are jump targets, and so breaks
the logic.
@tul tul linked a pull request Jan 10, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

disable LOADNIL code generation optimisation tul/gopher-lua
1 participant
@tul