Skip to content

Commit

Permalink
Merge pull request #2255 from sconwayaus/instance_shadowing_fix_modport
Browse files Browse the repository at this point in the history
Instance shadowing fix modport
  • Loading branch information
fangism authored Sep 23, 2024
2 parents f4d7237 + bbe85ac commit a602f07
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 16 deletions.
32 changes: 20 additions & 12 deletions verilog/analysis/checkers/instance_shadow_rule.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -61,18 +61,20 @@ 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)) {
Expand All @@ -83,29 +85,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
Expand Down
6 changes: 3 additions & 3 deletions verilog/analysis/checkers/instance_shadow_rule.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
15 changes: 14 additions & 1 deletion verilog/analysis/checkers/instance_shadow_rule_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<VerilogAnalyzer, InstanceShadowRule>(
kInstanceShadowingTestCases);
Expand Down Expand Up @@ -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, "<<inline-test>>");
absl::Status unused_parser_status = analyzer.Analyze();
verible::SyntaxTreeLinter linter_;
Expand Down

0 comments on commit a602f07

Please sign in to comment.