diff --git a/verilog/CST/BUILD b/verilog/CST/BUILD index e3a63e774..30ff07d94 100644 --- a/verilog/CST/BUILD +++ b/verilog/CST/BUILD @@ -299,6 +299,7 @@ cc_test( deps = [ ":expression", ":match-test-utils", + ":verilog-matchers", # fixdeps: keep ":verilog-nonterminals", "//common/analysis:syntax-tree-search", "//common/analysis:syntax-tree-search-test-utils", @@ -748,6 +749,7 @@ cc_test( deps = [ ":match-test-utils", ":statement", + ":verilog-matchers", ":verilog-nonterminals", "//common/analysis:syntax-tree-search", "//common/analysis:syntax-tree-search-test-utils", @@ -755,6 +757,7 @@ cc_test( "//common/text:concrete-syntax-tree", "//common/text:symbol", "//common/text:text-structure", + "//common/text:tree-utils", "//common/util:logging", "//verilog/analysis:verilog-analyzer", "@com_google_absl//absl/strings:string_view", diff --git a/verilog/CST/expression.cc b/verilog/CST/expression.cc index 651099609..388f0372d 100644 --- a/verilog/CST/expression.cc +++ b/verilog/CST/expression.cc @@ -183,4 +183,33 @@ const verible::TokenInfo *ReferenceIsSimpleIdentifier( return ReferenceBaseIsSimple(base_node); } +const verible::SyntaxTreeLeaf *GetIncrementDecrementOperator( + const verible::Symbol &expr) { + if (expr.Kind() != SymbolKind::kNode) return nullptr; + + const SyntaxTreeNode &node = verible::SymbolCastToNode(expr); + + if (!node.MatchesTag(NodeEnum::kIncrementDecrementExpression)) return nullptr; + + // Structure changes depending on the type of IncrementDecrement + bool is_post = node.front().get()->Kind() == SymbolKind::kNode; + + return verible::GetSubtreeAsLeaf( + expr, NodeEnum::kIncrementDecrementExpression, is_post ? 1 : 0); +} + +const verible::SyntaxTreeNode *GetIncrementDecrementOperand( + const verible::Symbol &expr) { + if (expr.Kind() != SymbolKind::kNode) return nullptr; + + const SyntaxTreeNode &node = verible::SymbolCastToNode(expr); + + if (!node.MatchesTag(NodeEnum::kIncrementDecrementExpression)) return nullptr; + + // Structure changes depending on the type of IncrementDecrement + bool is_post = node.front().get()->Kind() == SymbolKind::kNode; + return verible::GetSubtreeAsNode( + expr, NodeEnum::kIncrementDecrementExpression, is_post ? 0 : 1); +} + } // namespace verilog diff --git a/verilog/CST/expression.h b/verilog/CST/expression.h index 2e097f62b..8f6f982dd 100644 --- a/verilog/CST/expression.h +++ b/verilog/CST/expression.h @@ -116,6 +116,16 @@ std::vector FindAllReferenceFullExpressions( const verible::TokenInfo *ReferenceIsSimpleIdentifier( const verible::Symbol &reference); +// Return the operator for a kIncrementDecrementExpression +// This function would extract the leaf containing '++' from expression '++a' +const verible::SyntaxTreeLeaf *GetIncrementDecrementOperator( + const verible::Symbol &expr); + +// Return the operator for a kIncrementDecrementExpression +// This function would extract the leaf containing 'a' from expression '++a' +const verible::SyntaxTreeNode *GetIncrementDecrementOperand( + const verible::Symbol &expr); + } // namespace verilog #endif // VERIBLE_VERILOG_CST_EXPRESSION_H_ diff --git a/verilog/CST/expression_test.cc b/verilog/CST/expression_test.cc index 3d2f3e01b..33f4f4835 100644 --- a/verilog/CST/expression_test.cc +++ b/verilog/CST/expression_test.cc @@ -28,6 +28,7 @@ #include "common/util/logging.h" #include "gtest/gtest.h" #include "verilog/CST/match_test_utils.h" +#include "verilog/CST/verilog_matchers.h" #include "verilog/CST/verilog_nonterminals.h" #include "verilog/analysis/verilog_analyzer.h" #include "verilog/analysis/verilog_excerpt_parse.h" @@ -43,7 +44,7 @@ using verible::TextStructureView; using verible::TreeSearchMatch; TEST(IsZeroTest, NonZero) { - const char* kTestCases[] = { + const char *kTestCases[] = { "a", "a0", "1", @@ -57,7 +58,7 @@ TEST(IsZeroTest, NonZero) { for (auto code : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); @@ -66,7 +67,7 @@ TEST(IsZeroTest, NonZero) { } TEST(IsZeroTest, Zero) { - const char* kTestCases[] = { + const char *kTestCases[] = { "0", "00", "00000", @@ -75,7 +76,7 @@ TEST(IsZeroTest, Zero) { for (auto code : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); @@ -84,7 +85,7 @@ TEST(IsZeroTest, Zero) { } TEST(ConstantIntegerValueTest, NotInteger) { - const char* kTestCases[] = { + const char *kTestCases[] = { "a", "1+1", "(2)", @@ -92,7 +93,7 @@ TEST(ConstantIntegerValueTest, NotInteger) { for (auto code : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); @@ -102,7 +103,7 @@ TEST(ConstantIntegerValueTest, NotInteger) { } TEST(ConstantIntegerValueTest, IsInteger) { - const std::pair kTestCases[] = { + const std::pair kTestCases[] = { {"0", 0}, {"1", 1}, {"666", 666}, @@ -110,7 +111,7 @@ TEST(ConstantIntegerValueTest, IsInteger) { for (auto test : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(test.first, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); @@ -139,10 +140,10 @@ TEST(AssociativeBinaryExpressionsTest, FlatTree) { {"function f;\n", "a = ", {kTag, "b ^ c ^ d ^ e"}, "; endfunction\n"}, {"function f;\n", "a = ", {kTag, "b ~^ c ~^ d ~^ e"}, "; endfunction\n"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); return FindAllBinaryOperations(*ABSL_DIE_IF_NULL(root)); }); } @@ -172,12 +173,12 @@ TEST(AssociativeBinaryExpressionsTest, ThreeFlatOperands) { {kTag, "x*y*z"}, "); endfunction\n"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); auto matches = FindAllBinaryOperations(*ABSL_DIE_IF_NULL(root)); - for (const auto& match : matches) { + for (const auto &match : matches) { // "A op B op C" is 5 sibling tokens, due to flattening EXPECT_EQ(verible::SymbolCastToNode(*ABSL_DIE_IF_NULL(match.match)) .size(), @@ -228,16 +229,16 @@ TEST(GetConditionExpressionPredicateTest, Various) { "endcase\n", "\nendmodule"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); const auto exprs = FindAllConditionExpressions(*ABSL_DIE_IF_NULL(root)); std::vector predicates; - for (const auto& expr : exprs) { - const auto* predicate = + for (const auto &expr : exprs) { + const auto *predicate = GetConditionExpressionPredicate(*expr.match); if (predicate != nullptr) { predicates.push_back( @@ -315,16 +316,16 @@ TEST(GetConditionExpressionTrueCaseTest, Various) { "endcase\n", "\nendmodule"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); const auto exprs = FindAllConditionExpressions(*ABSL_DIE_IF_NULL(root)); std::vector predicates; - for (const auto& expr : exprs) { - const auto* predicate = GetConditionExpressionTrueCase(*expr.match); + for (const auto &expr : exprs) { + const auto *predicate = GetConditionExpressionTrueCase(*expr.match); if (predicate != nullptr) { predicates.push_back( TreeSearchMatch{predicate, {/* ignored context */}}); @@ -401,16 +402,16 @@ TEST(GetConditionExpressionFalseCaseTest, Various) { "endcase\n", "\nendmodule"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); const auto exprs = FindAllConditionExpressions(*ABSL_DIE_IF_NULL(root)); std::vector predicates; - for (const auto& expr : exprs) { - const auto* predicate = + for (const auto &expr : exprs) { + const auto *predicate = GetConditionExpressionFalseCase(*expr.match); if (predicate != nullptr) { predicates.push_back( @@ -427,21 +428,21 @@ TEST(GetConditionExpressionFalseCaseTest, Various) { } TEST(GetUnaryPrefixOperator, Exprs) { - const std::pair kTestCases[] = { + const std::pair kTestCases[] = { {"-(2)", "-"}, {"-1", "-"}, {"&1", "&"}, {"666", nullptr}, {"1 + 2", nullptr}, {"!1", "!"}, }; for (auto test : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(test.first, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); - const verible::Symbol* last_node = DescendThroughSingletons(*node); + const verible::Symbol *last_node = DescendThroughSingletons(*node); if (test.second) { - const verible::SyntaxTreeNode& unary_expr = + const verible::SyntaxTreeNode &unary_expr = verible::SymbolCastToNode(*last_node); EXPECT_EQ(NodeEnum(unary_expr.Tag().tag), NodeEnum::kUnaryPrefixExpression); @@ -455,20 +456,20 @@ TEST(GetUnaryPrefixOperator, Exprs) { } TEST(GetUnaryPrefixOperand, Exprs) { - const std::pair kTestCases[] = { + const std::pair kTestCases[] = { {"-1", ""}, {"&1", ""}, {"666", nullptr}, {"1 + 2", nullptr}, {"!1", ""}, }; for (auto test : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(test.first, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); - const verible::Symbol* last_node = DescendThroughSingletons(*node); + const verible::Symbol *last_node = DescendThroughSingletons(*node); if (test.second) { - const verible::SyntaxTreeNode& unary_expr = + const verible::SyntaxTreeNode &unary_expr = verible::SymbolCastToNode(*last_node); EXPECT_EQ(NodeEnum(unary_expr.Tag().tag), NodeEnum::kUnaryPrefixExpression); @@ -516,10 +517,10 @@ TEST(FindAllConditionExpressionsTest, Various) { "endcase\n", "\nendmodule"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); return FindAllConditionExpressions(*ABSL_DIE_IF_NULL(root)); }); } @@ -629,17 +630,17 @@ TEST(FindAllReferenceExpressionsTest, Various) { // reference could contain other references like "a[a]", but the testing // framework doesn't support nested expected ranges... yet. }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); return FindAllReferenceFullExpressions(*ABSL_DIE_IF_NULL(root)); }); } } TEST(ReferenceIsSimpleTest, Simple) { - const char* kTestCases[] = { + const char *kTestCases[] = { "a", "bbb", "z1", @@ -648,7 +649,7 @@ TEST(ReferenceIsSimpleTest, Simple) { for (auto code : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); { const auto status = analyzer_ptr->LexStatus(); ASSERT_TRUE(status.ok()) << status.message(); @@ -659,8 +660,8 @@ TEST(ReferenceIsSimpleTest, Simple) { } const std::vector refs( FindAllReferenceFullExpressions(*ABSL_DIE_IF_NULL(node))); - for (const auto& ref : refs) { - const verible::TokenInfo* token = ReferenceIsSimpleIdentifier(*ref.match); + for (const auto &ref : refs) { + const verible::TokenInfo *token = ReferenceIsSimpleIdentifier(*ref.match); ASSERT_NE(token, nullptr) << "reference: " << code; EXPECT_EQ(token->text(), code); } @@ -668,7 +669,7 @@ TEST(ReferenceIsSimpleTest, Simple) { } TEST(ReferenceIsSimpleTest, NotSimple) { - const char* kTestCases[] = { + const char *kTestCases[] = { "a[0]", "a[4][6]", "bbb[3:0]", "x.y", "x.y.z", "x[0].y[1].z[2]", "x()", "x.y()", "x()[0]", "x(1)", "f(0,1)", "j[9].k(3, 2, 1)", }; @@ -676,7 +677,7 @@ TEST(ReferenceIsSimpleTest, NotSimple) { VLOG(1) << __FUNCTION__ << " test: " << code; const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); { const auto status = analyzer_ptr->LexStatus(); ASSERT_TRUE(status.ok()) << status.message(); @@ -687,7 +688,7 @@ TEST(ReferenceIsSimpleTest, NotSimple) { } const std::vector refs( FindAllReferenceFullExpressions(*ABSL_DIE_IF_NULL(node))); - for (const auto& ref : refs) { + for (const auto &ref : refs) { VLOG(1) << "match: " << verible::StringSpanOfSymbol(*ref.match); EXPECT_FALSE(ReferenceIsSimpleIdentifier(*ref.match)) << "reference: " << code; @@ -695,5 +696,93 @@ TEST(ReferenceIsSimpleTest, NotSimple) { } } +TEST(GetIncrementDecrementOperatorTest, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m; endmodule\n"}, + {"module m;\ninitial begin end\nendmodule"}, + {"module m;\nalways_comb begin\n" + "a", + {kTag, "++"}, + ";\nend\nendmodule"}, + {"module m;\nalways_comb begin\n", + {kTag, "++"}, + "a;" + "\nend\nendmodule"}, + {"module m;\nalways_comb begin\n" + "somelargename", + {kTag, "++"}, + ";\nend\nendmodule"}, + {"module m;\nalways_comb begin\n", + {kTag, "++"}, + "somelargename;\n" + "end\nendmodule"}, + {"module m;\nalways_comb begin\nk = a + 2;\nend\nendmodule"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto exprs = verible::SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekIncrementDecrementExpression()); + + std::vector operators; + for (const auto &expr : exprs) { + const auto *operator_ = GetIncrementDecrementOperator(*expr.match); + operators.push_back( + TreeSearchMatch{operator_, {/* ignored context */}}); + } + return operators; + }); + } +} + +TEST(GetIncrementDecrementOperandTest, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m; endmodule\n"}, + {"module m;\ninitial begin end\nendmodule"}, + {"module m;\n" + "always_comb begin\n", + {kTag, "a"}, + "++;\nend\nendmodule"}, + {"module m;\n" + "always_comb begin\n" + "++", + {kTag, "a"}, + ";\nend\nendmodule"}, + {"module m;\n" + "always_comb begin\n", + {kTag, "somelargename"}, + "++;\nend\nendmodule"}, + {"module m;\n" + "always_comb begin\n++", + {kTag, "somelargename"}, + ";\nend\nendmodule"}, + {"module m;\n" + "always_comb begin\n" + "k = a + 2;\n" + "end\nendmodule"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto exprs = verible::SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekIncrementDecrementExpression()); + + std::vector operands; + for (const auto &expr : exprs) { + const auto *operand = GetIncrementDecrementOperand(*expr.match); + operands.push_back( + TreeSearchMatch{operand, {/* ignored context */}}); + } + return operands; + }); + } +} + } // namespace } // namespace verilog diff --git a/verilog/CST/statement.cc b/verilog/CST/statement.cc index 71b9d9d61..98675538a 100644 --- a/verilog/CST/statement.cc +++ b/verilog/CST/statement.cc @@ -43,6 +43,11 @@ std::vector FindAllGenerateBlocks( return SearchSyntaxTree(root, NodekGenerateBlock()); } +std::vector FindAllNonBlockingAssignments( + const verible::Symbol &root) { + return SearchSyntaxTree(root, NodekNonblockingAssignmentStatement()); +} + static const SyntaxTreeNode *GetGenericStatementBody( const SyntaxTreeNode *node) { if (!node) return node; @@ -503,4 +508,31 @@ const verible::Symbol *GetEventControlFromProceduralTimingControl( 0, NodeEnum::kEventControl); } +const verible::SyntaxTreeNode *GetNonBlockingAssignmentLhs( + const verible::SyntaxTreeNode &non_blocking_assignment) { + return verible::GetSubtreeAsNode( + non_blocking_assignment, NodeEnum::kNonblockingAssignmentStatement, 0); +} + +const verible::SyntaxTreeNode *GetNonBlockingAssignmentRhs( + const verible::SyntaxTreeNode &non_blocking_assignment) { + return verible::GetSubtreeAsNode( + non_blocking_assignment, NodeEnum::kNonblockingAssignmentStatement, 3); +} + +const verible::SyntaxTreeNode *GetIfClauseHeader( + const verible::SyntaxTreeNode &if_clause) { + return verible::GetSubtreeAsNode(if_clause, NodeEnum::kIfClause, 0); +} + +const verible::SyntaxTreeNode *GetIfHeaderExpression( + const verible::SyntaxTreeNode &if_header) { + const verible::SyntaxTreeNode *paren_group = + verible::GetSubtreeAsNode(if_header, NodeEnum::kIfHeader, 2); + + if (!paren_group) return nullptr; + + return verible::GetSubtreeAsNode(*paren_group, NodeEnum::kParenGroup, 1); +} + } // namespace verilog diff --git a/verilog/CST/statement.h b/verilog/CST/statement.h index 8ff2caf6c..2140d6ded 100644 --- a/verilog/CST/statement.h +++ b/verilog/CST/statement.h @@ -33,6 +33,10 @@ std::vector FindAllForLoopsInitializations( std::vector FindAllGenerateBlocks( const verible::Symbol &root); +// Extract every nonblocking assignment starting from root +std::vector FindAllNonBlockingAssignments( + const verible::Symbol &root); + // Generate flow control constructs // // TODO(fangism): consider moving the *GenerateBody functions to generate.{h,cc} @@ -202,6 +206,22 @@ const verible::SyntaxTreeNode *GetProceduralTimingControlFromAlways( const verible::Symbol *GetEventControlFromProceduralTimingControl( const verible::SyntaxTreeNode &proc_timing_ctrl); +// Return the left hand side (lhs) from a nonblocking assignment +// Example: get 'x' from 'x <= y' +const verible::SyntaxTreeNode *GetNonBlockingAssignmentLhs( + const verible::SyntaxTreeNode &non_blocking_assignment); + +// Return the right hand side (rhs) from a nonblocking assignment +// Example: get 'y' from 'x <= y' +const verible::SyntaxTreeNode *GetNonBlockingAssignmentRhs( + const verible::SyntaxTreeNode &non_blocking_assignment); + +const verible::SyntaxTreeNode *GetIfClauseHeader( + const verible::SyntaxTreeNode &if_clause); + +const verible::SyntaxTreeNode *GetIfHeaderExpression( + const verible::SyntaxTreeNode &if_header); + } // namespace verilog #endif // VERIBLE_VERILOG_CST_STATEMENT_H_ diff --git a/verilog/CST/statement_test.cc b/verilog/CST/statement_test.cc index 100f63887..1e6bad91a 100644 --- a/verilog/CST/statement_test.cc +++ b/verilog/CST/statement_test.cc @@ -24,9 +24,11 @@ #include "common/text/concrete_syntax_tree.h" #include "common/text/symbol.h" #include "common/text/text_structure.h" +#include "common/text/tree_utils.h" #include "common/util/logging.h" #include "gtest/gtest.h" #include "verilog/CST/match_test_utils.h" +#include "verilog/CST/verilog_matchers.h" #include "verilog/CST/verilog_nonterminals.h" #include "verilog/analysis/verilog_analyzer.h" @@ -1368,5 +1370,220 @@ TEST(GetGenerateBlockEndTest, Various) { } } +TEST(FindAllNonBlockingAssignmentTest, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {"module m;\n" + "always @(*) x = 1;\n" + "endmodule"}, + {"module m;\n" + "always_ff @(posedge clk) ", + {kTag, "x <= 1;"}, + "\nendmodule"}, + {"module m;\n" + "always_ff @(posedge clk) begin\n", + {kTag, "a <= b;"}, + "\n", + {kTag, "c <= d();"}, + "\n", + {kTag, "e <= `F;"}, + "\nend endmodule"}, + {"module m;\n" + "always_latch ", + {kTag, "x <= 1;"}, + "\nendmodule"}, + {"module m;\n" + "always@(*) ", + {kTag, "x[1] <= y[2];"}, + "\nendmodule"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + return FindAllNonBlockingAssignments(*ABSL_DIE_IF_NULL(root)); + }); + } +} + +TEST(GetNonBlockingAssignmentRhsTest, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {"module m;\n" + "always @(*) x = 1;\n" + "endmodule"}, + {"module m;\n" + "always_ff @(posedge clk) " + "x <= ", + {kTag, "1"}, + ";\nendmodule"}, + {"module m;\n" + "always_ff @(posedge clk) begin\n" + "a <= ", + {kTag, "b"}, + ";\nc <= ", + {kTag, "d()"}, + ";\ne <= ", + {kTag, "`F"}, + ";\nend endmodule"}, + {"module m;\n" + "always_latch " + "x <= ", + {kTag, "1"}, + ";\nendmodule"}, + {"module m;\n" + "always@(*) x[1] <= ", + {kTag, "y[2]"}, + ";\nendmodule"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &non_blocking_assignments = + FindAllNonBlockingAssignments(*ABSL_DIE_IF_NULL(root)); + + std::vector right_hand_sides; + right_hand_sides.reserve(non_blocking_assignments.size()); + for (const auto &assignment : non_blocking_assignments) { + const auto *rhs = GetNonBlockingAssignmentRhs( + verible::SymbolCastToNode(*assignment.match)); + right_hand_sides.emplace_back( + TreeSearchMatch{rhs, {/* ignored context */}}); + } + return right_hand_sides; + }); + } +} + +TEST(GetNonBlockingAssignmentLhsTest, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {"module m;\n" + "always @(*) x = 1;\n" + "endmodule"}, + {"module m;\n" + "always_ff @(posedge clk) ", + {kTag, "x"}, + " <= 1;\n", + "endmodule"}, + {"module m;\n" + "always_ff @(posedge clk) begin\n", + {kTag, "a"}, + " <= b;\n", + {kTag, "c"}, + " <= d();\n", + {kTag, "e"}, + " <= `F;\n" + "end endmodule"}, + {"module m;\n" + "always_latch ", + {kTag, "x"}, + " <= 1;\n" + "endmodule"}, + {"module m;\n" + "always@(*) ", + {kTag, "x[1]"}, + " <= y[2];\n" + "endmodule"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &non_blocking_assignments = + FindAllNonBlockingAssignments(*ABSL_DIE_IF_NULL(root)); + + std::vector left_hand_sides; + left_hand_sides.reserve(non_blocking_assignments.size()); + for (const auto &assignment : non_blocking_assignments) { + const auto *lhs = GetNonBlockingAssignmentLhs( + verible::SymbolCastToNode(*assignment.match)); + left_hand_sides.emplace_back( + TreeSearchMatch{lhs, {/* ignored context */}}); + } + return left_hand_sides; + }); + } +} + +TEST(GetIfClauseHeaderTest, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {"module m;\n" + "always @(*)\n", + {kTag, "if (y)"}, + " x = 1;\n" + "endmodule"}, + {"module m;\n" + "always @(*) x = 1;\n" + "endmodule"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &if_clauses = + verible::SearchSyntaxTree(*root, NodekIfClause()); + + std::vector if_headers; + if_headers.reserve(if_clauses.size()); + for (const auto &if_clause : if_clauses) { + const auto *header = + GetIfClauseHeader(verible::SymbolCastToNode(*if_clause.match)); + if_headers.emplace_back( + TreeSearchMatch{header, {/* ignored context */}}); + } + return if_headers; + }); + } +} + +TEST(GetIfClauseExpressionTest, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {"module m;\n" + "always @(*) if(", + {kTag, "y"}, + ")\n " + "x = 1;\n" + "endmodule"}, + {"module m;\n" + "always @(*) if(", + {kTag, "func()"}, + ") x = 1;\n" + "endmodule"}, + {"module m;\n" + "always @(*) if(", + {kTag, "y"}, + ") begin\n", + "if(", + {kTag, "z"}, + ") x = 1;\n" + "end endmodule"}, + {"module m;\n" + "always @(*) x = 1;\n" + "endmodule"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &if_headers = + verible::SearchSyntaxTree(*root, NodekIfHeader()); + + std::vector if_expressions; + if_expressions.reserve(if_headers.size()); + for (const auto &if_header : if_headers) { + const auto *expression = GetIfHeaderExpression( + verible::SymbolCastToNode(*if_header.match)); + if_expressions.emplace_back( + TreeSearchMatch{expression, {/* ignored context */}}); + } + return if_expressions; + }); + } +} + } // namespace } // namespace verilog diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index fcb95088f..3cc23395f 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -21,6 +21,7 @@ cc_library( ":case-missing-default-rule", ":constraint-name-style-rule", ":create-object-name-match-rule", + ":dff-name-style-rule", ":disable-statement-rule", ":endif-comment-rule", ":enum-name-style-rule", @@ -2276,3 +2277,51 @@ cc_test( "@com_google_googletest//:gtest_main", ], ) + +cc_library( + name = "dff-name-style-rule", + srcs = ["dff_name_style_rule.cc"], + hdrs = ["dff_name_style_rule.h"], + deps = [ + "//common/analysis:lint-rule-status", + "//common/analysis:syntax-tree-lint-rule", + "//common/analysis/matcher", + "//common/analysis/matcher:bound-symbol-manager", + "//common/analysis/matcher:core-matchers", + "//common/text:concrete-syntax-leaf", + "//common/text:concrete-syntax-tree", + "//common/text:config-utils", + "//common/text:symbol", + "//common/text:syntax-tree-context", + "//common/text:token-info", + "//common/text:tree-utils", + "//verilog/CST:expression", + "//verilog/CST:statement", + "//verilog/CST:verilog-matchers", + "//verilog/CST:verilog-nonterminals", + "//verilog/analysis:descriptions", + "//verilog/analysis:lint-rule-registry", + "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/status", + "@com_google_absl//absl/strings", + "@com_google_absl//absl/strings:string_view", + "@com_googlesource_code_re2//:re2", + ], + alwayslink = 1, +) + +cc_test( + name = "dff-name-style-rule_test", + srcs = ["dff_name_style_rule_test.cc"], + deps = [ + ":dff-name-style-rule", + "//common/analysis:linter-test-utils", + "//common/analysis:syntax-tree-linter-test-utils", + "//verilog/analysis:verilog-analyzer", + "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/log:check", + "@com_google_absl//absl/strings:string_view", + "@com_google_googletest//:gtest", + "@com_google_googletest//:gtest_main", + ], +) diff --git a/verilog/analysis/checkers/dff_name_style_rule.cc b/verilog/analysis/checkers/dff_name_style_rule.cc new file mode 100644 index 000000000..ea2f6f177 --- /dev/null +++ b/verilog/analysis/checkers/dff_name_style_rule.cc @@ -0,0 +1,416 @@ +// Copyright 2017-2023 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/dff_name_style_rule.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "absl/status/status.h" +#include "absl/strings/ascii.h" +#include "absl/strings/match.h" +#include "absl/strings/str_cat.h" +#include "absl/strings/str_join.h" +#include "absl/strings/str_split.h" +#include "absl/strings/string_view.h" +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/matcher/bound_symbol_manager.h" +#include "common/analysis/matcher/core_matchers.h" +#include "common/analysis/matcher/matcher.h" +#include "common/text/concrete_syntax_leaf.h" +#include "common/text/concrete_syntax_tree.h" +#include "common/text/config_utils.h" +#include "common/text/symbol.h" +#include "common/text/syntax_tree_context.h" +#include "common/text/token_info.h" +#include "common/text/tree_utils.h" +#include "re2/re2.h" +#include "verilog/CST/expression.h" +#include "verilog/CST/statement.h" +#include "verilog/CST/verilog_matchers.h" +#include "verilog/CST/verilog_nonterminals.h" +#include "verilog/analysis/descriptions.h" +#include "verilog/analysis/lint_rule_registry.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { + +using verible::LintRuleStatus; +using verible::LintViolation; +using verible::SyntaxTreeContext; +using verible::config::SetString; +using verible::matcher::Matcher; + +// Register DffNameStyleRule. +VERILOG_REGISTER_LINT_RULE(DffNameStyleRule); + +const LintRuleDescriptor &DffNameStyleRule::GetDescriptor() { + static const LintRuleDescriptor d{ + .name = "dff-name-style", + .topic = "dff-name-style", + .desc = + "Checks that D Flip-Flops use appropiate naming conventions in both " + "input and output ports. The left hand side (output) and right hand " + "side (input) are checked against a set of valid suffixes. " + "Additionally, register names might end in a number " + "to denote the pipeline stage index (var_q/var_q1, var_q2, ...). " + "Pipelined signals must get their value from the previous stage: " + "var_q3 <= var_q2. " + "Exceptions to this rule can be configured using a regular " + "expression or waiving whole `if` blocks", + .param = { + {"input", std::string(kDefaultInputSuffixes), + "Comma separated list of allowed suffixes for the input port. " + "Suffixes should not include the preceding \"_\". Empty field " + "means no checks for the input port"}, + {"output", std::string(kDefaultOutputSuffixes), + "Comma separated list of allowed suffixes for the output port. " + "Should not include the preceding \"_\". Empty field means no " + "checks for the output port"}, + {"waive_ifs_with_conditions", std::string(kDefaultWaiveConditions), + "Comma separated list of conditions that will disable the rule " + "inside the `if`s they are evaluated in"}, + {"waive_lhs_regex", std::string(kDefaultWaiveRegex), + "Nonblocking assigments whose lhs match the regex will not be " + "evaluated"}, + }}; + return d; +} + +static const Matcher &AlwaysFFMatcher() { + static const Matcher matcher(NodekAlwaysStatement(AlwaysFFKeyword())); + return matcher; +} + +// Ensure that variables being driven by a blocking assigment don't follow the +// naming convention of DFF outputs +void DffNameStyleRule::HandleBlockingAssignments( + const verible::Symbol &symbol, const verible::SyntaxTreeContext &context) { + const verible::SyntaxTreeNode &node = SymbolCastToNode(symbol); + if (node.empty()) return; + + const verible::Symbol *driven_variable = node.front().get(); + + // Only "++var" has its identifier in a position other than the 1st child + if (node.MatchesTag(NodeEnum::kIncrementDecrementExpression)) { + driven_variable = GetIncrementDecrementOperand(symbol); + } + + absl::string_view lhs_str = verible::StringSpanOfSymbol(*driven_variable); + + const bool found_id = + std::any_of(valid_output_suffixes.cbegin(), valid_output_suffixes.cend(), + [&](const std::string &suffix) -> bool { + return lhs_str.size() > suffix.size() && + absl::EndsWith(lhs_str, suffix); + }); + + if (!found_id) return; + violations_.insert(LintViolation( + *driven_variable, + absl::StrCat(lhs_str, + " should be driven by a nonblocking assignment " + "inside an always_ff block"), + context)); +} + +// Ensure that variables being driven by a nonblocking assigment follow the +// naming convention of DFF outputs +void DffNameStyleRule::HandleNonBlockingAssignments( + const verible::Symbol &non_blocking_assignment, + const verible::SyntaxTreeContext &context) { + const verible::SyntaxTreeNode &node = + verible::SymbolCastToNode(non_blocking_assignment); + + const verible::Symbol &lhs = *GetNonBlockingAssignmentLhs(node); + const verible::SyntaxTreeNode &rhs_expr = *GetNonBlockingAssignmentRhs(node); + + absl::string_view lhs_str = verible::StringSpanOfSymbol(lhs); + absl::string_view rhs_str = verible::StringSpanOfSymbol(rhs_expr); + + // If this variable matches the waive regex, ignore it. + if (waive_lhs_regex && RE2::FullMatch(lhs_str, *waive_lhs_regex)) { + return; + } + + auto [clean_lhs_str, lhs_pipe_stage] = ExtractPipelineStage(lhs_str); + + // Check if the string without the pipeline number has a valid format + absl::string_view lhs_base = + CheckSuffix(context, lhs, clean_lhs_str, valid_output_suffixes); + // If the base is empty, lhs is wrongly formatted. Stop making more checks + if (lhs_base.empty()) return; + + // Pipeline stage present on the lhs + // ID_suffixN <= expr; + if (uint64_t lhs_stage = lhs_pipe_stage.value_or(0); + lhs_stage > kFirstValidPipeStage) { + // TODO: properly document this feature once examples can be shown in rule + // description message "data_qN" should be driven by "data_qN-1", but + + // "data_q2" can be driven by "data_q" or "data_q1" + // "data_q" and "data_q1" should be driven by "data_n" + std::string expected_rhs = absl::StrCat(clean_lhs_str, lhs_stage - 1); + const bool second_stage = lhs_stage == (kFirstValidPipeStage + 1); + + // Note: mixing suffixes when using pipeline identifiers + // is not allowed + // data_q2 <= data_q -> OK + // data_q2 <= data_ff -> WRONG + if (rhs_str != expected_rhs && + !(second_stage && rhs_str == clean_lhs_str)) { + violations_.insert(LintViolation( + rhs_expr, absl::StrCat(rhs_str, " Should be ", expected_rhs), + context)); + } + return; + } + + absl::string_view rhs_base = + CheckSuffix(context, rhs_expr, rhs_str, valid_input_suffixes); + + // If the rhs is wrongly formatted, there is no need to check that the + // bases match + if (rhs_base.empty()) return; + + if (lhs_base != rhs_base) { + // Bases should be equal + // "a_q <= a_n" -> OK + // "a_q <= b_n" -> WRONG + violations_.insert(LintViolation( + non_blocking_assignment, + absl::StrCat("Both parts before the suffix should be equal, but \"", + lhs_base, "\" != \"", rhs_base, "\""), + context)); + } +} + +void DffNameStyleRule::HandleSymbol(const verible::Symbol &symbol, + const SyntaxTreeContext &context) { + verible::matcher::BoundSymbolManager manager; + if (valid_input_suffixes.empty() && valid_output_suffixes.empty()) return; + + // Types of assignments intended for DFF inputs + static const Matcher blocking_assignment_matcher = verible::matcher::AnyOf( + NodekContinuousAssignmentStatement(), NodekNetVariableAssignment(), + NodekNetDeclarationAssignment(), NodekAssignModifyStatement(), + NodekIncrementDecrementExpression()); + if (blocking_assignment_matcher.Matches(symbol, &manager)) { + HandleBlockingAssignments(symbol, context); + return; + } + + // From this point, ignore everything that isn't a nonblocking assignment + // inside an always_ff block + if (!NodekNonblockingAssignmentStatement().Matches(symbol, &manager) || + !context.NearestParentMatching( + [&](const verible::SyntaxTreeNode &node) -> bool { + return AlwaysFFMatcher().Matches(node, &manager); + })) { + return; + } + + // Waive if this particular nonblocking assignment is inside an if block + // related to a reset signal, as we are not capable of determining + // reset-values from analyzing the source code. + const verible::SyntaxTreeNode *waive = context.NearestParentMatching( + [&](const verible::SyntaxTreeNode &node) -> bool { + if (!node.MatchesTag(NodeEnum::kIfClause)) { + return false; + } + + const verible::SyntaxTreeNode *if_header = GetIfClauseHeader(node); + if (!if_header) return false; + const verible::Symbol *s = GetIfHeaderExpression(*if_header); + + if (!s) return false; + absl::string_view paren_str = + absl::StripAsciiWhitespace(verible::StringSpanOfSymbol(*s)); + + // EXACT matching w.r.t waive_ifs_with_conditions. Substring checking + // isn't really appropriate because we would have to check for tricky + // stuff like negation, negation around parenthesis... + return std::find(waive_ifs_with_conditions.cbegin(), + waive_ifs_with_conditions.cend(), + paren_str) != waive_ifs_with_conditions.cend(); + }); + + if (waive) { + return; + } + + HandleNonBlockingAssignments(symbol, context); +} + +absl::string_view DffNameStyleRule::CheckSuffix( + const verible::SyntaxTreeContext &context, const verible::Symbol &root, + absl::string_view id, const std::vector &suffixes) { + // Identifier is split between base and suffix: + // "myid_q" => {"myid", "_q"} + absl::string_view base; + // If there are no patterns to check against, everything passes the check + if (suffixes.empty()) return base; + + // Lhs and Rhs should be plain variable identifiers + const verible::SyntaxTreeLeaf *leftmost_leaf = verible::GetLeftmostLeaf(root); + const verible::SyntaxTreeLeaf *rightmost_leaf = + verible::GetRightmostLeaf(root); + + // TODO: this is notably restrictive, we might want to allow things like + // `data_q[index] <= data_n` + if (leftmost_leaf != rightmost_leaf || + leftmost_leaf->get().token_enum() != SymbolIdentifier) { + violations_.insert(LintViolation( + root, + absl::StrCat( + id, " Should be a simple reference, ending with a valid suffix: {", + absl::StrJoin(suffixes, ","), "}"), + context)); + + return base; + } + + absl::string_view suffix_match; + // Check if id conforms to any valid suffix + const bool id_ok = std::any_of(suffixes.cbegin(), suffixes.cend(), + [&](const std::string &suffix) -> bool { + if (absl::EndsWith(id, suffix)) { + base = absl::string_view( + id.begin(), id.size() - suffix.size()); + suffix_match = suffix; + return true; + } + return false; + }); + + // Exact match: id: "_q", suffix: "_q", there is no base + bool exact_match = suffix_match.size() == id.size(); + if (id_ok && !exact_match) return base; + + // The identifier can't be just the suffix we're matching against + const std::string lint_message = + exact_match + ? absl::StrCat( + "A valid identifier should not exactly match a valid prefix \"", + id, "\" == \"", suffix_match, "\"") + : absl::StrCat(id, " should end with a valid suffix: {", + absl::StrJoin(suffixes, ","), "}"); + violations_.insert(LintViolation(root, lint_message, context)); + + return base; +} + +absl::Status DffNameStyleRule::Configure(absl::string_view configuration) { + // If configuration is empty, stick to the default + if (configuration.empty()) return absl::OkStatus(); + + // Default values + std::string output{kDefaultOutputSuffixes.data(), + kDefaultOutputSuffixes.length()}; + std::string input{kDefaultInputSuffixes.data(), + kDefaultInputSuffixes.length()}; + std::string waive_ifs_with_conditions_str{kDefaultWaiveConditions.data(), + kDefaultWaiveConditions.length()}; + + absl::Status status = verible::ParseNameValues( + configuration, + {{"output", SetString(&output)}, + {"input", SetString(&input)}, + {"waive_lhs_regex", verible::config::SetRegex(&waive_lhs_regex)}, + {"waive_ifs_with_conditions", + SetString(&waive_ifs_with_conditions_str)}}); + + if (!status.ok()) return status; + + valid_input_suffixes = ProcessSuffixes(input); + valid_output_suffixes = ProcessSuffixes(output); + waive_ifs_with_conditions = + absl::StrSplit(waive_ifs_with_conditions_str, ','); + + // Trim whitespaces for user input + std::for_each( + waive_ifs_with_conditions.begin(), waive_ifs_with_conditions.end(), + [](std::string &str) -> void { absl::RemoveExtraAsciiWhitespace(&str); }); + + return status; +} + +std::vector DffNameStyleRule::ProcessSuffixes( + absl::string_view config) { + // Split input string: "q,ff,reg" => {"q", "ff", "reg"} + std::vector split_suffixes = + absl::StrSplit(config, ',', absl::SkipEmpty()); + + std::vector result(split_suffixes.size()); + + // Prepend an underscore to the suffixes to check against them + // {"q", "ff", "reg"} => {"_q", "_ff", "_reg"} + const auto prepend_underscore = [](absl::string_view str) -> std::string { + return absl::StrCat("_", str); + }; + std::transform(split_suffixes.cbegin(), split_suffixes.cend(), result.begin(), + prepend_underscore); + + return result; +} + +std::pair > +DffNameStyleRule::ExtractPipelineStage(absl::string_view id) { + // Find the number of trailing digits inside the identifier + absl::string_view::const_reverse_iterator last_non_num = std::find_if( + id.rbegin(), id.rend(), [](unsigned char c) { return !std::isdigit(c); }); + uint64_t num_digits = + static_cast(std::distance(id.rbegin(), last_non_num)); + + // If there are no trailing digits, or the id is just composed + // of digits + if (num_digits == 0 || num_digits == id.size()) return {id, {}}; + + // Extract the integer value for the pipeline stage + uint64_t pipe_stage; + std::from_chars_result result = std::from_chars( + id.cbegin() + id.size() - num_digits, id.cend(), pipe_stage); + + // https://en.cppreference.com/w/cpp/utility/from_chars + // Check whether: + // - There are errors parsing the string + // - There are non-numeric characters inside the range (shouldn't!) + // - The pipeline stage number is in the valid range + if (result.ec != std::errc() || result.ptr != id.end() || + pipe_stage < kFirstValidPipeStage) { + return {id, {}}; + } + + // Return the id without the trailing digits so we can do the suffix check + // and the value for the pipeline stage + id.remove_suffix(num_digits); + return {id, pipe_stage}; +} + +LintRuleStatus DffNameStyleRule::Report() const { + return LintRuleStatus(violations_, GetDescriptor()); +} + +} // namespace analysis +} // namespace verilog diff --git a/verilog/analysis/checkers/dff_name_style_rule.h b/verilog/analysis/checkers/dff_name_style_rule.h new file mode 100644 index 000000000..f22aa83ed --- /dev/null +++ b/verilog/analysis/checkers/dff_name_style_rule.h @@ -0,0 +1,166 @@ +// Copyright 2017-2023 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_CONSTRAINT_NAME_STYLE_RULE_H_ +#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_CONSTRAINT_NAME_STYLE_RULE_H_ + +#include +#include +#include +#include +#include +#include +#include + +#include "absl/status/status.h" +#include "absl/strings/str_split.h" +#include "absl/strings/string_view.h" +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/syntax_tree_lint_rule.h" +#include "common/text/symbol.h" +#include "common/text/syntax_tree_context.h" +#include "re2/re2.h" +#include "verilog/analysis/descriptions.h" + +namespace verilog { +namespace analysis { + +// DffNameStyleRule checks that D flip flops use appropriate naming conventions +// for both their input and outputs. +// +// The rule can be configured via specifying a comma-separated list of +// suffixes (one for input, one for output). Providing an empty list +// means no checks for the corresponding field +// +// For example, the defaults are equivalent to: +// +dff-name-style=output:reg,r,ff,q;input:next,n,d +// Which gets expanded into +// `valid_output_suffixes` = { "reg", "r", "ff", "q" } +// `valid_input_suffixes` = { "next", "n", "d" } +// +// Given a nonblocking assignment inside an always_ff block +// we will then check the left-hand side (lhs) and right +// hand side (rhs) of the assignment against the valid +// suffixes +// +// Given "data_q <= data_n", we will check that: +// - data_q ends with any of { "_reg", "_r", "_ff", "_q" } +// - data_n ends with any of { "_next", "_n", "_d" } +// +// If those checks succeed, we will also check that their prefixes +// are equal. +// data_q <= data_n -> OK +// data_q <= something_n -> WRONG: "data" != "something" +// +// Following [1] we also allow for using trailing numbers +// in the identifiers to specify the pipeline stage where +// a given signal comes from. +// +// Under this convention, data_q3 should be driven by the +// previous stage data_q2 +// +// The rule might not always be applicable. Apart from manual waiving, there are +// two supported ways to disable the checks: +// 1. Using the `waive_ifs_with_conditions` argument, we can specify certain +// `if`s under which the rule shouldn't apply. For example: +// if(!rst_ni) data_q <= '{default: 0}; +// 2. `waive_lhs_regex` lets us disable the check for some nonblocking +// assignments. +// +// +// clang-format off +// [1] https://github.com/lowRISC/style-guides/blob/9b47bff75b19696e23a43f38ee7161112705e1e3/VerilogCodingStyle.md#suffixes +// clang-format on +class DffNameStyleRule : public verible::SyntaxTreeLintRule { + public: + using rule_type = verible::SyntaxTreeLintRule; + inline static constexpr absl::string_view kDefaultInputSuffixes = "next,n,d"; + inline static constexpr absl::string_view kDefaultOutputSuffixes = + "reg,r,ff,q"; + inline static constexpr absl::string_view kDefaultWaiveRegex = "(?i)mem.*"; + inline static constexpr absl::string_view kDefaultWaiveConditions = + "!rst_ni,flush_i,!rst_ni || flush_i,flush_i || !rst_ni"; + + static const LintRuleDescriptor &GetDescriptor(); + + void HandleSymbol(const verible::Symbol &symbol, + const verible::SyntaxTreeContext &context) final; + + verible::LintRuleStatus Report() const final; + + absl::Status Configure(absl::string_view) final; + + // Identifiers can optionally include a trailing number + // indicating the pipeline stage where the signal originates from. + // + // This function returns the identifier without the pipeline stage, and the + // integer value of the pipeline stage (> kFirstValidPipeStage) if present + // + // Examples: + // ExtractPipelineStage("data_q") => {"data_q", {})} + // ExtractPipelineStage("data_q1") => {"data_q1", {})} + // ExtractPipelineStage("data_q2") => {"data_q", 2)} + // https://github.com/lowRISC/style-guides/blob/9b47bff75b19696e23a43f38ee7161112705e1e3/VerilogCodingStyle.md#suffixes-for-signals-and-types + static std::pair> + ExtractPipelineStage(absl::string_view id); + + private: + absl::string_view CheckSuffix(const verible::SyntaxTreeContext &context, + const verible::Symbol &root, + absl::string_view id, + const std::vector &suffixes); + + void HandleBlockingAssignments(const verible::Symbol &symbol, + const verible::SyntaxTreeContext &context); + + void HandleNonBlockingAssignments( + const verible::Symbol &non_blocking_assignment, + const verible::SyntaxTreeContext &context); + + // Extract the individual suffixes from the comma separated list + // coming from configuration. + // "q,ff,reg" => { "q", "ff", "reg" } + // + // Used to initialize `valid_input_suffixes` and `valid_output_suffixes` + static std::vector ProcessSuffixes(absl::string_view config); + + std::set violations_; + + std::vector valid_input_suffixes = + ProcessSuffixes(kDefaultInputSuffixes); + std::vector valid_output_suffixes = + ProcessSuffixes(kDefaultOutputSuffixes); + + // Waive `if` branches we do not want to take into account. e.g: + // if(!rst_ni) + // data_q <= SOME_DEFAULT_VALUE; + // Exact matching with respect to the waive conditions is required (the only + // exception are leading and trailing whitespaces which are removed) + std::vector waive_ifs_with_conditions = + absl::StrSplit(kDefaultWaiveConditions, ','); + + // Regex to waive specific variables. Intended for (but not limited to) + // things like memories + // mem[addr] <= value; + std::unique_ptr waive_lhs_regex = + std::make_unique(kDefaultWaiveRegex); + + // (*) Valid integers span from 1 to n + static constexpr uint64_t kFirstValidPipeStage = 1; +}; + +} // namespace analysis +} // namespace verilog + +#endif // VERIBLE_VERILOG_ANALYSIS_CHECKERS_NAMING_CONVENTIONS_H_ diff --git a/verilog/analysis/checkers/dff_name_style_rule_test.cc b/verilog/analysis/checkers/dff_name_style_rule_test.cc new file mode 100644 index 000000000..5df9b6971 --- /dev/null +++ b/verilog/analysis/checkers/dff_name_style_rule_test.cc @@ -0,0 +1,316 @@ +// Copyright 2017-2023 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/dff_name_style_rule.h" + +#include +#include +#include +#include + +#include "absl/log/check.h" +#include "absl/strings/string_view.h" +#include "common/analysis/linter_test_utils.h" +#include "common/analysis/syntax_tree_linter_test_utils.h" +#include "gtest/gtest.h" +#include "verilog/analysis/verilog_analyzer.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { +namespace { + +using verible::LintTestCase; +using verible::RunConfiguredLintTestCases; +using verible::RunLintTestCases; + +// Tests that DffNameStyleRule correctly accepts valid names. +TEST(DffNameStyleRuleTest, AcceptDefaults) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) a_r <= a_next; endmodule"}, + {"module m; always_ff @(posedge c) a_r1 <= a_next; endmodule"}, + {"module m; always_ff @(posedge c) a_ff <= a_n; endmodule"}, + {"module m; always_ff @(posedge c) a_q <= a_d; endmodule"}, + {"module m; always_ff @(posedge c) if(!rst_ni) begin a_reg <= 0; end; " + "endmodule"}, + }; + RunLintTestCases(kTestCases); +} + +TEST(DffNameStyleRuleTest, RejectDefaultSuffixes) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) begin ", + {SymbolIdentifier, "a_n"}, + " <= b_n; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_q <= ", + {SymbolIdentifier, "b_q"}, + "; end endmodule"}, + // Should have same base ('a' != 'b') + {"module m; always_ff @(posedge c) begin ", + {SymbolIdentifier, "a_q"}, + " <= b_n; end endmodule"}, + }; + RunLintTestCases(kTestCases); +} + +TEST(DffNameStyleRuleTest, AcceptTests) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) a_ff <= a_next; endmodule"}, + {"module m; always_ff @(posedge c) someverylongname_ff <= " + "someverylongname_next; endmodule"}, + {"module m; always_ff @(posedge c) if(!rst_ni) begin a_ff <= 0; end; " + "endmodule"}, + {"module m; always_ff @(posedge c) if(!rst_ni) begin aAaAaAaAaAa_ff <= " + "0; end endmodule"}, + {"module m; always_ff @(posedge c) a_a_a_a_a_a_ff <= a_a_a_a_a_a_next; " + "endmodule"}, + }; + RunConfiguredLintTestCases(kTestCases, + "output:ff;" + "input:next"); +} + +TEST(DffNameStyleRuleTest, RejectTests) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) a_ff <= ", + {TK_DecNumber, "1"}, + "; endmodule"}, + {"module m; always_ff @(posedge c) a_ff <= ", + {TK_LP, "'{"}, + "default: '0}; endmodule"}, + // '(' token value is its ASCII value (40 == '(') + {"module m; always_ff @(posedge c) a_ff <= ", + {'(', "("}, + "1 + 2); endmodule"}, + }; + RunConfiguredLintTestCases(kTestCases, + "output:ff"); +} + +TEST(DffNameStyleRuleTest, AcceptPipelineStages) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) a_ff2 <= a_ff; endmodule"}, + {"module m; always_ff @(posedge c) a_ff2 <= a_ff1; endmodule"}, + {"module m; always_ff @(posedge c) a_ff3 <= a_ff2; endmodule"}, + {"module m; always_ff @(posedge c) a_ff4 <= a_ff3; endmodule"}, + {"module m; always_ff @(posedge c) a_ff40 <= a_ff39; endmodule"}, + {"module m; always_ff @(posedge c) if(!rst_ni) begin a_ff2 <= 0; end " + "endmodule"}, + {"module m; always_ff @(posedge c) if(!rst_ni) begin a_ff3 <= 0; end " + "endmodule"}, + }; + RunConfiguredLintTestCases(kTestCases, + "output:ff;" + "input:next"); +} + +TEST(DffNameStyleRuleTest, RejectPipelineStages) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) begin a_q1 <= ", + {SymbolIdentifier, "a_q0"}, + "; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_q2", + " <= ", + {SymbolIdentifier, "a_q0"}, + "; end endmodule"}, + {"module m; always_ff @(posedge c) begin " + "a_q3 <= ", + {SymbolIdentifier, "a_q5"}, + "; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_q5 <= ", + {SymbolIdentifier, "a_q2"}, + "; end endmodule"}, + {"module m; always_ff @(posedge c) begin " + "a_q2 <= ", + {SymbolIdentifier, "a_n"}, + "; end endmodule"}, + {"module m; always_ff @(posedge c) begin " + "a_q3 <= ", + {SymbolIdentifier, "a_n2"}, + "; end endmodule"}, + {"module m; always_ff @(posedge c) begin " + "a_q2 <= ", + {SymbolIdentifier, "a_ff"}, + "; end endmodule"}, + }; + RunConfiguredLintTestCases( + kTestCases, "output:q,ff;input:n"); +} + +TEST(DffNameStyleRuleTest, AcceptTestsNoSuffixes) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) a <= b; endmodule"}, + {"module m; always_ff @(posedge c) a_ff <= b_n; endmodule"}, + {"module m; always_ff @(posedge c) a_myverylongsuffix <= b_freedom; " + "endmodule"}, + }; + RunConfiguredLintTestCases(kTestCases, + "output:;" + "input:"); +} + +TEST(DffNameStyleRuleTest, Reject) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) begin ", + {SymbolIdentifier, "a"}, + " <= b_n; end endmodule"}, + {"module m; always_ff @(posedge c) begin ", + {MacroIdentifier, "`A_q"}, + " <= b_n; end endmodule"}, + {"module m; always_ff @(posedge c) begin ", + {MacroIdentifier, "`A"}, + " <= b_n; end endmodule"}, + {"module m; always_ff @(posedge c) begin ", + {SymbolIdentifier, "mystruct"}, + ".a_q <= b_n; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_ff <= ", + {SymbolIdentifier, "b"}, + "; end endmodule"}, + {"module m; always_ff @(posedge c) begin ", + {SymbolIdentifier, "a1_ff"}, + " <= a0_n; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_ff <= ", + {SymbolIdentifier, "mystruct"}, + ".b_n; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_ff <= ", + {SymbolIdentifier, "mystruct"}, + ".b; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_ff <= ", + {SymbolIdentifier, "b_n"}, + "[2]; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_ff <= ", + {SymbolIdentifier, "b"}, + "[2]; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_ff <= ", + {SymbolIdentifier, "b_n"}, + "(); end endmodule"}, + {"module m; always_ff @(posedge c) begin a_ff <= ", + {SymbolIdentifier, "b"}, + "(); end endmodule"}, + {"module m; always_ff @(posedge c) begin a_ff <= ", + {MacroIdentifier, "`DEF"}, + "; end endmodule"}, + {"module m; always_ff @(posedge c) begin a_ff <= ", + {MacroIdentifier, "`DEF_n"}, + "; end endmodule"}, + // Equal base + {"module m; always_ff @(posedge c) begin ", + {SymbolIdentifier, "a_q"}, + " <= b_n; end endmodule"}, + }; + RunConfiguredLintTestCases( + kTestCases, "output:ff,q;input:n"); +} + +TEST(DffNameStyleRuleTest, ExtractPipelineStage) { + struct Test { + absl::string_view str; + std::pair> expected; + }; + const std::vector kTestCases = { + {"data_q0", {"data_q0", {}}}, {"data_q1", {"data_q", {1}}}, + {"data_q2", {"data_q", {2}}}, {"data_q20", {"data_q", {20}}}, + {"data_q0", {"data_q0", {}}}, {"a", {"a", {}}}, + {"data", {"data", {}}}}; + + for (auto &test : kTestCases) { + auto [result_str, result_int] = + DffNameStyleRule::ExtractPipelineStage(test.str); + CHECK_EQ(test.expected.first, result_str); + CHECK_EQ(test.expected.second == result_int, true); + } +} + +TEST(DffNameStyleRuleTest, ExtraChecks) { + const std::initializer_list kTestCases = { + {"wire ", {SymbolIdentifier, "data_ff"}, " = 1;"}, + {"module m; assign ", {SymbolIdentifier, "data_ff"}, " = 1; endmodule"}, + {"module m;\nalways_comb begin\n", + {SymbolIdentifier, "data_ff"}, + " = 1;\n", + {SymbolIdentifier, "data_ff"}, + "++;\n", + "++", + {SymbolIdentifier, "data_ff"}, + ";\n", + {SymbolIdentifier, "data_ff"}, + " &= 1;\n", + "end\nendmodule"}, + {"reg data_n = 1;"}, + }; + RunLintTestCases(kTestCases); +} + +TEST(DffNameStyleRuleTest, WaiveRegexAccept) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) mem <= 1; endmodule"}, + {"module m; always_ff @(posedge c) mem[addr] <= val; endmodule"}, + {"module m; always_ff @(posedge c) mem[addr][other] <= val; endmodule"}, + {"module m; always_ff @(posedge c) if(something) begin mem[addr] <= 0; " + "end; endmodule"}, + }; + RunLintTestCases(kTestCases); +} + +TEST(DffNameStyleRuleTest, WaiveIfBranches) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) if(!rst_ni) begin a_reg <= 0; end; " + "endmodule "}, + {"module m; always_ff @(posedge c) if( flush_i ) begin a_reg <= 0; end; " + "endmodule "}, + {"module m; always_ff @(posedge c) if(!rst_ni || flush_i) begin a_reg <= " + "0; end; " + "endmodule "}, + {"module m; always_ff @(posedge c) if(flush_i || !rst_ni) begin a_reg <= " + "0; end; " + "endmodule "}, + {"module m; always_ff @(posedge c) if(reset) begin a_reg <= ", + {TK_DecNumber, "0"}, + "; end; endmodule"}, + {"module m; always_ff @(posedge c) if ( reset ) begin a_reg <= ", + {TK_DecNumber, "0"}, + "; end; endmodule"}, + }; + RunLintTestCases(kTestCases); +} + +TEST(DffNameStyleRuleTest, WaiveIfBranchesConfig) { + const std::initializer_list kTestCases = { + {"module m; always_ff @(posedge c) if(!rst_ni) begin a_reg <= ", + {TK_DecNumber, "0"}, + "; end; " + "endmodule "}, + {"module m; always_ff @(posedge c) if( flush_i ) begin a_reg <= ", + {TK_DecNumber, "0"}, + "; end; " + "endmodule "}, + {"module m; always_ff @(posedge c) if(!rst_ni || flush_i) begin a_reg " + "<= ", + {TK_DecNumber, "0"}, + "; end; " + "endmodule "}, + {"module m; always_ff @(posedge c) if(flush_i || !rst_ni) begin a_reg <= " + "0; end; " + "endmodule "}, + {"module m; always_ff @(posedge c) if(reset) begin a_reg <= ", + {TK_DecNumber, "0"}, + "; end; endmodule"}, + }; + RunConfiguredLintTestCases( + kTestCases, "waive_ifs_with_conditions: flush_i || !rst_ni "); +} + +} // namespace +} // namespace analysis +} // namespace verilog diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index 1e9e22e8e..307c27060 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -58,6 +58,7 @@ _linter_test_configs = [ ("always-ff-non-blocking", "always_ff_non_blocking", True), ("case-missing-default", "case_missing_default", True), ("constraint-name-style", "constraint_name_style", True), + ("dff-name-style", "dff_name_style", False), ("disable-statement", "disable_statement", False), ("endif-comment", "endif_comment", False), ("enum-name-style", "enum_name_style", True), @@ -69,6 +70,7 @@ _linter_test_configs = [ ("explicit-task-lifetime", "explicit_task_lifetime", True), ("forbid-consecutive-null-statements", "forbid_consecutive_null_statements", True), ("forbid-line-continuations", "forbid_line_continuations", True), + ("forbid-negative-array-dim", "forbid_negative_array_dim", False), ("generate-label", "generate_label_module", True), ("generate-label", "generate-label-module-body", True), # uses parse directive ("generate-label-prefix", "generate_label_prefix", True), diff --git a/verilog/tools/lint/testdata/dff_name_style.sv b/verilog/tools/lint/testdata/dff_name_style.sv new file mode 100644 index 000000000..55b07ef75 --- /dev/null +++ b/verilog/tools/lint/testdata/dff_name_style.sv @@ -0,0 +1,5 @@ +module dff_name_style (); + always_ff @(posedge clk) begin + data_q <= data; + end +endmodule diff --git a/verilog/tools/lint/testdata/forbid_negative_array_dim.sv b/verilog/tools/lint/testdata/forbid_negative_array_dim.sv new file mode 100644 index 000000000..7a4769eb9 --- /dev/null +++ b/verilog/tools/lint/testdata/forbid_negative_array_dim.sv @@ -0,0 +1,3 @@ +module forbid_negative_array_dim (); + reg [-1 : 0] x; +endmodule