From ef5719d7e6e2ed02e0f1d241a977e785f0371335 Mon Sep 17 00:00:00 2001 From: Aleksandr Kirsanov Date: Fri, 3 May 2024 20:58:21 +0300 Subject: [PATCH] [FunC] Forbid impure operations inside pure functions In stdlib, all existing pure functions are asm-implemented. But since we introduced a `pure` keyword applicable to user-defined functions, we need to check that they won't have any side effects (exceptions, globals modification, etc.) --- crypto/func/abscode.cpp | 17 +------ crypto/func/analyzer.cpp | 49 +++++++++++++++---- .../func/auto-tests/tests/invalid-pure-1.fc | 18 +++++++ .../func/auto-tests/tests/invalid-pure-2.fc | 25 ++++++++++ .../func/auto-tests/tests/invalid-pure-3.fc | 23 +++++++++ .../func/auto-tests/tests/pure-functions.fc | 47 ++++++++++++++++++ crypto/func/func.h | 33 ++++++------- crypto/func/gen-abscode.cpp | 10 ++-- crypto/func/parse-func.cpp | 10 +++- 9 files changed, 181 insertions(+), 51 deletions(-) create mode 100644 crypto/func/auto-tests/tests/invalid-pure-1.fc create mode 100644 crypto/func/auto-tests/tests/invalid-pure-2.fc create mode 100644 crypto/func/auto-tests/tests/invalid-pure-3.fc create mode 100644 crypto/func/auto-tests/tests/pure-functions.fc diff --git a/crypto/func/abscode.cpp b/crypto/func/abscode.cpp index f1ffcfa4..d5e1fb0e 100644 --- a/crypto/func/abscode.cpp +++ b/crypto/func/abscode.cpp @@ -223,15 +223,6 @@ void VarDescrList::show(std::ostream& os) const { os << " ]\n"; } -void Op::flags_set_clear(int set, int clear) { - flags = (flags | set) & ~clear; - for (auto& op : block0) { - op.flags_set_clear(set, clear); - } - for (auto& op : block1) { - op.flags_set_clear(set, clear); - } -} void Op::split_vars(const std::vector& vars) { split_var_list(left, vars); split_var_list(right, vars); @@ -296,7 +287,7 @@ void Op::show(std::ostream& os, const std::vector& vars, std::string pfx if (noreturn()) { dis += " "; } - if (!is_pure()) { + if (impure()) { dis += " "; } switch (cl) { @@ -469,12 +460,6 @@ void Op::show_block(std::ostream& os, const Op* block, const std::vector os << pfx << "}"; } -void CodeBlob::flags_set_clear(int set, int clear) { - for (auto& op : ops) { - op.flags_set_clear(set, clear); - } -} - std::ostream& operator<<(std::ostream& os, const CodeBlob& code) { code.print(os); return os; diff --git a/crypto/func/analyzer.cpp b/crypto/func/analyzer.cpp index ec6931af..3e3f42ac 100644 --- a/crypto/func/analyzer.cpp +++ b/crypto/func/analyzer.cpp @@ -360,10 +360,10 @@ bool Op::compute_used_vars(const CodeBlob& code, bool edit) { case _Tuple: case _UnTuple: { // left = EXEC right; - if (!next_var_info.count_used(left) && is_pure()) { + if (!next_var_info.count_used(left) && !impure()) { // all variables in `left` are not needed if (edit) { - disable(); + set_disabled(); } return std_compute_used_vars(true); } @@ -372,7 +372,7 @@ bool Op::compute_used_vars(const CodeBlob& code, bool edit) { case _SetGlob: { // GLOB = right if (right.empty() && edit) { - disable(); + set_disabled(); } return std_compute_used_vars(right.empty()); } @@ -399,7 +399,7 @@ bool Op::compute_used_vars(const CodeBlob& code, bool edit) { } if (!cnt && edit) { // all variables in `left` are not needed - disable(); + set_disabled(); } return set_var_info(std::move(new_var_info)); } @@ -860,15 +860,45 @@ VarDescrList Op::fwd_analyze(VarDescrList values) { } } -bool Op::set_noreturn(bool nr) { - if (nr) { +void Op::set_disabled(bool flag) { + if (flag) { + flags |= _Disabled; + } else { + flags &= ~_Disabled; + } +} + + +bool Op::set_noreturn(bool flag) { + if (flag) { flags |= _NoReturn; } else { flags &= ~_NoReturn; } - return nr; + return flag; } +void Op::set_impure(const CodeBlob &code) { + // todo calling this function with `code` is a bad design (flags are assigned after Op is constructed) + // later it's better to check this somewhere in code.emplace_back() + if (code.flags & CodeBlob::_ForbidImpure) { + throw src::ParseError(where, "An impure operation in a pure function"); + } + flags |= _Impure; +} + +void Op::set_impure(const CodeBlob &code, bool flag) { + if (flag) { + if (code.flags & CodeBlob::_ForbidImpure) { + throw src::ParseError(where, "An impure operation in a pure function"); + } + flags |= _Impure; + } else { + flags &= ~_Impure; + } +} + + bool Op::mark_noreturn() { switch (cl) { case _Nop: @@ -888,13 +918,14 @@ bool Op::mark_noreturn() { case _Call: return set_noreturn(next->mark_noreturn()); case _Return: - return set_noreturn(true); + return set_noreturn(); case _If: case _TryCatch: + // note, that & | (not && ||) here and below is mandatory to invoke both left and right calls return set_noreturn((block0->mark_noreturn() & (block1 && block1->mark_noreturn())) | next->mark_noreturn()); case _Again: block0->mark_noreturn(); - return set_noreturn(true); + return set_noreturn(); case _Until: return set_noreturn(block0->mark_noreturn() | next->mark_noreturn()); case _While: diff --git a/crypto/func/auto-tests/tests/invalid-pure-1.fc b/crypto/func/auto-tests/tests/invalid-pure-1.fc new file mode 100644 index 00000000..284fa705 --- /dev/null +++ b/crypto/func/auto-tests/tests/invalid-pure-1.fc @@ -0,0 +1,18 @@ +int f_impure(); + +int f_pure() pure { + return f_impure(); +} + +int main() { + return f_pure(); +} + +{- +@compilation_should_fail +@stderr +""" +An impure operation in a pure function +return f_impure(); +""" +-} diff --git a/crypto/func/auto-tests/tests/invalid-pure-2.fc b/crypto/func/auto-tests/tests/invalid-pure-2.fc new file mode 100644 index 00000000..9def660b --- /dev/null +++ b/crypto/func/auto-tests/tests/invalid-pure-2.fc @@ -0,0 +1,25 @@ +builder begin_cell() pure asm "NEWC"; + +global int g; + +(builder) f_pure() pure { + var g; // strange, but this doesn't make a variable local, it still refers to a global one + builder b = begin_cell(); + g = g + 1; + return b; +} + +int main() { + g = 0; + f_pure(); + return g; +} + +{- +@compilation_should_fail +@stderr +""" +An impure operation in a pure function +g = g + 1; +""" +-} diff --git a/crypto/func/auto-tests/tests/invalid-pure-3.fc b/crypto/func/auto-tests/tests/invalid-pure-3.fc new file mode 100644 index 00000000..98b2fded --- /dev/null +++ b/crypto/func/auto-tests/tests/invalid-pure-3.fc @@ -0,0 +1,23 @@ +(int, int, int, int) compute_data_size?(cell c, int max_cells) pure asm "CDATASIZEQ NULLSWAPIFNOT2 NULLSWAPIFNOT"; +builder begin_cell() pure asm "NEWC"; +cell end_cell(builder b) pure asm "ENDC"; + +(int, int) validate_input(cell input) pure { + var (x, y, z, correct) = compute_data_size?(input, 10); + throw_unless(102, correct); +} + +int main() pure { + cell c = begin_cell().end_cell(); + validate_input(c); + return 0; +} + +{- +@compilation_should_fail +@stderr +""" +An impure operation in a pure function +throw_unless +""" +-} diff --git a/crypto/func/auto-tests/tests/pure-functions.fc b/crypto/func/auto-tests/tests/pure-functions.fc new file mode 100644 index 00000000..983a650f --- /dev/null +++ b/crypto/func/auto-tests/tests/pure-functions.fc @@ -0,0 +1,47 @@ +cell get_data() pure asm "c4 PUSH"; +slice begin_parse(cell c) pure asm "CTOS"; +builder begin_cell() pure asm "NEWC"; +cell end_cell(builder b) pure asm "ENDC"; +() set_data(cell c) asm "c4 POP"; + +int f_pure2() pure; + +int f_pure1() pure { + return f_pure2(); +} + +int f_pure2() pure { + return 2; +} + +(int, int) get_contract_data() pure { + cell c = get_data(); + slice cs = c.begin_parse(); + cs~load_bits(32); + int value = cs~load_uint(16); + return (1, value); +} + +() save_contract_data(int value) { + builder b = begin_cell().store_int(1, 32).store_uint(value, 16); + set_data(b.end_cell()); +} + +int test1() pure method_id(101) { + return f_pure1(); +} + +int test2(int value) method_id(102) { + save_contract_data(value); + (_, var restored) = get_contract_data(); + return restored; +} + +() main() { return (); } + +{- + +TESTCASE | 101 | | 2 +TESTCASE | 102 | 44 | 44 + +-} diff --git a/crypto/func/func.h b/crypto/func/func.h index ca844947..db1da728 100644 --- a/crypto/func/func.h +++ b/crypto/func/func.h @@ -593,16 +593,19 @@ struct Op { SymDef* _fun = nullptr) : cl(_cl), flags(0), fun_ref(_fun), where(_where), left(std::move(_left)), right(std::move(_right)) { } - bool disabled() const { - return flags & _Disabled; - } - bool enabled() const { - return !disabled(); - } - void disable() { - flags |= _Disabled; - } - void flags_set_clear(int set, int clear); + + bool disabled() const { return flags & _Disabled; } + void set_disabled() { flags |= _Disabled; } + void set_disabled(bool flag); + + bool noreturn() const { return flags & _NoReturn; } + bool set_noreturn() { flags |= _NoReturn; return true; } + bool set_noreturn(bool flag); + + bool impure() const { return flags & _Impure; } + void set_impure(const CodeBlob &code); + void set_impure(const CodeBlob &code, bool flag); + void show(std::ostream& os, const std::vector& vars, std::string pfx = "", int mode = 0) const; void show_var_list(std::ostream& os, const std::vector& idx_list, const std::vector& vars) const; void show_var_list(std::ostream& os, const std::vector& list, const std::vector& vars) const; @@ -618,17 +621,10 @@ struct Op { bool set_var_info_except(VarDescrList&& new_var_info, const std::vector& var_list); void prepare_args(VarDescrList values); VarDescrList fwd_analyze(VarDescrList values); - bool set_noreturn(bool nr); bool mark_noreturn(); - bool noreturn() const { - return flags & _NoReturn; - } bool is_empty() const { return cl == _Nop && !next; } - bool is_pure() const { - return !(flags & _Impure); - } bool generate_code_step(Stack& stack); void generate_code_all(Stack& stack); Op& last() { @@ -689,7 +685,7 @@ typedef std::vector FormalArgList; struct AsmOpList; struct CodeBlob { - enum { _AllowPostModification = 1, _ComputeAsmLtr = 2 }; + enum { _AllowPostModification = 1, _ComputeAsmLtr = 2, _ForbidImpure = 4 }; int var_cnt, in_var_cnt, op_cnt; TypeExpr* ret_type; std::string name; @@ -733,7 +729,6 @@ struct CodeBlob { pop_cur(); } void simplify_var_types(); - void flags_set_clear(int set, int clear); void prune_unreachable_code(); void fwd_analyze(); void mark_noreturn(); diff --git a/crypto/func/gen-abscode.cpp b/crypto/func/gen-abscode.cpp index 60e8c8e7..5e8a8761 100644 --- a/crypto/func/gen-abscode.cpp +++ b/crypto/func/gen-abscode.cpp @@ -229,7 +229,7 @@ var_idx_t Expr::new_tmp(CodeBlob& code) const { void add_set_globs(CodeBlob& code, std::vector>& globs, const SrcLocation& here) { for (const auto& p : globs) { auto& op = code.emplace_back(here, Op::_SetGlob, std::vector{}, std::vector{ p.second }, p.first); - op.flags |= Op::_Impure; + op.set_impure(code); } } @@ -289,7 +289,7 @@ std::vector pre_compile_tensor(const std::vector args, CodeBl if (code.flags & CodeBlob::_AllowPostModification) { if (!lval_globs && (var.cls & TmpVar::_Named)) { Op *op = &code.emplace_back(nullptr, Op::_Let, std::vector(), std::vector()); - op->flags |= Op::_Disabled; + op->set_disabled(); var.on_modification.push_back([modified_vars, i, j, op, done = false](const SrcLocation &here) mutable { if (!done) { done = true; @@ -319,7 +319,7 @@ std::vector pre_compile_tensor(const std::vector args, CodeBl var_idx_t v2 = code.create_tmp_var(code.vars[v].v_type, code.vars[v].where.get()); m.op->left = {v2}; m.op->right = {v}; - m.op->flags &= ~Op::_Disabled; + m.op->set_disabled(false); v = v2; } std::vector res; @@ -371,7 +371,7 @@ std::vector Expr::pre_compile(CodeBlob& code, std::vector Expr::pre_compile(CodeBlob& code, std::vectorsym); if (args[0]->flags & _IsImpure) { - op.flags |= Op::_Impure; + op.set_impure(code); } return rvect; } else { diff --git a/crypto/func/parse-func.cpp b/crypto/func/parse-func.cpp index bc298caa..a9c771df 100644 --- a/crypto/func/parse-func.cpp +++ b/crypto/func/parse-func.cpp @@ -1216,7 +1216,7 @@ blk_fl::val parse_stmt(Lexer& lex, CodeBlob& code) { } } -CodeBlob* parse_func_body(Lexer& lex, FormalArgList arg_list, TypeExpr* ret_type) { +CodeBlob* parse_func_body(Lexer& lex, FormalArgList arg_list, TypeExpr* ret_type, bool marked_as_pure) { lex.expect('{'); CodeBlob* blob = new CodeBlob{ret_type}; if (pragma_allow_post_modification.enabled()) { @@ -1225,6 +1225,9 @@ CodeBlob* parse_func_body(Lexer& lex, FormalArgList arg_list, TypeExpr* ret_type if (pragma_compute_asm_ltr.enabled()) { blob->flags |= CodeBlob::_ComputeAsmLtr; } + if (marked_as_pure) { + blob->flags |= CodeBlob::_ForbidImpure; + } blob->import_params(std::move(arg_list)); blk_fl::val res = blk_fl::init; bool warned = false; @@ -1604,7 +1607,10 @@ void parse_func_def(Lexer& lex) { if (func_sym_code->code) { lex.cur().error("redefinition of function `"s + func_name.str + "`"); } - CodeBlob* code = parse_func_body(lex, arg_list, ret_type); + if (marked_as_pure && ret_type->get_width() == 0) { + lex.cur().error("a pure function should return something, otherwise it will be optimized out anyway"); + } + CodeBlob* code = parse_func_body(lex, arg_list, ret_type, marked_as_pure); code->name = func_name.str; code->loc = loc; // code->print(std::cerr); // !!!DEBUG!!!