diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule.cc b/verilog/analysis/checkers/always_ff_non_blocking_rule.cc index dddf214319..7191ce81e7 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule.cc +++ b/verilog/analysis/checkers/always_ff_non_blocking_rule.cc @@ -17,6 +17,7 @@ #include #include +#include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" @@ -105,11 +106,13 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, // Rule may be waived if complete lhs consists of local variables // -> determine root of lhs const verible::Symbol *check_root = nullptr; + absl::string_view lhs_id; verible::matcher::BoundSymbolManager symbol_man; if (asgn_blocking_matcher.Matches(symbol, &symbol_man)) { const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol); check_root = verilog::GetNetVariableAssignmentLhs(*node); + lhs_id = verible::StringSpanOfSymbol(*check_root); const verible::SyntaxTreeLeaf *leaf = verible::GetSubtreeAsLeaf(symbol, NodeEnum::kNetVariableAssignment, 1); @@ -129,14 +132,16 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, const verible::SyntaxTreeNode &rhs = *GetAssignModifyRhs(*node); const verible::SyntaxTreeLeaf &operator_ = *GetAssignModifyOperator(*node); - check_root = &lhs; - const std::string fix = - absl::StrCat(verible::StringSpanOfSymbol(lhs), - " <= ", verible::StringSpanOfSymbol(lhs), " ", - verible::StringSpanOfSymbol(operator_).substr(0, 1), " ", - verible::StringSpanOfSymbol(rhs), ";"); + bool needs_parenthesis = NeedsParenthesis(rhs); + + lhs_id = verible::StringSpanOfSymbol(lhs); + const std::string fix = absl::StrCat( + lhs_id, " <= ", lhs_id, " ", + verible::StringSpanOfSymbol(operator_).substr(0, 1), " ", + needs_parenthesis ? "(" : "", verible::StringSpanOfSymbol(rhs), + needs_parenthesis ? ")" : "", ";"); autofixes.emplace_back( AutoFix("Substitute assignment operator for equivalent " @@ -149,8 +154,7 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, GetIncrementDecrementOperator(symbol); check_root = operand; - const absl::string_view identifier = - verible::StringSpanOfSymbol(*operand); + lhs_id = verible::StringSpanOfSymbol(*operand); // Extract just the operation. Just '+' from '++' const absl::string_view op = verible::StringSpanOfSymbol(*operator_).substr(0, 1); @@ -158,8 +162,8 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, // Equivalent nonblocking assignment // {'x++', '++x'} become 'x <= x + 1' // {'x--', '--x'} become 'x <= x - 1' - std::string fix = - absl::StrCat(identifier, " <= ", identifier, " ", op, " 1;"); + const std::string fix = + absl::StrCat(lhs_id, " <= ", lhs_id, " ", op, " 1;"); autofixes.emplace_back( AutoFix("Substitute increment/decrement operator for " @@ -198,8 +202,12 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, } // Enqueue detected violation unless waived - if (!waived) { + if (waived) return; + + if (IsAutoFixSafe(symbol, lhs_id)) { violations_.insert(LintViolation(symbol, kMessage, context, autofixes)); + } else { + violations_.insert(LintViolation(symbol, kMessage, context)); } } // HandleSymbol() @@ -225,6 +233,7 @@ bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol &symbol, if (always_ff_matcher.Matches(symbol, &symbol_man)) { VLOG(4) << "always_ff @DEPTH=" << depth << std::endl; inside_ = depth; + CollectLocalReferences(symbol); } return false; } @@ -267,5 +276,94 @@ bool AlwaysFFNonBlockingRule::LocalDeclaration(const verible::Symbol &symbol) { return false; } // LocalDeclaration() +bool AlwaysFFNonBlockingRule::NeedsParenthesis( + const verible::Symbol &rhs) const { + // Avoid inserting parenthesis for simple expressions + // For example: x &= 1 -> x <= x & 1, and not x <= x & (1) + // This could be more precise, but checking every specific + // case where parenthesis are needed is hard. Adding them + // doesn't hurt and the user can remove them if needed. + bool complex_rhs_expr = + verible::GetLeftmostLeaf(rhs) != verible::GetRightmostLeaf(rhs); + + // Check whether the rhs expression is already inside parenthesis + // Avoid searching for kParenGroups, this should be safe and faster + return complex_rhs_expr && + !absl::StrContains(verible::StringSpanOfSymbol(rhs), '('); +} + +void AlwaysFFNonBlockingRule::CollectLocalReferences( + const verible::Symbol &root) { + static const Matcher reference_matcher{NodekReference()}; + + const std::vector references_ = + verible::SearchSyntaxTree(root, reference_matcher); + + // Precompute the StringSpan of every identifier referenced to. + // Avoid recomputing it several times + references.resize(references_.size()); + std::transform(references_.cbegin(), references_.cend(), references.begin(), + [](const verible::TreeSearchMatch &match) { + return ReferenceWithId{ + match, verible::StringSpanOfSymbol(*match.match)}; + }); +} + +bool AlwaysFFNonBlockingRule::IsAutoFixSafe( + const verible::Symbol &faulting_assignment, + absl::string_view lhs_id) const { + std::vector::const_iterator itr = references.end(); + + // Let's assume that 'x' is the variable affected by the faulting + // assignment. In order to ensure that the autofix is safe we have + // to ensure that there is no later reference to 'x' + // + // Can't autofix Can autofix + // begin begin + // x = x + 1; x = x + 1; + // y = x; y <= y + 1; + // end end + // + // In practical terms: we'll scan the 'references' vector for kReferences + // to 'x' that appear after the faulting assignment in the always_ff block + + static const Matcher reference_matcher{NodekReference()}; + + // Extract kReferences inside the faulting expression + const std::vector references_ = + verible::SearchSyntaxTree(faulting_assignment, reference_matcher); + + // ref points to the latest reference to 'x' in our faulting expression + // 'x++', 'x = x + 1', 'x &= x + 1' + // ^ ^ ^ + // We need this to know from where to start searching for later references + // to 'x', and decide whether the AutoFix is safe or not + const verible::Symbol *ref = + std::find_if(std::rbegin(references_), std::rend(references_), + [&](const verible::TreeSearchMatch &m) { + return verible::StringSpanOfSymbol(*m.match) == lhs_id; + }) + ->match; + + // Shouldn't happen, sanity check to avoid crashes + if (!ref) return false; + + // Find the last reference to 'x' in the faulting assignment + itr = std::find_if( + std::begin(references), std::end(references), + [&](const ReferenceWithId &r) { return r.match.match == ref; }); + + // Search from the last reference to 'x' onwards. If there is any, + // we can't apply the autofix safely + itr = std::find_if( + std::next(itr), std::end(references), + [&](const ReferenceWithId &ref) { return ref.id == lhs_id; }); + + // Let's say 'x' is affected by a flagged operation { 'x = 1', 'x++', ... } + // We can safely autofix if after the flagged operation there are no + // more references to 'x' + return references.end() == itr; +} + } // namespace analysis } // namespace verilog diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule.h b/verilog/analysis/checkers/always_ff_non_blocking_rule.h index e2d7bcd56d..05d4a78600 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule.h +++ b/verilog/analysis/checkers/always_ff_non_blocking_rule.h @@ -1,4 +1,4 @@ -// Copyright 2017-2020 The Verible Authors. +// 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. @@ -15,12 +15,12 @@ #ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_ALWAYS_FF_NON_BLOCKING_RULE_H_ #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_ALWAYS_FF_NON_BLOCKING_RULE_H_ -#include #include #include #include "common/analysis/lint_rule_status.h" #include "common/analysis/syntax_tree_lint_rule.h" +#include "common/analysis/syntax_tree_search.h" #include "common/text/symbol.h" #include "common/text/syntax_tree_context.h" #include "verilog/analysis/descriptions.h" @@ -32,19 +32,29 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule { public: using rule_type = verible::SyntaxTreeLintRule; - static const LintRuleDescriptor& GetDescriptor(); + static const LintRuleDescriptor &GetDescriptor(); absl::Status Configure(absl::string_view configuration) final; - void HandleSymbol(const verible::Symbol& symbol, - const verible::SyntaxTreeContext& context) final; + void HandleSymbol(const verible::Symbol &symbol, + const verible::SyntaxTreeContext &context) final; verible::LintRuleStatus Report() const final; private: // Detects entering and leaving relevant code inside always_ff - bool InsideBlock(const verible::Symbol& symbol, int depth); + bool InsideBlock(const verible::Symbol &symbol, int depth); // Processes local declarations - bool LocalDeclaration(const verible::Symbol& symbol); + bool LocalDeclaration(const verible::Symbol &symbol); + // Check the need of parenthesis in certain autofixes + // Might have false positives. Example: + // Fixing 'x *= y + 1' requires adding parenthesis + // 'x <= x & (y + 1)' + bool NeedsParenthesis(const verible::Symbol &rhs) const; + // Collect local references inside an always_ff block + void CollectLocalReferences(const verible::Symbol &root); + // Check if is safe to autofix + bool IsAutoFixSafe(const verible::Symbol &faulting_assignment, + absl::string_view lhs_id) const; private: // Collected violations. @@ -69,6 +79,16 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule { // In-order stack of local variable names std::vector locals_; + + // NodekReference + string representation + // of the ID they're referring to + struct ReferenceWithId { + verible::TreeSearchMatch match; + absl::string_view id; + }; + // Collection of references to variables + // assumed to be in program order + std::vector references; }; } // namespace analysis diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc b/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc index 0b65864800..094e530da8 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc +++ b/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc @@ -140,15 +140,11 @@ TEST(AlwaysFFNonBlockingRule, LocalWaiving) { TEST(AlwaysFFNonBlockingRule, AutoFixDefault) { const std::initializer_list kTestCases = { // Check we're not waiving local variables - {"module m;\nalways_ff begin reg k; k = 1; end\nendmodule", - "module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"}, {"module m;\nalways_ff begin reg k; k = 1; end\nendmodule", "module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"}, // Check we're ignoring modifying assignments {"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule", "module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"}, - {"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule", - "module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"}, }; RunApplyFixCases(kTestCases, ""); @@ -158,8 +154,40 @@ TEST(AlwaysFFNonBlockingRule, AutoFixCatchModifyingAssignments) { const std::initializer_list kTestCases = { {"module m;\nalways_ff begin k &= 1; end\nendmodule", "module m;\nalways_ff begin k <= k & 1; end\nendmodule"}, + {"module m;\nalways_ff begin k &= a; end\nendmodule", + "module m;\nalways_ff begin k <= k & a; end\nendmodule"}, {"module m;\nalways_ff begin k |= (2 + 1); end\nendmodule", "module m;\nalways_ff begin k <= k | (2 + 1); end\nendmodule"}, + {"module m;\nalways_ff begin k *= 2 + 1; end\nendmodule", + "module m;\nalways_ff begin k <= k * (2 + 1); end\nendmodule"}, + {"module m;\nalways_ff begin a++; end\nendmodule", + "module m;\nalways_ff begin a <= a + 1; end\nendmodule"}, + {"module m;\nalways_ff begin ++a; end\nendmodule", + "module m;\nalways_ff begin a <= a + 1; end\nendmodule"}, + }; + + RunApplyFixCases( + kTestCases, "catch_modifying_assignments:on"); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixDontBreakCode) { + const std::initializer_list kTestCases = { + // We can't fix 'k &= 1', because it would affect + // 'p <= k' + {"module m;\nalways_ff begin\n" + "k &= 1;\n" + "p <= k;\n" + "a++;" + "end\nendmodule", + "module m;\nalways_ff begin\n" + "k &= 1;\n" + "p <= k;\n" + "a <= a + 1;" + "end\nendmodule"}, + // Correctly fix despide there is a reference to 'k' in the rhs + {"module m;\nalways_ff begin k = k + 1; end\nendmodule", + "module m;\nalways_ff begin k <= k + 1; end\nendmodule"}, + // TODO: Add more tests }; RunApplyFixCases(