From 302a77a3259fa437d5adce9511c223ec70ceda33 Mon Sep 17 00:00:00 2001 From: Daniel Collins Date: Sun, 12 Feb 2023 01:58:03 +0000 Subject: [PATCH] Fix template variable scoping issues (#197, #191, #190). --- plugins/binary-template/executor.lua | 91 ++++++--- .../executor/arrayindexvalue.lua | 71 +++++++ plugins/binary-template/executor_spec.lua | 179 +++++++++++++++++- 3 files changed, 312 insertions(+), 29 deletions(-) create mode 100644 plugins/binary-template/executor/arrayindexvalue.lua diff --git a/plugins/binary-template/executor.lua b/plugins/binary-template/executor.lua index 5369b24a..e0f974c4 100644 --- a/plugins/binary-template/executor.lua +++ b/plugins/binary-template/executor.lua @@ -1,5 +1,5 @@ -- Binary Template plugin for REHex --- Copyright (C) 2021-2022 Daniel Collins +-- Copyright (C) 2021-2023 Daniel Collins -- -- This program is free software; you can redistribute it and/or modify it -- under the terms of the GNU General Public License version 2 as published by @@ -14,12 +14,13 @@ -- this program; if not, write to the Free Software Foundation, Inc., 51 -- Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -local ArrayValue = require 'executor.arrayvalue' -local FileArrayValue = require 'executor.filearrayvalue' -local FileValue = require 'executor.filevalue' -local ImmediateValue = require 'executor.immediatevalue' -local PlainValue = require 'executor.plainvalue' -local StructValue = require 'executor.structvalue' +local ArrayIndexValue = require 'executor.arrayindexvalue' +local ArrayValue = require 'executor.arrayvalue' +local FileArrayValue = require 'executor.filearrayvalue' +local FileValue = require 'executor.filevalue' +local ImmediateValue = require 'executor.immediatevalue' +local PlainValue = require 'executor.plainvalue' +local StructValue = require 'executor.structvalue' local M = {} @@ -508,6 +509,39 @@ local function _make_value_from_value(context, dst_type, src_type, src_val, move end end +local function _build_parent_scope_vars(context) + local parent_scope_vars = {} + + for frame_idx = #context.stack, 1, -1 + do + local frame = context.stack[frame_idx] + + for k,v in pairs(frame.vars) + do + if parent_scope_vars[k] == nil + then + parent_scope_vars[k] = v + end + end + + if frame.frame_type == FRAME_TYPE_FUNCTION or frame.frame_type == FRAME_TYPE_STRUCT + then + for k,v in pairs(frame.parent_scope_vars) + do + if parent_scope_vars[k] == nil + then + parent_scope_vars[k] = v + end + end + + -- Don't look for variables beyond containing scope + break + end + end + + return parent_scope_vars +end + local INT8_MIN = -128 local INT8_MAX = 127 local INT16_MIN = -32768 @@ -579,8 +613,10 @@ _builtin_types = { } local _builtin_variables = { - ["true"] = { _builtin_types.int, ImmediateValue:new(1) }, - ["false"] = { _builtin_types.int, ImmediateValue:new(0) }, + ["true"] = function(context) return { _builtin_types.int, ImmediateValue:new(1) } end, + ["false"] = function(context) return { _builtin_types.int, ImmediateValue:new(0) } end, + + ["ArrayIndex"] = function(context) return { _builtin_types.int64_t, ArrayIndexValue:new(context) } end, } local function _builtin_function_BigEndian(context, argv) @@ -788,7 +824,7 @@ local function _resize_array(context, array_type, array_value, new_length, struc for i = #array_value, new_length - 1 do - table.insert(array_value, expand_value(context, element_type, struct_arg_values, true)) + table.insert(array_value, expand_value(context, element_type, struct_arg_values, i)) context.interface.yield() end end @@ -1085,16 +1121,16 @@ _eval_ref = function(context, statement) if frame.frame_type == FRAME_TYPE_FUNCTION or frame.frame_type == FRAME_TYPE_STRUCT then + if frame.parent_scope_vars[ path[1] ] ~= nil + then + return _walk_path(frame.parent_scope_vars[ path[1] ]) + end + -- Don't look for variables in parent functions break end end - if context.global_vars[ path[1] ] ~= nil - then - return _walk_path(context.global_vars[ path[1] ]) - end - _template_error(context, "Attempt to use undefined variable '" .. path[1] .. "'") end @@ -1318,7 +1354,7 @@ _eval_unary_minus = function(context, statement) return value_t, ImmediateValue:new(-1 * value_v:get()) end -expand_value = function(context, type_info, struct_arg_values, is_array_element) +expand_value = function(context, type_info, struct_arg_values, array_element_idx) if type_info.base == "struct" then local args_ok = true @@ -1380,7 +1416,9 @@ expand_value = function(context, type_info, struct_arg_values, is_array_element) frame_type = FRAME_TYPE_STRUCT, var_types = {}, vars = {}, + parent_scope_vars = type_info.struct_parent_scope_vars, struct_members = members, + array_element_idx = array_element_idx, blocks_flowctrl_types = (FLOWCTRL_TYPE_RETURN | FLOWCTRL_TYPE_BREAK | FLOWCTRL_TYPE_CONTINUE), } @@ -1486,7 +1524,7 @@ local function _decl_variable(context, statement, var_type, var_name, struct_arg then dest_tables = { context.stack[#context.stack].vars } else - dest_tables = { context.global_vars } + dest_tables = { context.stack[#context.stack].vars, context.global_vars } end if dest_tables[1][var_name] ~= nil @@ -1534,7 +1572,7 @@ local function _decl_variable(context, statement, var_type, var_name, struct_arg then local base_off = context.next_variable - root_value = expand_value(context, type_info, struct_arg_values, false) + root_value = expand_value(context, type_info, struct_arg_values, nil) for _,t in ipairs(dest_tables) do @@ -1581,7 +1619,7 @@ local function _decl_variable(context, statement, var_type, var_name, struct_arg for i = 0, ArrayLength_val:get() - 1 do - local value = expand_value(context, type_info, struct_arg_values, true) + local value = expand_value(context, type_info, struct_arg_values, i) table.insert(root_value, value) context.interface.yield() @@ -1815,11 +1853,14 @@ _eval_func_defn = function(context, statement) table.insert(arg_types, type_info) end + local parent_scope_vars = _build_parent_scope_vars(context) + local impl_func = function(context, arguments) local frame = { frame_type = FRAME_TYPE_FUNCTION, var_types = {}, vars = {}, + parent_scope_vars = parent_scope_vars, handles_flowctrl_types = FLOWCTRL_TYPE_RETURN, blocks_flowctrl_types = (FLOWCTRL_TYPE_BREAK | FLOWCTRL_TYPE_CONTINUE), @@ -1925,6 +1966,8 @@ _eval_struct_defn = function(context, statement) type_key = {}, -- Uniquely-identifiable table reference used to check if struct -- types are derived from the same root (and thus compatible) + struct_parent_scope_vars = _build_parent_scope_vars(context), + -- rehex_type_le = "s8", -- rehex_type_be = "s8", -- length = 1, @@ -2491,14 +2534,6 @@ local function execute(interface, statements) -- Each element points to a tuple of { type, value } like other rvalues. global_vars = {}, - -- This table holds our current position in global_vars - under which new variables - -- be added and relative to which any lookups should be performed. - -- - -- Each key will either be a tuple of { key, table }, where the key is a string - -- when we are descending into a struct member or a number where we are in an array - -- and the table is a reference to the element in global_vars at this point. - global_stack = {}, - next_variable = 0, -- Are we currently accessing variables from the file as big endian? Toggled by @@ -2527,7 +2562,7 @@ local function execute(interface, statements) local base_vars = {} for k, v in pairs(_builtin_variables) do - base_vars[k] = v + base_vars[k] = v(context) end table.insert(context.stack, { diff --git a/plugins/binary-template/executor/arrayindexvalue.lua b/plugins/binary-template/executor/arrayindexvalue.lua new file mode 100644 index 00000000..7d8bfda2 --- /dev/null +++ b/plugins/binary-template/executor/arrayindexvalue.lua @@ -0,0 +1,71 @@ +-- Binary Template plugin for REHex +-- Copyright (C) 2023 Daniel Collins +-- +-- This program is free software; you can redistribute it and/or modify it +-- under the terms of the GNU General Public License version 2 as published by +-- the Free Software Foundation. +-- +-- This program is distributed in the hope that it will be useful, but WITHOUT +-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +-- FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +-- more details. +-- +-- You should have received a copy of the GNU General Public License along with +-- this program; if not, write to the Free Software Foundation, Inc., 51 +-- Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +--- Value (as in lvalue) class for the global ArrayIndex variable. + +local ArrayIndexValue = {} +ArrayIndexValue.__index = ArrayIndexValue + +-- Copied from executor.lua - ugh +local FRAME_TYPE_STRUCT = "struct" +local FRAME_TYPE_FUNCTION = "function" + +--- Construct a new ArrayIndexValue object +-- +-- @param context Reference to executor internal context table +-- +function ArrayIndexValue:new(context) + local self = { + context = context, + } + + setmetatable(self, ArrayIndexValue) + return self +end + +function ArrayIndexValue:get() + for frame_idx = #self.context.stack, 1, -1 + do + local frame = self.context.stack[frame_idx] + + if frame.frame_type == FRAME_TYPE_FUNCTION or frame.frame_type == FRAME_TYPE_STRUCT + then + if frame.array_element_idx ~= nil + then + return frame.array_element_idx + end + + -- Don't look for variables beyond containing scope + break + end + end + + self.context.template_error(self.context, "Attempt to read ArrayIndex variable outside of an array element") +end + +function ArrayIndexValue:set(value) + self.context.template_error(self.context, "Attempt to write to ArrayIndex variable") +end + +function ArrayIndexValue:copy() + return ArrayIndexValue:new(self.context) +end + +function ArrayIndexValue:data_range() + return -- Not backed by file data +end + +return ArrayIndexValue diff --git a/plugins/binary-template/executor_spec.lua b/plugins/binary-template/executor_spec.lua index dc429642..4af227b6 100644 --- a/plugins/binary-template/executor_spec.lua +++ b/plugins/binary-template/executor_spec.lua @@ -1,5 +1,5 @@ -- Binary Template plugin for REHex --- Copyright (C) 2021-2022 Daniel Collins +-- Copyright (C) 2021-2023 Daniel Collins -- -- This program is free software; you can redistribute it and/or modify it -- under the terms of the GNU General Public License version 2 as published by @@ -6657,4 +6657,181 @@ describe("executor", function() }) end, "Cannot use type 'string' to declare a file variable at test.bt:2") end) + + it("exposes ArrayIndex variable in struct array elements", function() + local interface, log = test_interface(string.char( + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00 + )) + + executor.execute(interface, { + { "test.bt", 1, "struct", "B", {}, + { + { "test.bt", 2, "call", "Printf", { + { "test.bt", 2, "str", "B ArrayIndex = %d" }, + { "test.bt", 2, "ref", { "ArrayIndex" } } } }, + + { "test.bt", 3, "variable", "char", "x", nil, nil }, + } }, + + { "test.bt", 5, "struct", "A", {}, + { + { "test.bt", 6, "call", "Printf", { + { "test.bt", 6, "str", "A ArrayIndex = %d" }, + { "test.bt", 6, "ref", { "ArrayIndex" } } } }, + + { "test.bt", 7, "variable", "struct B", "b", nil, { "test.bt", 1, "num", 3 } }, + } }, + + { "test.bt", 9, "variable", "struct A", "a", nil, { "test.bt", 1, "num", 2 } }, + }) + + local expect_log = { + "print(A ArrayIndex = 0)", + "print(B ArrayIndex = 0)", + "print(B ArrayIndex = 1)", + "print(B ArrayIndex = 2)", + "print(A ArrayIndex = 1)", + "print(B ArrayIndex = 0)", + "print(B ArrayIndex = 1)", + "print(B ArrayIndex = 2)", + + "set_comment(0, 1, x)", + "set_comment(0, 1, b[0])", + "set_comment(1, 1, x)", + "set_comment(1, 1, b[1])", + "set_comment(2, 1, x)", + "set_comment(2, 1, b[2])", + "set_comment(0, 3, a[0])", + "set_comment(3, 1, x)", + "set_comment(3, 1, b[0])", + "set_comment(4, 1, x)", + "set_comment(4, 1, b[1])", + "set_comment(5, 1, x)", + "set_comment(5, 1, b[2])", + "set_comment(3, 3, a[1])", + "set_data_type(0, 1, s8)", + "set_data_type(1, 1, s8)", + "set_data_type(2, 1, s8)", + "set_data_type(3, 1, s8)", + "set_data_type(4, 1, s8)", + "set_data_type(5, 1, s8)", + } + + assert.are.same(expect_log, log) + end) + + it("errors when modifying ArrayIndex variable", function() + local interface, log = test_interface() + + assert.has_error(function() + executor.execute(interface, { + { "test.bt", 1, "struct", "A", {}, + { + { "test.bt", 2, "assign", + { "test.bt", 2, "ref", { "ArrayIndex" } }, + { "test.bt", 2, "num", 5678 } }, + } }, + + { "test.bt", 9, "variable", "struct A", "a", nil, { "test.bt", 1, "num", 2 } }, + }) + end, "Attempt to write to ArrayIndex variable at test.bt:2") + end) + + it("errors on use of ArrayIndex in non-array-element struct", function() + local interface, log = test_interface() + + assert.has_error(function() + executor.execute(interface, { + { "test.bt", 1, "struct", "B", {}, + { + { "test.bt", 2, "call", "Printf", { + { "test.bt", 2, "str", "B ArrayIndex = %d" }, + { "test.bt", 2, "ref", { "ArrayIndex" } } } }, + + { "test.bt", 3, "variable", "char", "x", nil, nil }, + } }, + + { "test.bt", 5, "struct", "A", {}, + { + { "test.bt", 6, "call", "Printf", { + { "test.bt", 6, "str", "A ArrayIndex = %d" }, + { "test.bt", 6, "ref", { "ArrayIndex" } } } }, + + { "test.bt", 7, "variable", "struct B", "b", nil, nil }, + } }, + + { "test.bt", 9, "variable", "struct A", "a", nil, { "test.bt", 1, "num", 2 } }, + }) + end, "Attempt to read ArrayIndex variable outside of an array element at test.bt:2") + end) + + it("allows use of global defined before a function inside the function", function() + local interface, log = test_interface(string.char( + 0xD2, 0x04, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00 + )) + + executor.execute(interface, { + { "test.bt", 1, "variable", "int", "filevar", nil, nil }, + { "test.bt", 2, "local-variable", "int", "localvar", nil, nil, { "test.bt", 2, "num", 5678 } }, + + { "test.bt", 4, "function", "void", "foo", {}, { + { "test.bt", 5, "call", "Printf", { { "test.bt", 5, "str", "filevar = %d" }, { "test.bt", 5, "ref", { "filevar" } } } }, + { "test.bt", 6, "call", "Printf", { { "test.bt", 6, "str", "localvar = %d" }, { "test.bt", 6, "ref", { "localvar" } } } }, + } }, + + { "test.bt", 8, "call", "foo", {} }, + }) + + local expect_log = { + "print(filevar = 1234)", + "print(localvar = 5678)", + + "set_comment(0, 4, filevar)", + "set_data_type(0, 4, s32le)", + } + + assert.are.same(expect_log, log) + end) + + it("errors on use of global defined after a function inside the function", function() + local interface, log = test_interface(string.char( + 0xD2, 0x04, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00 + )) + + assert.has_error(function() + executor.execute(interface, { + { "test.bt", 1, "function", "void", "foo", {}, { + { "test.bt", 2, "call", "Printf", { { "test.bt", 2, "str", "filevar = %d" }, { "test.bt", 2, "ref", { "filevar" } } } } } }, + + { "test.bt", 4, "variable", "int", "filevar", nil, nil }, + { "test.bt", 5, "local-variable", "int", "localvar", nil, nil, { "test.bt", 5, "num", 5678 } }, + + { "test.bt", 7, "call", "foo", {} }, + }) + end, "Attempt to use undefined variable 'filevar' at test.bt:2") + + assert.has_error(function() + executor.execute(interface, { + { "test.bt", 1, "function", "void", "foo", {}, { + { "test.bt", 2, "call", "Printf", { { "test.bt", 2, "str", "localvar = %d" }, { "test.bt", 2, "ref", { "localvar" } } } } } }, + + { "test.bt", 4, "variable", "int", "filevar", nil, nil }, + { "test.bt", 5, "local-variable", "int", "localvar", nil, nil, { "test.bt", 5, "num", 5678 } }, + + { "test.bt", 7, "call", "foo", {} }, + }) + end, "Attempt to use undefined variable 'localvar' at test.bt:2") + end) end)