From c90265351b9f967be58b404630bfb28cf3916eb5 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sat, 7 Jan 2023 20:33:36 -0800 Subject: [PATCH 1/4] Duplicate default function/task argument expressions The default value for a function or task argument is elaborated once and then used for each function invocation where no actual value is provided. This means if a function or task is called multiple times the same NetExpr is passed as a sub-expression to multiple statements or expressions such as the function call. This is causing problems because each expression or statement expects to have exclusive ownership over its sub-expressions. It can for example result in a double free or other undefined behavior. To mitigate this duplicate the default argument expression before it is given as a sub-expression to another expression or statement. Signed-off-by: Lars-Peter Clausen --- elab_expr.cc | 4 ++-- elaborate.cc | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/elab_expr.cc b/elab_expr.cc index 7db1f21ef..e77d60f7b 100644 --- a/elab_expr.cc +++ b/elab_expr.cc @@ -2954,7 +2954,7 @@ unsigned PECallFunction::elaborate_arguments_(Design*des, NetScope*scope, << "requires SystemVerilog." << endl; des->errors += 1; } - parms[pidx] = def->port_defe(pidx); + parms[pidx] = def->port_defe(pidx)->dup_expr(); } else { missing_parms += 1; @@ -6573,7 +6573,7 @@ NetExpr* PENewClass::elaborate_expr_constructor_(Design*des, NetScope*scope, // Ran out of explicit arguments. Is there a default // argument we can use? if (NetExpr*tmp = def->port_defe(idx)) { - parms[idx] = tmp; + parms[idx] = tmp->dup_expr(); continue; } diff --git a/elaborate.cc b/elaborate.cc index 95effd6ea..6890e2b77 100644 --- a/elaborate.cc +++ b/elaborate.cc @@ -3344,7 +3344,7 @@ NetProc* PChainConstructor::elaborate(Design*des, NetScope*scope) const } if (NetExpr*tmp = def->port_defe(idx)) { - parms[idx] = tmp; + parms[idx] = tmp->dup_expr(); continue; } @@ -4173,9 +4173,9 @@ NetProc* PCallTask::elaborate_build_call_(Design*des, NetScope*scope, "requires SystemVerilog." << endl; des->errors += 1; } - rv = def->port_defe(idx); + rv = def->port_defe(idx)->dup_expr(); if (lv_type==IVL_VT_BOOL||lv_type==IVL_VT_LOGIC) - rv = pad_to_width(rv->dup_expr(), wid, *this); + rv = pad_to_width(rv, wid, *this); } else { cerr << get_fileline() << ": error: " From aea202b2e910df81f8db8373d75b71984a0d8089 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Tue, 3 Jan 2023 08:08:09 -0800 Subject: [PATCH 2/4] PECallFunction: Handle empty parameters in has_aa_term() and declare_implicit_nets() A function parameter can be an empty value, in which case its expression is a nullptr and can not be dereferenced. Make sure this case is handled in the has_aa_term() and declare_implicit_nets() methods. Signed-off-by: Lars-Peter Clausen --- PExpr.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/PExpr.cc b/PExpr.cc index 8454ad930..d898dbf6e 100644 --- a/PExpr.cc +++ b/PExpr.cc @@ -286,7 +286,8 @@ PECallFunction::~PECallFunction() void PECallFunction::declare_implicit_nets(LexicalScope*scope, NetNet::Type type) { for (unsigned idx = 0 ; idx < parms_.size() ; idx += 1) { - parms_[idx]->declare_implicit_nets(scope, type); + if (parms_[idx]) + parms_[idx]->declare_implicit_nets(scope, type); } } @@ -294,7 +295,8 @@ bool PECallFunction::has_aa_term(Design*des, NetScope*scope) const { bool flag = false; for (unsigned idx = 0 ; idx < parms_.size() ; idx += 1) { - flag = parms_[idx]->has_aa_term(des, scope) || flag; + if (parms_[idx]) + flag |= parms_[idx]->has_aa_term(des, scope); } return flag; } From 6cf19ec9640cffdec5c131f3b8c03407fe084d8b Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Mon, 2 Jan 2023 18:59:46 -0800 Subject: [PATCH 3/4] Fixup empty function/task argument lists in a consistent way As a quirk of the (System)Verilog grammar a function or task argument list with no arguments can not be distinguished from a argument list with a single empty argument. The iverilog parses it as the latter. There are currently many places in the code base where this is fixed up in slightly different ways. Fix this up in the parser in a central way before passing the arguments to the elaboration stage. The existing implementation in some cases removes all empty trailing arguments. While this works to handle the case for zero arguments it also hides some errors that should be detected. E.g. in the following 3 arguments are passed to a function which only takes two arguments. But no error is reported since the explicitly specified empty arguments are removed. ``` function f(integer a, integer b = 2); ... endfunction f(1,,); ``` In the new implementation the empty argument will only be removed if there is exactly one empty argument in the argument list. Signed-off-by: Lars-Peter Clausen --- elab_expr.cc | 16 +-------------- elaborate.cc | 16 --------------- net_func.cc | 9 -------- parse.y | 58 ++++++++++++++++++++++++++-------------------------- 4 files changed, 30 insertions(+), 69 deletions(-) diff --git a/elab_expr.cc b/elab_expr.cc index e77d60f7b..208dd8391 100644 --- a/elab_expr.cc +++ b/elab_expr.cc @@ -1816,17 +1816,7 @@ NetExpr* PECallFunction::elaborate_sfunc_(Design*des, NetScope*scope, return cast_to_width_(sub, expr_wid); } - /* How many parameters are there? The Verilog language allows - empty parameters in certain contexts, so the parser will - allow things like func(1,,3). It will also cause func() to - be interpreted as a single empty parameter. - - Functions cannot really take empty parameters, but the - case ``func()'' is the same as no parameters at all. So - catch that special case here. */ unsigned nparms = parms_.size(); - if ((nparms == 1) && (parms_[0] == 0)) - nparms = 0; NetESFunc*fun = new NetESFunc(name, expr_type_, expr_width_, nparms, is_overridden_); fun->set_line(*this); @@ -2907,11 +2897,7 @@ unsigned PECallFunction::elaborate_arguments_(Design*des, NetScope*scope, const unsigned parm_count = parms.size() - parm_off; const unsigned actual_count = parms_.size(); - /* The parser can't distinguish between a function call with - no arguments and a function call with one empty argument, - and always supplies one empty argument. Handle the no - argument case here. */ - if ((parm_count == 0) && (actual_count == 1) && (parms_[0] == 0)) + if (parm_count == 0 && actual_count == 0) return 0; if (actual_count > parm_count) { diff --git a/elaborate.cc b/elaborate.cc index 6890e2b77..264663bb7 100644 --- a/elaborate.cc +++ b/elaborate.cc @@ -3508,13 +3508,6 @@ NetProc* PCallTask::elaborate_sys(Design*des, NetScope*scope) const } unsigned parm_count = parms_.size(); - - /* Catch the special case that the system task has no - parameters. The "()" string will be parsed as a single - empty parameter, when we really mean no parameters at all. */ - if ((parm_count== 1) && (parms_[0] == 0)) - parm_count = 0; - vectoreparms (parm_count); perm_string name = peek_tail_name(path_); @@ -3660,10 +3653,7 @@ NetProc* PCallTask::elaborate_sys_task_method_(Design*des, NetScope*scope, NetESignal*sig = new NetESignal(net); sig->set_line(*this); - /* If there is a single NULL argument then ignore it since it is - * left over from the parser and is not needed by the method. */ unsigned nparms = parms_.size(); - if ((nparms == 1) && (parms_[0] == 0)) nparms = 0; vectorargv (1 + nparms); argv[0] = sig; @@ -4356,12 +4346,6 @@ bool PCallTask::elaborate_elab(Design*des, NetScope*scope) const unsigned parm_count = parms_.size(); - /* Catch the special case that the elaboration task has no - parameters. The "()" string will be parsed as a single - empty parameter, when we really mean no parameters at all. */ - if ((parm_count== 1) && (parms_[0] == 0)) - parm_count = 0; - perm_string name = peek_tail_name(path_); if (!gn_system_verilog()) { diff --git a/net_func.cc b/net_func.cc index a2ce368d3..3f3a5610f 100644 --- a/net_func.cc +++ b/net_func.cc @@ -82,15 +82,6 @@ bool PECallFunction::check_call_matches_definition_(Design*des, NetScope*dscope) { assert(dscope); - /* How many parameters have I got? Normally the size of the - list is correct, but there is the special case of a list of - 1 nil pointer. This is how the parser tells me of no - parameter. In other words, ``func()'' is 1 nil parameter. */ - - unsigned parms_count = parms_.size(); - if ((parms_count == 1) && (parms_[0] == 0)) - parms_count = 0; - if (dscope->type() != NetScope::FUNC) { cerr << get_fileline() << ": error: Attempt to call scope " << scope_path(dscope) << " as a function." << endl; diff --git a/parse.y b/parse.y index 4599f7ccc..694042529 100644 --- a/parse.y +++ b/parse.y @@ -174,15 +174,13 @@ template void append(vector&out, const std::vector&in) } /* - * Look at the list and pull null pointers off the end. + * The parser parses an empty argument list as an argument list with an single + * empty argument. Fix this up here and replace it with an empty list. */ -static void strip_tail_items(list*lst) +static void argument_list_fixup(list*lst) { - while (! lst->empty()) { - if (lst->back() != 0) - return; - lst->pop_back(); - } + if (lst->size() == 1 && !lst->front()) + lst->clear(); } /* @@ -641,7 +639,7 @@ static void current_function_set_statement(const YYLTYPE&loc, std::vector delay_value delay_value_simple %type delay1 delay3 delay3_opt delay_value_list %type expression_list_with_nuls expression_list_proper -%type argument_list_parens_opt +%type argument_list_parens argument_list_parens_opt %type cont_assign cont_assign_list %type variable_decl_assignment @@ -984,9 +982,7 @@ class_scope class_new /* IEEE1800-2005 A.2.4 */ : K_new argument_list_parens_opt - { std::list*expr_list = $2; - strip_tail_items(expr_list); - PENewClass*tmp = new PENewClass(*expr_list); + { PENewClass*tmp = new PENewClass(*$2); FILE_NAME(tmp, @1); delete $2; $$ = tmp; @@ -994,9 +990,7 @@ class_new /* IEEE1800-2005 A.2.4 */ // This can't be a class_scope_opt because it will lead to shift/reduce // conflicts with array_new | class_scope K_new argument_list_parens_opt - { std::list*expr_list = $3; - strip_tail_items(expr_list); - PENewClass *new_expr = new PENewClass(*expr_list, $1); + { PENewClass *new_expr = new PENewClass(*$3, $1); FILE_NAME(new_expr, @2); delete $3; $$ = new_expr; @@ -3620,12 +3614,22 @@ expression_list_with_nuls } ; + /* An argument list enclosed in parenthesis. The parser will parse '()' as a + * argument list with an single empty item. We fix this up once the list + * parsing is done by replacing it with the empty list. + */ +argument_list_parens + : '(' expression_list_with_nuls ')' + { argument_list_fixup($2); + $$ = $2; } + ; + /* A task or function can be invoked with the task/function name followed by * an argument list in parenthesis or with just the task/function name by * itself. When an argument list is used it might be empty. */ argument_list_parens_opt - : '(' expression_list_with_nuls ')' - { $$ = $2; } + : argument_list_parens + { $$ = $1; } | { $$ = new std::list; } @@ -3757,21 +3761,17 @@ expr_primary function call. If a system identifier, then a system function call. It can also be a call to a class method (function). */ - | hierarchy_identifier attribute_list_opt '(' expression_list_with_nuls ')' - { std::list*expr_list = $4; - strip_tail_items(expr_list); - PECallFunction*tmp = pform_make_call_function(@1, *$1, *expr_list); + | hierarchy_identifier attribute_list_opt argument_list_parens + { PECallFunction*tmp = pform_make_call_function(@1, *$1, *$3); delete $1; delete $2; - delete expr_list; + delete $3; $$ = tmp; } - | class_hierarchy_identifier '(' expression_list_with_nuls ')' - { list*expr_list = $3; - strip_tail_items(expr_list); - PECallFunction*tmp = pform_make_call_function(@1, *$1, *expr_list); + | class_hierarchy_identifier argument_list_parens + { PECallFunction*tmp = pform_make_call_function(@1, *$1, *$2); delete $1; - delete expr_list; + delete $2; $$ = tmp; } | SYSTEM_IDENTIFIER '(' expression_list_proper ')' @@ -3782,11 +3782,11 @@ expr_primary delete $3; $$ = tmp; } - | package_scope hierarchy_identifier { lex_in_package_scope(0); } '(' expression_list_with_nuls ')' - { PECallFunction*tmp = new PECallFunction($1, *$2, *$5); + | package_scope hierarchy_identifier { lex_in_package_scope(0); } argument_list_parens + { PECallFunction*tmp = new PECallFunction($1, *$2, *$4); FILE_NAME(tmp, @2); delete $2; - delete $5; + delete $4; $$ = tmp; } | SYSTEM_IDENTIFIER '(' ')' From fe5e60840f26520e726011c8f591f7ffa9d934f4 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sun, 8 Jan 2023 21:30:47 -0800 Subject: [PATCH 4/4] Add regression test for function calls with empty arguments Check that function calls with empty arguments are supported. Check the general case and special cases such as calling a function with empty arguments as part of a module port binding or force statements in automatic contexts. Also check that calling a function with too many trailing empty arguments as well as passing an empty argument for a port without a default value is an error. Signed-off-by: Lars-Peter Clausen --- ivtest/ivltests/func_empty_arg1.v | 33 +++++++++++++++++++++ ivtest/ivltests/func_empty_arg2.v | 29 ++++++++++++++++++ ivtest/ivltests/func_empty_arg3.v | 41 ++++++++++++++++++++++++++ ivtest/ivltests/func_empty_arg_fail1.v | 16 ++++++++++ ivtest/ivltests/func_empty_arg_fail2.v | 16 ++++++++++ ivtest/ivltests/func_empty_arg_fail3.v | 16 ++++++++++ ivtest/ivltests/func_empty_arg_fail4.v | 16 ++++++++++ ivtest/regress-sv.list | 7 +++++ ivtest/regress-vlog95.list | 1 + 9 files changed, 175 insertions(+) create mode 100644 ivtest/ivltests/func_empty_arg1.v create mode 100644 ivtest/ivltests/func_empty_arg2.v create mode 100644 ivtest/ivltests/func_empty_arg3.v create mode 100644 ivtest/ivltests/func_empty_arg_fail1.v create mode 100644 ivtest/ivltests/func_empty_arg_fail2.v create mode 100644 ivtest/ivltests/func_empty_arg_fail3.v create mode 100644 ivtest/ivltests/func_empty_arg_fail4.v diff --git a/ivtest/ivltests/func_empty_arg1.v b/ivtest/ivltests/func_empty_arg1.v new file mode 100644 index 000000000..107370094 --- /dev/null +++ b/ivtest/ivltests/func_empty_arg1.v @@ -0,0 +1,33 @@ +// Check that it is possible to call functins with empty arguments if they have +// default values. + +module test; + + bit failed = 1'b0; + + `define check(expr, val) \ + if (expr !== val) begin \ + $display("FAILED. %s, expected %d, got %d", `"expr`", val, expr); \ + failed = 1'b1; \ + end + + function integer f(integer a = 1, integer b = 2, integer c = 3); + return a * 100 + b * 10 + c; + endfunction + + initial begin + `check(f(4, 5, 6), 456); + `check(f(4, 5, ), 453); + `check(f(4, , 6), 426); + `check(f( , 5, 6), 156); + `check(f(4, , ), 423); + `check(f( , 5, ), 153); + `check(f( , , 6), 126); + `check(f( , , ), 123); + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/ivltests/func_empty_arg2.v b/ivtest/ivltests/func_empty_arg2.v new file mode 100644 index 000000000..e19bd0ed0 --- /dev/null +++ b/ivtest/ivltests/func_empty_arg2.v @@ -0,0 +1,29 @@ +// Check that it is possible to call functins with empty arguments if they have +// default values. Check that this works if the function call is in a module +// port binding expression. + +module M (input [31:0] x, input [31:0] y); + + initial begin + #1 + if (x === 623 && y == 624) begin + $display("PASSED"); + end else begin + $display("FAILED"); + end + end + +endmodule + +module test; + + function [31:0] f(reg [31:0] a, reg [31:0] b = 2, reg [31:0] c = 3); + return a * 100 + b * 10 + c; + endfunction + + M i_m ( + .x(f(6, )), + .y(f(6, , 4)) + ); + +endmodule diff --git a/ivtest/ivltests/func_empty_arg3.v b/ivtest/ivltests/func_empty_arg3.v new file mode 100644 index 000000000..8b949a894 --- /dev/null +++ b/ivtest/ivltests/func_empty_arg3.v @@ -0,0 +1,41 @@ +// Check that it is possible to call functins with empty arguments if they have +// default values. Check that this works if the function call is part of a force +// statement in an automatic context. + +module test; + + bit failed = 1'b0; + + `define check(expr, val) \ + if (expr !== val) begin \ + $display("FAILED. %s, expected %d, got %d", `"expr`", val, expr); \ + failed = 1'b1; \ + end + + integer x, y, z, w; + + function automatic integer f(integer a = 1, integer b = 2, integer c = 3); + return a * 100 + b * 10 + c; + endfunction + + task automatic t; + force x = f(4, , 6); + force y = f(4, 5, ); + force z = f(4, , ); + force w = f( , , 6); + endtask + + initial begin + t; + + `check(x, 426); + `check(y, 453); + `check(z, 423); + `check(w, 126); + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/ivltests/func_empty_arg_fail1.v b/ivtest/ivltests/func_empty_arg_fail1.v new file mode 100644 index 000000000..0804438dd --- /dev/null +++ b/ivtest/ivltests/func_empty_arg_fail1.v @@ -0,0 +1,16 @@ +// Check that passing too many empty arguments to a function results in an error. + +module test; + + function f(integer a = 1, integer b = 2, integer c = 3); + return a + 10 * b + 100 * c; + endfunction + + initial begin + integer x; + x = f( , , ,); // This should fail. 4 empty args, even though the function + // only takes 3 args + $display("FAILED"); + end + +endmodule diff --git a/ivtest/ivltests/func_empty_arg_fail2.v b/ivtest/ivltests/func_empty_arg_fail2.v new file mode 100644 index 000000000..d197a719b --- /dev/null +++ b/ivtest/ivltests/func_empty_arg_fail2.v @@ -0,0 +1,16 @@ +// Check that passing additional empty arguments to a function results in an error. + +module test; + + function f(integer a = 1, integer b = 2, integer c = 3); + return a + 10 * b + 100 * c; + endfunction + + initial begin + integer x; + x = f(4, 5, 6, ); // This should fail. 4 empty args, even though the function + // only takes 3 args + $display("FAILED"); + end + +endmodule diff --git a/ivtest/ivltests/func_empty_arg_fail3.v b/ivtest/ivltests/func_empty_arg_fail3.v new file mode 100644 index 000000000..72480a47d --- /dev/null +++ b/ivtest/ivltests/func_empty_arg_fail3.v @@ -0,0 +1,16 @@ +// Check that passing empty arguments to a function without any ports is an +// error. + +module test; + + function f(); + return 42; + endfunction + + initial begin + integer x; + x = f( , ); // This should fail. The function takes no arguments. + $display("FAILED"); + end + +endmodule diff --git a/ivtest/ivltests/func_empty_arg_fail4.v b/ivtest/ivltests/func_empty_arg_fail4.v new file mode 100644 index 000000000..a1ed27c97 --- /dev/null +++ b/ivtest/ivltests/func_empty_arg_fail4.v @@ -0,0 +1,16 @@ +// Check that an error is reported when passing an empty function argument for a +// port without a default value. + +module test; + + function f(integer a, integer b = 2); + return a + 10 * b + 100 * c; + endfunction + + initial begin + integer x; + x = f( , 3); // This should fail. No default value specified. + $display("FAILED"); + end + +endmodule diff --git a/ivtest/regress-sv.list b/ivtest/regress-sv.list index 156e76d49..f19cf254f 100644 --- a/ivtest/regress-sv.list +++ b/ivtest/regress-sv.list @@ -313,6 +313,13 @@ fork_join_any normal,-g2009 ivltests fork_join_dis normal,-g2009 ivltests fork_join_none normal,-g2009 ivltests fr49 normal,-g2009 ivltests +func_empty_arg1 normal,-g2005-sv ivltests +func_empty_arg2 normal,-g2005-sv ivltests +func_empty_arg3 normal,-g2005-sv ivltests +func_empty_arg_fail1 CE,-g2005-sv ivltests +func_empty_arg_fail2 CE,-g2005-sv ivltests +func_empty_arg_fail3 CE,-g2005-sv ivltests +func_empty_arg_fail4 CE,-g2005-sv ivltests func_init_var1 normal,-g2009 ivltests func_init_var2 normal,-g2009 ivltests func_init_var3 normal,-g2009 ivltests diff --git a/ivtest/regress-vlog95.list b/ivtest/regress-vlog95.list index 84e2bd204..76c297b2f 100644 --- a/ivtest/regress-vlog95.list +++ b/ivtest/regress-vlog95.list @@ -82,6 +82,7 @@ automatic_task3 CE ivltests br942 CE ivltests br_gh531 CE ivltests def_nettype CE ivltests +func_empty_arg3 CE,-g2005-sv ivltests func_init_var1 CE,-pallowsigned=1 ivltests func_init_var2 CE,-pallowsigned=1 ivltests func_init_var3 CE,-pallowsigned=1 ivltests