From 0ab1ed916fd3c6a3b55763fcaabcf00f18ceadf4 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Tue, 1 Feb 2022 10:18:26 +0100 Subject: [PATCH 1/2] Report error when trying to override non-existing parameter Overriding a parameter that does not exist will only generate a warning at the moment. This can hide programming mistakes such as an typo in a parameter override. There is nothing in the LRMs to support that this should only be warning, so elevate this to an error. This is consistent with how an error is generated when trying to reference a non-existing port or variable. The generated error message differentiates between whether the parameter does not exist at all, or whether it is a localparam. There are two regression tests that rely on that only a warning is generated, these have been updated to expect an error. Signed-off-by: Lars-Peter Clausen --- elab_scope.cc | 12 +++-------- ivtest/gold/br_gh157.gold | 4 ++-- ivtest/gold/pr498b.gold | 2 ++ ivtest/ivltests/{pr498.v => pr498a.v} | 1 - ivtest/ivltests/pr498b.v | 30 +++++++++++++++++++++++++++ ivtest/regress-ivl1.list | 2 +- ivtest/regress-vlg.list | 3 ++- net_design.cc | 15 ++------------ net_scope.cc | 22 ++++++++++++++------ netlist.h | 2 +- 10 files changed, 59 insertions(+), 34 deletions(-) create mode 100644 ivtest/gold/pr498b.gold rename ivtest/ivltests/{pr498.v => pr498a.v} (95%) create mode 100644 ivtest/ivltests/pr498b.v diff --git a/elab_scope.cc b/elab_scope.cc index c7d205d1f..c1b8546dd 100644 --- a/elab_scope.cc +++ b/elab_scope.cc @@ -623,7 +623,7 @@ static void elaborate_scope_classes(Design*des, NetScope*scope, } } -static void replace_scope_parameters(NetScope*scope, const LineInfo&loc, +static void replace_scope_parameters(Design *des, NetScope*scope, const LineInfo&loc, const Module::replace_t&replacements) { if (debug_scopes) { @@ -649,13 +649,7 @@ static void replace_scope_parameters(NetScope*scope, const LineInfo&loc, cerr << loc.get_fileline() << ": : " << "Type=" << val->expr_type() << endl; } - bool flag = scope->replace_parameter((*cur).first, val, - scope->parent()); - if (! flag) { - cerr << val->get_fileline() << ": warning: parameter " - << (*cur).first << " not found in " - << scope_path(scope) << "." << endl; - } + scope->replace_parameter(des, (*cur).first, val, scope->parent()); } } @@ -818,7 +812,7 @@ bool Module::elaborate_scope(Design*des, NetScope*scope, // Run parameter replacements that were collected from the // containing scope and meant for me. - replace_scope_parameters(scope, *this, replacements); + replace_scope_parameters(des, scope, *this, replacements); elaborate_scope_enumerations(des, scope, enum_sets); diff --git a/ivtest/gold/br_gh157.gold b/ivtest/gold/br_gh157.gold index 46ba98b2f..14dc1f437 100644 --- a/ivtest/gold/br_gh157.gold +++ b/ivtest/gold/br_gh157.gold @@ -1,2 +1,2 @@ -./ivltests/br_gh157.v:13: warning: parameter x not found in test.dut. -2 +./ivltests/br_gh157.v:13: error: Cannot override localparam `x` in `test.dut`. +1 error(s) during elaboration. diff --git a/ivtest/gold/pr498b.gold b/ivtest/gold/pr498b.gold new file mode 100644 index 000000000..815b71411 --- /dev/null +++ b/ivtest/gold/pr498b.gold @@ -0,0 +1,2 @@ +./ivltests/pr498b.v:23: error: parameter `foo` not found in `main`. +1 error(s) during elaboration. diff --git a/ivtest/ivltests/pr498.v b/ivtest/ivltests/pr498a.v similarity index 95% rename from ivtest/ivltests/pr498.v rename to ivtest/ivltests/pr498a.v index 6a943a84b..44951133d 100644 --- a/ivtest/ivltests/pr498.v +++ b/ivtest/ivltests/pr498a.v @@ -20,7 +20,6 @@ module main; test tt(); - defparam foo = 3; /* This should generate a warning. */ defparam tt.foo = 4; endmodule // main diff --git a/ivtest/ivltests/pr498b.v b/ivtest/ivltests/pr498b.v new file mode 100644 index 000000000..449dba64e --- /dev/null +++ b/ivtest/ivltests/pr498b.v @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2002 Stephen Williams (steve@icarus.com) + * + * This source code is free software; you can redistribute it + * and/or modify it in source code form under the terms of the GNU + * General Public License as published by the Free Software + * Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * 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., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA + */ + + +module main; + test tt(); + defparam foo = 3; /* This should generate an error. */ +endmodule // main + +module test; + parameter foo = 10; + reg [foo-1:0] bar; + +endmodule // test diff --git a/ivtest/regress-ivl1.list b/ivtest/regress-ivl1.list index 97ead6058..bf2394966 100644 --- a/ivtest/regress-ivl1.list +++ b/ivtest/regress-ivl1.list @@ -121,7 +121,7 @@ clog2 normal ivltests clog2-signal normal ivltests param_vec normal ivltests param_vec2 normal ivltests -pr498 normal ivltests +pr498a normal ivltests pr721 normal ivltests pr809 normal ivltests pr809b normal ivltests diff --git a/ivtest/regress-vlg.list b/ivtest/regress-vlg.list index 1aafa42eb..89075234c 100644 --- a/ivtest/regress-vlg.list +++ b/ivtest/regress-vlg.list @@ -308,7 +308,7 @@ br_gh127e normal ivltests gold=br_gh127e.gold br_gh127f normal ivltests gold=br_gh127f.gold br_gh142 CE ivltests br_gh152 CE ivltests -br_gh157 normal ivltests gold=br_gh157.gold +br_gh157 CE ivltests gold=br_gh157.gold br_gh162 normal ivltests br_gh163 CE ivltests br_gh198 normal ivltests gold=br_gh198.gold @@ -736,6 +736,7 @@ pr445 normal ivltests pr478 normal ivltests pr487 normal ivltests gold=pr487.gold pr492 normal ivltests gold=pr492.gold +pr498b CE ivltests gold=pr498b.gold pr508 normal ivltests pr509 normal ivltests pr509b normal ivltests diff --git a/net_design.cc b/net_design.cc index f5d2eddb6..12c8b14c6 100644 --- a/net_design.cc +++ b/net_design.cc @@ -433,13 +433,7 @@ void NetScope::run_defparams(Design*des) continue; } - bool flag = targ_scope->replace_parameter(perm_name, val, this); - if (! flag) { - cerr << val->get_fileline() << ": warning: parameter " - << perm_name << " not found in " - << scope_path(targ_scope) << "." << endl; - } - + targ_scope->replace_parameter(des, perm_name, val, this); } // If some of the defparams didn't find a scope in the name, @@ -474,12 +468,7 @@ void NetScope::run_defparams_later(Design*des) continue; } - bool flag = targ_scope->replace_parameter(name, val, this); - if (! flag) { - cerr << val->get_fileline() << ": warning: parameter " - << name << " not found in " - << scope_path(targ_scope) << "." << endl; - } + targ_scope->replace_parameter(des, name, val, this); // We'll need to re-evaluate parameters in this scope target_scopes.insert(targ_scope); diff --git a/net_scope.cc b/net_scope.cc index 1a4216069..5abdf0b77 100644 --- a/net_scope.cc +++ b/net_scope.cc @@ -25,6 +25,7 @@ # include "netclass.h" # include "netenum.h" # include "netvector.h" +# include "PExpr.h" # include "PPackage.h" # include # include @@ -381,18 +382,27 @@ bool NetScope::auto_name(const char*prefix, char pad, const char* suffix) * Return false if the parameter does not already exist. * A parameter is not automatically created. */ -bool NetScope::replace_parameter(perm_string key, PExpr*val, NetScope*scope) +void NetScope::replace_parameter(Design *des, perm_string key, PExpr*val, NetScope*scope) { - if (parameters.find(key) == parameters.end()) - return false; + if (parameters.find(key) == parameters.end()) { + cerr << val->get_fileline() << ": error: parameter `" + << key << "` not found in `" + << scope_path(this) << "`." << endl; + des->errors++; + return; + } param_expr_t&ref = parameters[key]; - if (ref.local_flag) - return false; + if (ref.local_flag) { + cerr << val->get_fileline() << ": error: " + << "Cannot override localparam `" << key << "` in `" + << scope_path(this) << "`." << endl; + des->errors++; + return; + } ref.val_expr = val; ref.val_scope = scope; - return true; } bool NetScope::make_parameter_unannotatable(perm_string key) diff --git a/netlist.h b/netlist.h index 85dc8ce8b..a5babeb4e 100644 --- a/netlist.h +++ b/netlist.h @@ -983,7 +983,7 @@ class NetScope : public Definitions, public Attrib { expression with a new expression, without affecting the range or signed_flag. Return false if the name does not exist. */ - bool replace_parameter(perm_string name, PExpr*val, NetScope*scope); + void replace_parameter(Design *des, perm_string name, PExpr*val, NetScope*scope); /* This is used to ensure the value of a parameter cannot be changed at run-time. This is required if a specparam is used From 880f4927bf80d06a6a36c8a43966433a4122cafb Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sun, 23 Jan 2022 21:11:19 +0100 Subject: [PATCH 2/2] Add regression test for invalid parameter overrides Check that invalid parameter overrides generate an error. There are some cases that are not handled correctly today. The test will be updated once they are addressed. Signed-off-by: Lars-Peter Clausen --- ivtest/gold/parameter_invalid_override.gold | 21 +++++++ ivtest/ivltests/parameter_invalid_override.v | 63 ++++++++++++++++++++ ivtest/regress-sv.list | 1 + 3 files changed, 85 insertions(+) create mode 100644 ivtest/gold/parameter_invalid_override.gold create mode 100644 ivtest/ivltests/parameter_invalid_override.v diff --git a/ivtest/gold/parameter_invalid_override.gold b/ivtest/gold/parameter_invalid_override.gold new file mode 100644 index 000000000..029fc3262 --- /dev/null +++ b/ivtest/gold/parameter_invalid_override.gold @@ -0,0 +1,21 @@ +./ivltests/parameter_invalid_override.v:28: error: Cannot override localparam `C` in `test.i_a`. +./ivltests/parameter_invalid_override.v:29: error: Cannot override localparam `D` in `test.i_a`. +./ivltests/parameter_invalid_override.v:31: error: Cannot override localparam `F` in `test.i_a`. +./ivltests/parameter_invalid_override.v:32: error: Cannot override localparam `G` in `test.i_a`. +./ivltests/parameter_invalid_override.v:33: error: Cannot override localparam `H` in `test.i_a`. +./ivltests/parameter_invalid_override.v:34: error: Cannot override localparam `I` in `test.i_a`. +./ivltests/parameter_invalid_override.v:35: error: parameter `Z` not found in `test.i_a`. +./ivltests/parameter_invalid_override.v:52: error: Cannot override localparam `C` in `test.i_b`. +./ivltests/parameter_invalid_override.v:53: error: Cannot override localparam `D` in `test.i_b`. +./ivltests/parameter_invalid_override.v:54: error: parameter `Z` not found in `test.i_b`. +./ivltests/parameter_invalid_override.v:40: error: Cannot override localparam `C` in `test.i_a`. +./ivltests/parameter_invalid_override.v:41: error: Cannot override localparam `D` in `test.i_a`. +./ivltests/parameter_invalid_override.v:43: error: Cannot override localparam `F` in `test.i_a`. +./ivltests/parameter_invalid_override.v:44: error: Cannot override localparam `G` in `test.i_a`. +./ivltests/parameter_invalid_override.v:45: error: Cannot override localparam `H` in `test.i_a`. +./ivltests/parameter_invalid_override.v:46: error: Cannot override localparam `I` in `test.i_a`. +./ivltests/parameter_invalid_override.v:47: error: parameter `Z` not found in `test.i_a`. +./ivltests/parameter_invalid_override.v:59: error: Cannot override localparam `C` in `test.i_b`. +./ivltests/parameter_invalid_override.v:60: error: Cannot override localparam `D` in `test.i_b`. +./ivltests/parameter_invalid_override.v:61: error: parameter `Z` not found in `test.i_b`. +21 error(s) during elaboration. diff --git a/ivtest/ivltests/parameter_invalid_override.v b/ivtest/ivltests/parameter_invalid_override.v new file mode 100644 index 000000000..32843690c --- /dev/null +++ b/ivtest/ivltests/parameter_invalid_override.v @@ -0,0 +1,63 @@ +// Check that invalid parameter overrides generate an error + +module a #( + parameter A = 1, B = 2, + localparam C = 3, localparam D = 4, // TODO: D should be localparam even when the keyword omitted + parameter E = 5 +); + + // TODO: parameter here should be treated as a localparam since the module has a + // parameter port list + /*parameter*/ localparam F = 6, G = 7; + localparam H = 8, I = 9; + +endmodule + +module b; + + parameter A = 1, B = 2; + localparam C = 3, D = 4; + +endmodule + +module test; + + a #( + .A(10), + .B(20), + .C(30), + .D(40), + .E(50), + .F(60), + .G(70), + .H(80), + .I(90), + .Z(99) + ) i_a(); + + defparam i_a.A = 100; + defparam i_a.B = 200; + defparam i_a.C = 300; + defparam i_a.D = 400; + defparam i_a.E = 500; + defparam i_a.F = 600; + defparam i_a.G = 700; + defparam i_a.H = 800; + defparam i_a.I = 900; + defparam i_a.Z = 999; + + b #( + .A(10), + .B(20), + .C(30), + .D(40), + .Z(99) + ) i_b(); + + defparam i_b.A = 100; + defparam i_b.B = 200; + defparam i_b.C = 300; + defparam i_b.D = 400; + defparam i_b.Z = 999; + +endmodule diff --git a/ivtest/regress-sv.list b/ivtest/regress-sv.list index a0bfc15e6..f11026406 100644 --- a/ivtest/regress-sv.list +++ b/ivtest/regress-sv.list @@ -311,6 +311,7 @@ named_fork normal,-g2009 ivltests named_fork_fail CE,-g2009 ivltests packeda normal,-g2009 ivltests packeda2 normal,-g2009 ivltests +parameter_invalid_override CE,-g2005-sv ivltests gold=parameter_invalid_override.gold parameter_type2 normal,-g2009 ivltests parpkg_test normal,-g2009 ivltests parpkg_test2 normal,-g2009 ivltests