From 9c113b4bb8f7ee4712c206c94a2995ed900513c4 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Tue, 17 Sep 2024 23:04:47 +1000 Subject: [PATCH] Ignoring modport simple and clock ports as these are not declaring the symbol, rather referencing from the interface signal declaration. --- .../analysis/checkers/instance_shadow_rule.cc | 31 ++++++++++++------- .../analysis/checkers/instance_shadow_rule.h | 6 ++-- .../checkers/instance_shadow_rule_test.cc | 15 ++++++++- 3 files changed, 36 insertions(+), 16 deletions(-) diff --git a/verilog/analysis/checkers/instance_shadow_rule.cc b/verilog/analysis/checkers/instance_shadow_rule.cc index 064302fef..57d7f7db6 100644 --- a/verilog/analysis/checkers/instance_shadow_rule.cc +++ b/verilog/analysis/checkers/instance_shadow_rule.cc @@ -51,7 +51,7 @@ const char InstanceShadowRule::kTopic[] = "mark-shadowed-instances"; const char InstanceShadowRule::kMessage[] = "Instance shadows the already declared variable"; -const LintRuleDescriptor& InstanceShadowRule::GetDescriptor() { +const LintRuleDescriptor &InstanceShadowRule::GetDescriptor() { static const LintRuleDescriptor d{ .name = "instance-shadowing", .topic = "mark-shadowed-instances", @@ -61,18 +61,19 @@ const LintRuleDescriptor& InstanceShadowRule::GetDescriptor() { return d; } -static const Matcher& InstanceShadowMatcher() { +static const Matcher &InstanceShadowMatcher() { static const Matcher matcher(SymbolIdentifierLeaf()); return matcher; } -static bool isInAllowedNode(const SyntaxTreeContext& ctx) { +static bool isInAllowedNode(const SyntaxTreeContext &ctx) { return ctx.IsInside(NodeEnum::kSeqBlock) || ctx.IsInside(NodeEnum::kGenvarDeclaration) || - ctx.IsInside(NodeEnum::kReference); + ctx.IsInside(NodeEnum::kReference); // || + // ctx.IsInside(NodeEnum::kModportSimplePort); } -void InstanceShadowRule::HandleSymbol(const verible::Symbol& symbol, - const SyntaxTreeContext& context) { +void InstanceShadowRule::HandleSymbol(const verible::Symbol &symbol, + const SyntaxTreeContext &context) { verible::matcher::BoundSymbolManager manager; bool omit_node = false; if (!InstanceShadowMatcher().Matches(symbol, &manager)) { @@ -83,29 +84,35 @@ void InstanceShadowRule::HandleSymbol(const verible::Symbol& symbol, // if the considered symbol name is not a declaration if (context.IsInside(NodeEnum::kReference)) return; + // if the considered symbol name is in a modport, discard it + if (context.IsInside(NodeEnum::kModportSimplePort) || + context.IsInside(NodeEnum::kModportClockingPortsDeclaration)) { + return; + } + // TODO: don't latch on to K&R-Style form in which the same symbol shows // up twice. const auto rcontext = reversed_view(context); - const auto* const rdirectParent = *std::next(rcontext.begin()); + const auto *const rdirectParent = *std::next(rcontext.begin()); // we are looking for the potential labels that might overlap the considered // declaration. We are searching all the labels within the visible scope // until we find the node or we reach the top of the scope - for (const auto* node : rcontext) { - for (const verible::SymbolPtr& child : node->children()) { + for (const auto *node : rcontext) { + for (const verible::SymbolPtr &child : node->children()) { if (child == nullptr) { continue; } const auto overlappingLabels = FindAllSymbolIdentifierLeafs(*child); - for (const verible::TreeSearchMatch& omatch : overlappingLabels) { - const auto& overlappingLabel = SymbolCastToLeaf(*omatch.match); + for (const verible::TreeSearchMatch &omatch : overlappingLabels) { + const auto &overlappingLabel = SymbolCastToLeaf(*omatch.match); // variable in different scopes or this is not a // vulnerable declaration if (isInAllowedNode(omatch.context)) { omit_node = true; break; } - const auto& label = SymbolCastToLeaf(*labels[0].match); + const auto &label = SymbolCastToLeaf(*labels[0].match); // if found label has the same adress as considered label // we found the same node so we don't // want to look further diff --git a/verilog/analysis/checkers/instance_shadow_rule.h b/verilog/analysis/checkers/instance_shadow_rule.h index 0db62e483..99df10d58 100644 --- a/verilog/analysis/checkers/instance_shadow_rule.h +++ b/verilog/analysis/checkers/instance_shadow_rule.h @@ -36,10 +36,10 @@ class InstanceShadowRule : public verible::SyntaxTreeLintRule { // Returns the description of the rule implemented formatted for either the // helper flag or markdown depending on the parameter type. - static const LintRuleDescriptor& GetDescriptor(); + static const LintRuleDescriptor &GetDescriptor(); - void HandleSymbol(const verible::Symbol& symbol, - const verible::SyntaxTreeContext& context) override; + void HandleSymbol(const verible::Symbol &symbol, + const verible::SyntaxTreeContext &context) override; verible::LintRuleStatus Report() const override; diff --git a/verilog/analysis/checkers/instance_shadow_rule_test.cc b/verilog/analysis/checkers/instance_shadow_rule_test.cc index b5fdb5e27..9db3c0858 100644 --- a/verilog/analysis/checkers/instance_shadow_rule_test.cc +++ b/verilog/analysis/checkers/instance_shadow_rule_test.cc @@ -76,6 +76,19 @@ TEST(InstanceShadowingTest, FunctionPass) { "int a;\n", "endmodule:foo;\n", }, + { + "interface i;\n", + "logic a;\n", + "modport m (input a);\n", + "endinterface\n", + }, + { + "interface i;\n", + "logic a;\n", + "modport mm(clocking a);\n", + "endinterface\n", + }, + }; RunLintTestCases( kInstanceShadowingTestCases); @@ -156,7 +169,7 @@ TEST(InstanceShadowingTest, CorrectLocationTest) { CHECK(config_status.ok()) << config_status.message(); return instance; }; - for (const auto& test : kInstanceShadowingTestCases) { + for (const auto &test : kInstanceShadowingTestCases) { VerilogAnalyzer analyzer(test.code, "<>"); absl::Status unused_parser_status = analyzer.Analyze(); verible::SyntaxTreeLinter linter_;