From 472598dd74ed24084853581a54ce6a3b226448dd Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Thu, 20 Jan 2022 15:59:28 +0100 Subject: [PATCH 1/4] Report errors for nets with invalid data type While a variable can have any data type the data type for nets is quite restricted. The SystemVerilog LRM section 6.7.1 ("Net declarations with built-in net types") requires that the data type of a wire is either a 4-state packed or a unpacked struct or unpacked array of 4-state packed types. As an extension to this iverilog allows real data type for wires as well as 2-state packed types. Add a check that reports an error if a net with any other type is declared. In addition in Verilog a net can not have an explicit data type at all. It can only have a packed dimension and a signed flag. As an extension to this Icarus also allows wires to be of `real` data type. Note that in Verilog mode the data type is checked in the parser since only the parser knows whether the data type is an implicit type (`input reg [7:0]` and `input [7:0] x` elaborate the same). But for SystemVerilog the type is checked during elaboration since due to forward typedefs and type parameters the type is not necessarily known in the parser. Signed-off-by: Lars-Peter Clausen --- elab_sig.cc | 134 +++++++++++++++++++++++------------- ivtest/gold/pr2976242c.gold | 2 +- parse.y | 3 + pform.cc | 33 +++++++++ pform.h | 3 + 5 files changed, 128 insertions(+), 47 deletions(-) diff --git a/elab_sig.cc b/elab_sig.cc index ceb4256c4..3378902eb 100644 --- a/elab_sig.cc +++ b/elab_sig.cc @@ -90,6 +90,90 @@ void Statement::elaborate_sig(Design*, NetScope*) const { } +static void sig_check_data_type(Design*des, NetScope*scope, + PWire *wire, NetNet *sig) +{ + ivl_type_t type = sig->net_type(); + + if (!type) + return; + + if (type->packed()) { + switch (type->base_type()) { + case IVL_VT_LOGIC: // 4-state packed is allowed by the standard + case IVL_VT_BOOL: // Icarus allows 2-state packed as an extension + return; + default: + break; + } + } + + // Icarus allows real nets as an extension + if (type->base_type() == IVL_VT_REAL) + return; + + if (wire->symbol_type() == PNamedItem::NET) { + cerr << wire->get_fileline() << ": error: Net `" + << wire->basename() << "` can not be of type `" + << sig->data_type() << "`." << endl; + des->errors++; + } else if (scope->type() == NetScope::MODULE && + sig->port_type() != NetNet::NOT_A_PORT) { + // Module ports only support wire types a the moment + cerr << wire->get_fileline() << ": sorry: Port `" + << wire->basename() << "` of module `" + << scope->module_name() + << "` with type `" << sig->data_type() + << "` is not supported." + << endl; + des->errors++; + } +} + +static void sig_check_port_type(Design*des, NetScope*scope, + PWire *wire, NetNet *sig) +{ + if (sig->port_type() == NetNet::PREF) { + cerr << wire->get_fileline() << ": sorry: " + << "Reference ports not supported yet." << endl; + des->errors += 1; + } + + // Some extra checks for module ports + if (scope->type() != NetScope::MODULE) + return; + + /* If the signal is an input and is also declared as a + reg, then report an error. */ + + if (sig->port_type() == NetNet::PINPUT && + sig->type() == NetNet::REG) { + cerr << wire->get_fileline() << ": error: Port `" + << wire->basename() << "` of module `" + << scope->module_name() + << "` is declared as input and as a reg type." << endl; + des->errors += 1; + } + + if (sig->port_type() == NetNet::PINOUT && + sig->type() == NetNet::REG) { + cerr << wire->get_fileline() << ": error: Port `" + << wire->basename() << "` of module `" + << scope->module_name() + << "` is declared as inout and as a reg type." << endl; + des->errors += 1; + } + + if (sig->port_type() == NetNet::PINOUT && + sig->data_type() == IVL_VT_REAL) { + cerr << wire->get_fileline() << ": error: Port `" + << wire->basename() << "` of module `" + << scope->module_name() + << "` is declared as a real inout port." << endl; + des->errors += 1; + } +} + bool PScope::elaborate_sig_wires_(Design*des, NetScope*scope) const { bool flag = true; @@ -100,53 +184,11 @@ bool PScope::elaborate_sig_wires_(Design*des, NetScope*scope) const PWire*cur = (*wt).second; NetNet*sig = cur->elaborate_sig(des, scope); - if (sig && (sig->scope() == scope) - && (sig->port_type() == NetNet::PREF)) { + if (!sig || sig->scope() != scope) + continue; - cerr << cur->get_fileline() << ": sorry: " - << "Reference ports not supported yet." << endl; - des->errors += 1; - } - - - /* If the signal is an input and is also declared as a - reg, then report an error. */ - - if (sig && (sig->scope() == scope) - && (scope->type() == NetScope::MODULE) - && (sig->port_type() == NetNet::PINPUT) - && (sig->type() == NetNet::REG)) { - - cerr << cur->get_fileline() << ": error: Port " - << cur->basename() << " of module " - << scope->module_name() - << " is declared as input and as a reg type." << endl; - des->errors += 1; - } - - if (sig && (sig->scope() == scope) - && (scope->type() == NetScope::MODULE) - && (sig->port_type() == NetNet::PINOUT) - && (sig->type() == NetNet::REG)) { - - cerr << cur->get_fileline() << ": error: Port " - << cur->basename() << " of module " - << scope->module_name() - << " is declared as inout and as a reg type." << endl; - des->errors += 1; - } - - if (sig && (sig->scope() == scope) - && (scope->type() == NetScope::MODULE) - && (sig->port_type() == NetNet::PINOUT) - && (sig->data_type() == IVL_VT_REAL)) { - - cerr << cur->get_fileline() << ": error: Port " - << cur->basename() << " of module " - << scope->module_name() - << " is declared as a real inout port." << endl; - des->errors += 1; - } + sig_check_data_type(des, scope, cur, sig); + sig_check_port_type(des, scope, cur, sig); } diff --git a/ivtest/gold/pr2976242c.gold b/ivtest/gold/pr2976242c.gold index 45baf1d45..ade3d00bb 100644 --- a/ivtest/gold/pr2976242c.gold +++ b/ivtest/gold/pr2976242c.gold @@ -1,4 +1,4 @@ -./ivltests/pr2976242c.v:43: error: Port out of module io_real_to_vec is declared as a real inout port. +./ivltests/pr2976242c.v:43: error: Port `out` of module `io_real_to_vec` is declared as a real inout port. ./ivltests/pr2976242c.v:11: error: Cannot connect an arrayed instance of module vec_to_real to real signal r_vec. ./ivltests/pr2976242c.v:14: error: When automatically converting a real port of an arrayed instance to a bit signal ./ivltests/pr2976242c.v:14: : the signal width (5) must be an integer multiple of the instance count (2). diff --git a/parse.y b/parse.y index 32cbe6823..206caade4 100644 --- a/parse.y +++ b/parse.y @@ -4891,6 +4891,7 @@ module_item | attribute_list_opt net_type data_type_or_implicit delay3_opt net_variable_list ';' { data_type_t*data_type = $3; + pform_check_net_data_type(@2, $2, $3); if (data_type == 0) { data_type = new vector_type_t(IVL_VT_LOGIC, false, 0); FILE_NAME(data_type, @2); @@ -4925,6 +4926,7 @@ module_item | attribute_list_opt net_type data_type_or_implicit delay3_opt net_decl_assigns ';' { data_type_t*data_type = $3; + pform_check_net_data_type(@2, $2, $3); if (data_type == 0) { data_type = new vector_type_t(IVL_VT_LOGIC, false, 0); FILE_NAME(data_type, @2); @@ -4938,6 +4940,7 @@ module_item | attribute_list_opt net_type data_type_or_implicit drive_strength net_decl_assigns ';' { data_type_t*data_type = $3; + pform_check_net_data_type(@2, $2, $3); if (data_type == 0) { data_type = new vector_type_t(IVL_VT_LOGIC, false, 0); FILE_NAME(data_type, @2); diff --git a/pform.cc b/pform.cc index 27416c9c3..ca45e53eb 100644 --- a/pform.cc +++ b/pform.cc @@ -2634,6 +2634,8 @@ void pform_module_define_port(const struct vlltype&li, return; } + pform_check_net_data_type(li, type, vtype); + // Packed ranges list*prange = 0; // Unpacked dimensions @@ -3748,6 +3750,37 @@ bool pform_requires_sv(const struct vlltype&loc, const char *feature) return false; } +void pform_check_net_data_type(const struct vlltype&loc, NetNet::Type net_type, + const data_type_t *data_type) +{ + // For SystemVerilog the type is checked during elaboration since due to + // forward typedefs and type parameters the actual type might not be known + // yet. + if (gn_system_verilog()) + return; + + switch (net_type) { + case NetNet::REG: + case NetNet::IMPLICIT_REG: + return; + default: + break; + } + + if (!data_type) + return; + + const vector_type_t*vec_type = dynamic_cast(data_type); + if (vec_type && vec_type->implicit_flag) + return; + + const real_type_t*rtype = dynamic_cast(data_type); + if (rtype && rtype->type_code() == real_type_t::REAL) + return; + + pform_requires_sv(loc, "Net data type"); +} + FILE*vl_input = 0; extern void reset_lexor(); diff --git a/pform.h b/pform.h index 4e30ab750..5468d44b7 100644 --- a/pform.h +++ b/pform.h @@ -587,4 +587,7 @@ bool pform_requires_sv(const struct vlltype&loc, const char *feature); void pform_start_parameter_port_list(); void pform_end_parameter_port_list(); +void pform_check_net_data_type(const struct vlltype&loc, NetNet::Type net_type, + const data_type_t *data_type); + #endif /* IVL_pform_H */ From f67cdddecf91b19b734f8c365876100a94dfa48b Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Wed, 2 Mar 2022 09:44:44 +0100 Subject: [PATCH 2/4] Don't make input `integer` ports variables In Verilog module input ports can only have a packed dimensions and a signed flag, but no explicit data type. In SystemVerilog an explicit data type can be specified for module input ports. Such a port is a net, regardless of the data type, unless explicitly made a variable using the `var` keyword. This works for the most part in the current implementation, but for some data types such as `reg` and `integer` the input port is turned into a variable. And since input port's can't be variables in the current implementation this results in an error. Fix this by completely removing the `reg_flag` that is used to indicate that a certain data type is always a variable. There is no such restriction on data types for SystemVerilog and for Verilog there are already checks in place that a input port can only have an implicit (or real) data type. Signed-off-by: Lars-Peter Clausen --- parse.y | 47 +++++++---------------------------------------- pform.cc | 3 --- pform_types.h | 10 ++-------- 3 files changed, 9 insertions(+), 51 deletions(-) diff --git a/parse.y b/parse.y index 206caade4..50d5e62b0 100644 --- a/parse.y +++ b/parse.y @@ -1221,14 +1221,7 @@ packed_array_data_type /* IEEE1800-2005: A.2.2.1 */ data_type /* IEEE1800-2005: A.2.2.1 */ : integer_vector_type unsigned_signed_opt dimensions_opt - { ivl_variable_type_t use_vtype = $1; - bool reg_flag = false; - if (use_vtype == IVL_VT_NO_TYPE) { - use_vtype = IVL_VT_LOGIC; - reg_flag = true; - } - vector_type_t*tmp = new vector_type_t(use_vtype, $2, $3); - tmp->reg_flag = reg_flag; + { vector_type_t*tmp = new vector_type_t($1, $2, $3); FILE_NAME(tmp, @1); $$ = tmp; } @@ -1245,14 +1238,12 @@ data_type /* IEEE1800-2005: A.2.2.1 */ | K_integer signed_unsigned_opt { std::list*pd = make_range_from_width(integer_width); vector_type_t*tmp = new vector_type_t(IVL_VT_LOGIC, $2, pd); - tmp->reg_flag = true; tmp->integer_flag = true; $$ = tmp; } | K_time unsigned_signed_opt { std::list*pd = make_range_from_width(64); vector_type_t*tmp = new vector_type_t(IVL_VT_LOGIC, $2, pd); - tmp->reg_flag = !gn_system_verilog(); $$ = tmp; } | packed_array_data_type dimensions_opt @@ -1595,7 +1586,7 @@ inside_expression /* IEEE1800-2005 A.8.3 */ ; integer_vector_type /* IEEE1800-2005: A.2.2.1 */ - : K_reg { $$ = IVL_VT_NO_TYPE; } /* Usually a synonym for logic. */ + : K_reg { $$ = IVL_VT_LOGIC; } /* A synonym for logic. */ | K_bit { $$ = IVL_VT_BOOL; } | K_logic { $$ = IVL_VT_LOGIC; } | K_bool { $$ = IVL_VT_BOOL; } /* Icarus Verilog xtypes extension */ @@ -2179,14 +2170,7 @@ simple_immediate_assertion_statement /* IEEE1800-2012 A.6.10 */ simple_type_or_string /* IEEE1800-2005: A.2.2.1 */ : integer_vector_type - { ivl_variable_type_t use_vtype = $1; - bool reg_flag = false; - if (use_vtype == IVL_VT_NO_TYPE) { - use_vtype = IVL_VT_LOGIC; - reg_flag = true; - } - vector_type_t*tmp = new vector_type_t(use_vtype, false, 0); - tmp->reg_flag = reg_flag; + { vector_type_t*tmp = new vector_type_t($1, false, 0); FILE_NAME(tmp, @1); $$ = tmp; } @@ -2203,14 +2187,12 @@ simple_type_or_string /* IEEE1800-2005: A.2.2.1 */ | K_integer { std::list*pd = make_range_from_width(integer_width); vector_type_t*tmp = new vector_type_t(IVL_VT_LOGIC, true, pd); - tmp->reg_flag = true; tmp->integer_flag = true; $$ = tmp; } | K_time { std::list*pd = make_range_from_width(64); vector_type_t*tmp = new vector_type_t(IVL_VT_LOGIC, false, pd); - tmp->reg_flag = !gn_system_verilog(); $$ = tmp; } | K_string @@ -2776,12 +2758,8 @@ enum_base_type /* IEEE 1800-2012 A.2.2.1 */ $$ = enum_type; } | integer_vector_type unsigned_signed_opt dimensions_opt - { ivl_variable_type_t use_vtype = $1; - if (use_vtype == IVL_VT_NO_TYPE) - use_vtype = IVL_VT_LOGIC; - - enum_type_t*enum_type = new enum_type_t; - enum_type->base_type = use_vtype; + { enum_type_t*enum_type = new enum_type_t; + enum_type->base_type = $1; enum_type->signed_flag = $2; enum_type->integer_flag = false; enum_type->range.reset($3 ? $3 : make_range_from_width(1)); @@ -4552,9 +4530,7 @@ port_declaration NetNet::Type use_type = $3; if (use_type == NetNet::IMPLICIT) { if (vector_type_t*dtype = dynamic_cast ($4)) { - if (dtype->reg_flag) - use_type = NetNet::REG; - else if (dtype->implicit_flag) + if (dtype->implicit_flag) use_type = NetNet::IMPLICIT; else use_type = NetNet::IMPLICIT_REG; @@ -4596,14 +4572,7 @@ port_declaration perm_string name = lex_strings.make($5); NetNet::Type use_type = $3; if (use_type == NetNet::IMPLICIT) { - if (vector_type_t*dtype = dynamic_cast ($4)) { - if (dtype->reg_flag) - use_type = NetNet::REG; - else - use_type = NetNet::IMPLICIT_REG; - } else { - use_type = NetNet::IMPLICIT_REG; - } + use_type = NetNet::IMPLICIT_REG; } ptmp = pform_module_port_reference(name, @2.text, @2.first_line); pform_module_define_port(@2, name, NetNet::POUTPUT, use_type, $4, $1); @@ -5009,8 +4978,6 @@ module_item if (vector_type_t*dtype = dynamic_cast ($3)) { if (dtype->implicit_flag) use_type = NetNet::NONE; - else if (dtype->reg_flag) - use_type = NetNet::REG; else use_type = NetNet::IMPLICIT_REG; diff --git a/pform.cc b/pform.cc index ca45e53eb..08dbb8a70 100644 --- a/pform.cc +++ b/pform.cc @@ -2652,9 +2652,6 @@ void pform_module_define_port(const struct vlltype&li, data_type = vec_type->base_type; signed_flag = vec_type->signed_flag; prange = vec_type->pdims.get(); - if (vec_type->reg_flag) - type = NetNet::REG; - } else if (atom2_type_t*atype = dynamic_cast(vtype)) { data_type = IVL_VT_BOOL; signed_flag = atype->signed_flag; diff --git a/pform_types.h b/pform_types.h index a7f58152c..990407f88 100644 --- a/pform_types.h +++ b/pform_types.h @@ -233,12 +233,7 @@ extern atom2_type_t size_type; * bit unsigned foo * reg foo * - * There are a few special cases: - * - * For the most part, Verilog treats "logic" and "reg" as synonyms, - * but there are a few cases where the parser needs to know the - * difference. So "reg_flag" is set to true if the IVL_VT_LOGIC type - * is due to the "reg" keyword. + * There is one special case: * * If there are no reg/logic/bit/bool keywords, then Verilog will * assume the type is logic, but the context may need to know about @@ -247,7 +242,7 @@ extern atom2_type_t size_type; struct vector_type_t : public data_type_t { inline explicit vector_type_t(ivl_variable_type_t bt, bool sf, std::list*pd) - : base_type(bt), signed_flag(sf), reg_flag(false), integer_flag(false), implicit_flag(false), pdims(pd) { } + : base_type(bt), signed_flag(sf), integer_flag(false), implicit_flag(false), pdims(pd) { } virtual ivl_variable_type_t figure_packed_base_type(void)const; virtual void pform_dump(std::ostream&out, unsigned indent) const; virtual std::ostream& debug_dump(std::ostream&out) const; @@ -255,7 +250,6 @@ struct vector_type_t : public data_type_t { ivl_variable_type_t base_type; bool signed_flag; - bool reg_flag; // True if "reg" was used bool integer_flag; // True if "integer" was used bool implicit_flag; // True if this type is implicitly logic/reg std::unique_ptr< std::list > pdims; From 2921e66105f074630116c9de729fe364957f8a71 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Mon, 28 Feb 2022 10:25:16 +0100 Subject: [PATCH 3/4] Add regression test for invalid net data types Add regression tests that check that declaring a net of type class, dynamic array, queue or string result in an error. Signed-off-by: Lars-Peter Clausen --- ivtest/ivltests/net_class_fail.v | 12 ++++++++++++ ivtest/ivltests/net_darray_fail.v | 10 ++++++++++ ivtest/ivltests/net_queue_fail.v | 10 ++++++++++ ivtest/ivltests/net_string_fail.v | 10 ++++++++++ ivtest/regress-sv.list | 4 ++++ 5 files changed, 46 insertions(+) create mode 100644 ivtest/ivltests/net_class_fail.v create mode 100644 ivtest/ivltests/net_darray_fail.v create mode 100644 ivtest/ivltests/net_queue_fail.v create mode 100644 ivtest/ivltests/net_string_fail.v diff --git a/ivtest/ivltests/net_class_fail.v b/ivtest/ivltests/net_class_fail.v new file mode 100644 index 000000000..e38e670c3 --- /dev/null +++ b/ivtest/ivltests/net_class_fail.v @@ -0,0 +1,12 @@ +// Check that declaring a net of a class type results in an error + +module test; + class C; + endclass + + wire C x; + + initial begin + $display("FAILED"); + end +endmodule diff --git a/ivtest/ivltests/net_darray_fail.v b/ivtest/ivltests/net_darray_fail.v new file mode 100644 index 000000000..be7fe73a1 --- /dev/null +++ b/ivtest/ivltests/net_darray_fail.v @@ -0,0 +1,10 @@ +// Check that declaring a net of a dynamic array type results in an error + +module test; + + wire x[]; + + initial begin + $display("FAILED"); + end +endmodule diff --git a/ivtest/ivltests/net_queue_fail.v b/ivtest/ivltests/net_queue_fail.v new file mode 100644 index 000000000..e0f11c0be --- /dev/null +++ b/ivtest/ivltests/net_queue_fail.v @@ -0,0 +1,10 @@ +// Check that declaring a net of a queue type results in an error + +module test; + + wire x[$]; + + initial begin + $display("FAILED"); + end +endmodule diff --git a/ivtest/ivltests/net_string_fail.v b/ivtest/ivltests/net_string_fail.v new file mode 100644 index 000000000..a6c3af117 --- /dev/null +++ b/ivtest/ivltests/net_string_fail.v @@ -0,0 +1,10 @@ +// Check that declaring a net of string type results in an error + +module test; + + wire string x; + + initial begin + $display("FAILED"); + end +endmodule diff --git a/ivtest/regress-sv.list b/ivtest/regress-sv.list index 4f73cad6a..d6f190e08 100644 --- a/ivtest/regress-sv.list +++ b/ivtest/regress-sv.list @@ -318,6 +318,10 @@ named_begin normal,-g2009 ivltests named_begin_fail CE,-g2009 ivltests named_fork normal,-g2009 ivltests named_fork_fail CE,-g2009 ivltests +net_class_fail CE,-g2005-sv ivltests +net_darray_fail CE,-g2005-sv ivltests +net_queue_fail CE,-g2005-sv ivltests +net_string_fail CE,-g2005-sv ivltests packeda normal,-g2009 ivltests packeda2 normal,-g2009 ivltests parameter_in_generate2 CE,-g2005-sv ivltests From 8a2d4e4fa40cd4f434ca41a8ce28966ce5440eae Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Wed, 2 Mar 2022 09:36:07 +0100 Subject: [PATCH 4/4] Add regression test for Verilog data types on module input ports Using Verilog data types on module input and inout ports is an error in Verilog. But in SystemVerilog it is allowed and the port should be a net with the specified data type. Check that this is supported. Signed-off-by: Lars-Peter Clausen --- ivtest/ivltests/module_inout_port_type.v | 14 ++++++++++++++ ivtest/ivltests/module_input_port_type.v | 14 ++++++++++++++ ivtest/regress-fsv.list | 2 ++ ivtest/regress-vlg.list | 2 ++ 4 files changed, 32 insertions(+) create mode 100644 ivtest/ivltests/module_inout_port_type.v create mode 100644 ivtest/ivltests/module_input_port_type.v diff --git a/ivtest/ivltests/module_inout_port_type.v b/ivtest/ivltests/module_inout_port_type.v new file mode 100644 index 000000000..3c9d2f277 --- /dev/null +++ b/ivtest/ivltests/module_inout_port_type.v @@ -0,0 +1,14 @@ +// Check Verilog types on a module inout port. In Verilog this is an error, but +// in SystemVerilog it is supported + +module test ( + inout reg a, + inout time b, + inout integer c +); + + initial begin + $display("PASSED"); + end + +endmodule diff --git a/ivtest/ivltests/module_input_port_type.v b/ivtest/ivltests/module_input_port_type.v new file mode 100644 index 000000000..54dd15c99 --- /dev/null +++ b/ivtest/ivltests/module_input_port_type.v @@ -0,0 +1,14 @@ +// Check Verilog types on a module input port. In Verilog this is an error, but +// in SystemVerilog it is supported + +module test ( + input reg a, + input time b, + input integer c +); + + initial begin + $display("PASSED"); + end + +endmodule diff --git a/ivtest/regress-fsv.list b/ivtest/regress-fsv.list index 2b847a618..d990342fc 100644 --- a/ivtest/regress-fsv.list +++ b/ivtest/regress-fsv.list @@ -77,6 +77,8 @@ br_gh25b normal ivltests br_gh567 normal ivltests check_constant_3 normal ivltests function4 normal ivltests +module_inout_port_type normal ivltests +module_input_port_type normal ivltests parameter_in_generate1 normal ivltests parameter_no_default normal ivltests parameter_omit1 normal ivltests diff --git a/ivtest/regress-vlg.list b/ivtest/regress-vlg.list index ff022cf10..09d213229 100644 --- a/ivtest/regress-vlg.list +++ b/ivtest/regress-vlg.list @@ -644,6 +644,8 @@ mixed_width_case normal ivltests modparam normal ivltests top # Override parameter via passed down value module3.12A normal ivltests main module3.12B normal ivltests +module_inout_port_type CE ivltests +module_input_port_type CE ivltests module_output_port_var1 normal ivltests module_output_port_var2 normal ivltests modulus normal ivltests # wire % and reg % operators