Merge origin/master into fix/2547 Resolve verilog-equivalence.cc by keeping TokensAreWhitespaceDependent FormatEquivalent (covers MacroCallCloseToEndLine and line-ending macros). Resolve formatter_test.cc by keeping #2547, #2544, and #2542 regressions.
diff --git a/.github/bin/run-format.sh b/.github/bin/run-format.sh index 3c1c724..d5ccfa1 100755 --- a/.github/bin/run-format.sh +++ b/.github/bin/run-format.sh
@@ -31,10 +31,16 @@ FORMAT_OUT=${TMPDIR:-/tmp}/clang-format-diff.out -# Use the provided Clang format binary, or try to fallback to clang-format-17 -# or clang-format. +# Use the provided Clang format binary, or try clang-format-19 (CI version), +# then older numbered versions, then unversioned clang-format. if [[ ! -v CLANG_FORMAT ]]; then - if command -v "clang-format-17" 2>&1 >/dev/null + if command -v "clang-format-19" 2>&1 >/dev/null + then + CLANG_FORMAT="clang-format-19" + elif command -v "clang-format-18" 2>&1 >/dev/null + then + CLANG_FORMAT="clang-format-18" + elif command -v "clang-format-17" 2>&1 >/dev/null then CLANG_FORMAT="clang-format-17" elif command -v "clang-format" 2>&1 >/dev/null @@ -44,6 +50,7 @@ (echo "-- Missing the clang-format binary! --"; exit 1) fi fi +echo "Using ${CLANG_FORMAT} ($("${CLANG_FORMAT}" --version))" BUILDIFIER=${BUILDIFIER:-buildifier}
diff --git a/.github/bin/smoke-projects.hashes b/.github/bin/smoke-projects.hashes new file mode 100644 index 0000000..5298441 --- /dev/null +++ b/.github/bin/smoke-projects.hashes
@@ -0,0 +1,20 @@ +a12778b40ae905f82eb2c23ea4ac037f98099fae https://github.com/jamieiles/80x86 +4da15979747f326bde2f9869c64e587ce599772c https://github.com/pulp-platform/axi +b48037e28544425839dbd617d45b1a82631bc1a9 https://github.com/bespoke-silicon-group/basejump_stl +f91010f654a5dfd00f83dbe25dbda482218d540b https://github.com/black-parrot/black-parrot +86d09682492176a1f8fd1732e469bc2d309453a3 https://github.com/chipsalliance/caliptra-rtl +f9fbd09655ec6ee457438140c13a168390ebd043 https://github.com/chipsalliance/Cores-VeeR-EH2 +3ac6208ed0210ff66a557dd7dc4e79e34f1f2f4a https://github.com/openhwgroup/cva6 +0602ee4627b10f301298f2673d826cdd6baa9327 https://github.com/ijor/fx68k +8b8ee086aef72e0833b7f0493d9d33f1e4d3c8e2 https://github.com/lowRISC/ibex +a19e629a1879801ffcc6f2e6256ca435c20570f3 https://github.com/steveicarus/ivtest +8e83643c22a3ba7612a9bb9cec93292dad618ab5 https://github.com/trivialmips/nontrivial-mips +00981352642fb65ef2b6c47143a0f6bb31fd4a97 https://github.com/lowRISC/opentitan +491a4dc9bb25c33e0183caaf683102b9ce273bc1 https://github.com/gtaylormb/opl3_fpga +7b65f6ba0bce58d4d859082660123b7100aae975 https://github.com/rsd-devel/rsd +ebb5e3551a9d93c0ee95f0b767dd878b8927e702 https://github.com/syntacore/scr1 +f200eb2ed7b69ac1c6b8eddd47654522aeee5ce8 https://github.com/olofk/serv +c4229f3bd5220e6d3ba8f390e5d09c87e462e9c7 https://github.com/chipsalliance/sv-tests +7e9841ff775ea22fba3f222f7bdfe35668345e00 https://github.com/taichi-ishitani/tnoc +5d72b6618acfddece7f09382a032ccbc05862fdc https://github.com/SymbiFlow/uvm +1c8e05fd1e9a79ceb8b996a0996674122eed086f https://github.com/SymbiFlow/XilinxUnisimLibrary
diff --git a/.github/bin/smoke-test.sh b/.github/bin/smoke-test.sh index ba3ff96..5532187 100755 --- a/.github/bin/smoke-test.sh +++ b/.github/bin/smoke-test.sh
@@ -29,16 +29,13 @@ # crash Verible, we're good. ### -# Suppress '... aborted' messages bash would print when a tool crashes. -# Comment out to see syntax errors in bash while working on script. -exec 2>/dev/null - set -u # Be strict: only allow using a variable after it is assigned BAZEL_BUILD_OPTIONS="-c opt" TMPDIR="${TMPDIR:-/tmp}" readonly BASE_TEST_DIR="${TMPDIR}/test/verible-smoke-test" +readonly PROJECT_HASHES_FILE="$(dirname $0)/smoke-projects.hashes" # Write log files to this directory readonly SMOKE_LOGGING_DIR="${SMOKE_LOGGING_DIR:-$BASE_TEST_DIR/error-logs}" @@ -81,29 +78,11 @@ # # There are some known issues which are all recorded in the associative # array below, mapping them to Verible issue tracker numbers. -# TODO(hzeller): there should be a configuration file that contains two -# columns: URL + hash, so that we can fetch a particular known version not -# a moving target. -readonly TEST_GIT_PROJECTS="https://github.com/lowRISC/ibex \ - https://github.com/lowRISC/opentitan \ - https://github.com/chipsalliance/sv-tests \ - https://github.com/chipsalliance/Cores-VeeR-EH2 \ - https://github.com/chipsalliance/caliptra-rtl \ - https://github.com/openhwgroup/cva6 \ - https://github.com/SymbiFlow/uvm \ - https://github.com/taichi-ishitani/tnoc \ - https://github.com/ijor/fx68k \ - https://github.com/jamieiles/80x86 \ - https://github.com/SymbiFlow/XilinxUnisimLibrary \ - https://github.com/black-parrot/black-parrot - https://github.com/steveicarus/ivtest \ - https://github.com/trivialmips/nontrivial-mips \ - https://github.com/pulp-platform/axi \ - https://github.com/rsd-devel/rsd \ - https://github.com/syntacore/scr1 \ - https://github.com/olofk/serv \ - https://github.com/bespoke-silicon-group/basejump_stl \ - https://github.com/gtaylormb/opl3_fpga" + +if [ ! -f "${PROJECT_HASHES_FILE}" ]; then + echo "Project hashes file not found: ${PROJECT_HASHES_FILE}" + exit 1 +fi ## # Some of the files in the projects will have issues. @@ -135,24 +114,24 @@ ExpectedFailCount[syntax:ibex]=13 ExpectedFailCount[lint:ibex]=13 -ExpectedFailCount[project:ibex]=218 -ExpectedFailCount[preprocessor:ibex]=397 +ExpectedFailCount[project:ibex]=224 +ExpectedFailCount[preprocessor:ibex]=403 ExpectedFailCount[syntax:opentitan]=88 ExpectedFailCount[lint:opentitan]=88 -ExpectedFailCount[project:opentitan]=1068 +ExpectedFailCount[project:opentitan]=1077 ExpectedFailCount[formatter:opentitan]=0 -ExpectedFailCount[preprocessor:opentitan]=3074 +ExpectedFailCount[preprocessor:opentitan]=3017 ExpectedFailCount[syntax:sv-tests]=74 ExpectedFailCount[lint:sv-tests]=73 ExpectedFailCount[project:sv-tests]=176 -ExpectedFailCount[preprocessor:sv-tests]=128 +ExpectedFailCount[preprocessor:sv-tests]=129 -ExpectedFailCount[syntax:caliptra-rtl]=39 -ExpectedFailCount[lint:caliptra-rtl]=38 -ExpectedFailCount[project:caliptra-rtl]=453 -ExpectedFailCount[preprocessor:caliptra-rtl]=911 +ExpectedFailCount[syntax:caliptra-rtl]=41 +ExpectedFailCount[lint:caliptra-rtl]=40 +ExpectedFailCount[project:caliptra-rtl]=474 +ExpectedFailCount[preprocessor:caliptra-rtl]=993 ExpectedFailCount[syntax:Cores-VeeR-EH2]=2 ExpectedFailCount[lint:Cores-VeeR-EH2]=2 @@ -164,10 +143,10 @@ ExpectedFailCount[project:cva6]=84 ExpectedFailCount[preprocessor:cva6]=140 -ExpectedFailCount[syntax:uvm]=0 -ExpectedFailCount[lint:uvm]=0 -ExpectedFailCount[project:uvm]=40 -ExpectedFailCount[preprocessor:uvm]=126 +ExpectedFailCount[syntax:uvm]=1 +ExpectedFailCount[lint:uvm]=1 +ExpectedFailCount[project:uvm]=41 +ExpectedFailCount[preprocessor:uvm]=128 ExpectedFailCount[syntax:tnoc]=3 ExpectedFailCount[lint:tnoc]=3 @@ -187,9 +166,9 @@ ExpectedFailCount[project:black-parrot]=172 ExpectedFailCount[preprocessor:black-parrot]=173 -ExpectedFailCount[syntax:ivtest]=117 -ExpectedFailCount[lint:ivtest]=117 -ExpectedFailCount[project:ivtest]=146 +ExpectedFailCount[syntax:ivtest]=116 +ExpectedFailCount[lint:ivtest]=116 +ExpectedFailCount[project:ivtest]=145 ExpectedFailCount[preprocessor:ivtest]=26 ExpectedFailCount[syntax:nontrivial-mips]=2 @@ -237,11 +216,10 @@ expected_count_key="${TOOL_SHORT_NAME}:${PROJECT_NAME}" if [[ -v ExpectedFailCount[${expected_count_key}] ]]; then expected_count=${ExpectedFailCount[${expected_count_key}]} - # We allow some 5% deviation from expected values before we complain loudly - local GRACE_VALUE=$(( ${expected_count} / 20 )) - if [ ${GRACE_VALUE} -lt 2 ]; then - GRACE_VALUE=2 - fi + + # Since we have a fixed set of projects and hashes, we don't allow any + # derivation from the expected value (used to be 5%) + local GRACE_VALUE=0 if [ ${OBSERVED_NONZERO_COUNT} -gt ${expected_count} ] ; then local ALLOWED_UP_TO=$((${expected_count} + ${GRACE_VALUE})) if [ ${OBSERVED_NONZERO_COUNT} -gt ${ALLOWED_UP_TO} ]; then @@ -254,6 +232,7 @@ elif [ ${OBSERVED_NONZERO_COUNT} -lt ${expected_count} ] ; then echo "::notice:: 🎉 Yay, reduced non-zero exit count ${expected_count} -> ${OBSERVED_NONZERO_COUNT}" echo "Set ExpectedFailCount[${TOOL_SHORT_NAME}:${PROJECT_NAME}]=${OBSERVED_NONZERO_COUNT}" + return 1 # we still want the exact value to be recorded so actively fail fi else if [ ${OBSERVED_NONZERO_COUNT} -gt 0 ] ; then @@ -270,11 +249,14 @@ # # First parameter : project name # Second parameter: name of file containing a list of {System}Verilog files +# Third parameter : git URL +# Fourth parameter: git hash (optional, defaults to master) function run_smoke_test() { local PROJECT_FILE_LIST=${TMPDIR}/filelist.$$.list local PROJECT_NAME=$1 local FILELIST=$2 local GIT_URL=$3 + local GIT_HASH=${4:-master} local NUM_FILES=$(wc -l < ${FILELIST}) local result=0 @@ -355,7 +337,7 @@ else # This is an so far unknown issue echo "::error:: 😱 ${single_file}: crash exit code $EXIT_CODE for $tool" - echo "Input File URL: ${GIT_URL}/blob/master/$(echo $single_file | cut -d/ -f6-)" + echo "Input File URL: ${GIT_URL}/blob/${GIT_HASH}/$(echo $single_file | cut -d/ -f6-)" head -15 ${PROJECT_FILE_TOOL_OUT} # Might be useful in this case result=$((${result} + 1)) fi @@ -372,8 +354,55 @@ return ${result} } +# --- main + +KEEP_LOGS=0 +VERBOSE=0 +PROJECT_FILTER="" + +while [[ $# -gt 0 ]]; do + case "$1" in + --keep-logs) + KEEP_LOGS=1 + shift + ;; + --verbose|-v) + VERBOSE=1 + shift + ;; + --filter=*) + PROJECT_FILTER="${1#*=}" + shift + ;; + --filter|-f) + if [[ $# -lt 2 ]]; then + echo "Error: $1 requires an argument." >&2 + exit 1 + fi + PROJECT_FILTER="$2" + shift 2 + ;; + -h|--help) + echo "Usage: $0 [--keep-logs] [--verbose] [--filter=<name>]" + exit 0 + ;; + *) + echo "Unknown option: $1" >&2 + echo "Usage: $0 [--keep-logs] [--verbose] [--filter=<name>]" >&2 + exit 1 + ;; + esac +done + +if [ ${VERBOSE} -eq 0 ]; then + # Suppress '... aborted' messages bash would print when a tool crashes. + exec 2>/dev/null +fi + mkdir -p "${BASE_TEST_DIR}" -trap 'rm -rf -- "${BASE_TEST_DIR}"' EXIT +if [ ${KEEP_LOGS} -eq 0 ]; then + trap 'rm -rf -- "${BASE_TEST_DIR}"' EXIT +fi status_sum=0 @@ -388,18 +417,26 @@ bazel build ${BAZEL_BUILD_OPTIONS} :install-binaries & # While compiling, run potentially slow network ops -for git_project in ${TEST_GIT_PROJECTS} ; do - PROJECT_NAME="$(basename $git_project)" +while read -r git_hash git_project _; do + [[ -z "${git_hash}" || "${git_hash}" =~ ^# ]] && continue + PROJECT_NAME="$(basename "${git_project}")" + if [[ -n "${PROJECT_FILTER}" && "${PROJECT_NAME}" != *"${PROJECT_FILTER}"* ]]; then + continue + fi PROJECT_DIR="${BASE_TEST_DIR}/${PROJECT_NAME}" - git clone ${git_project} ${PROJECT_DIR} 2>/dev/null & -done + ( git clone "${git_project}" "${PROJECT_DIR}" && git -C "${PROJECT_DIR}" checkout -q "${git_hash}" ) 2>/dev/null & +done < "${PROJECT_HASHES_FILE}" echo "base test dir ${BASE_TEST_DIR}; writing logs to ${SMOKE_LOGGING_DIR}" echo "Waiting... for compilation and project download finished" wait -for git_project in ${TEST_GIT_PROJECTS} ; do - PROJECT_NAME="$(basename $git_project)" +while read -r git_hash git_project _; do + [[ -z "${git_hash}" || "${git_hash}" =~ ^# ]] && continue + PROJECT_NAME="$(basename "${git_project}")" + if [[ -n "${PROJECT_FILTER}" && "${PROJECT_NAME}" != *"${PROJECT_FILTER}"* ]]; then + continue + fi PROJECT_DIR="${BASE_TEST_DIR}/${PROJECT_NAME}" # Already cloned above @@ -412,17 +449,17 @@ FILELIST="${PROJECT_DIR}/verible.filelist" find "${PROJECT_DIR}" -name "*.sv" -o -name "*.svh" -o -name "*.v" | sort > ${FILELIST} - run_smoke_test "${PROJECT_NAME}" "${FILELIST}" "${git_project}" + run_smoke_test "${PROJECT_NAME}" "${FILELIST}" "${git_project}" "${git_hash}" status_sum=$((${status_sum} + $?)) echo -done +done < "${PROJECT_HASHES_FILE}" echo "::endgroup::" -echo "There were a total of ${status_sum} new, undocumented issues." +echo "There were a total of ${status_sum} mismatches" # Let's see if there are any issues that are fixed in the meantime. -if [ "${#KnownIssue[@]}" -ne 0 ]; then +if [ -z "${PROJECT_FILTER}" ] && [ "${#KnownIssue[@]}" -ne 0 ]; then echo "::warning ::There are ${#KnownIssue[@]} tool/file combinations, that no longer fail" declare -A DistinctIssues for key in "${!KnownIssue[@]}"; do @@ -434,7 +471,10 @@ for issue_id in "${!DistinctIssues[@]}"; do echo " 🐞 ${ISSUE_PREFIX}/${issue_id}" done - echo +fi + +if [ ${KEEP_LOGS} -ne 0 ]; then + echo "Logs and project files kept in ${BASE_TEST_DIR} (error logs: ${SMOKE_LOGGING_DIR})" fi exit ${status_sum}
diff --git a/.github/workflows/verible-ci.yml b/.github/workflows/verible-ci.yml index 5f03f2f..802a7a3 100644 --- a/.github/workflows/verible-ci.yml +++ b/.github/workflows/verible-ci.yml
@@ -34,6 +34,11 @@ with: fetch-depth: 0 + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: 'stable' + - name: Install Dependencies run: | sudo apt-get install clang-format-19
diff --git a/README.md b/README.md index b813b76..047fb20 100644 --- a/README.md +++ b/README.md
@@ -172,8 +172,9 @@ Verible's code base is written in C++. -To build, you need the [bazel] build system (Min version 7) and a C++20 -compatible compiler. +To build, you need the [bazel] build system (get it from +[bazel install][bazel-install] if not already on your system) and a +C++20 compatible compiler. Use your package manager to install the dependencies; on a system with the nix package manager simply run `nix-shell` to get a build environment. @@ -199,6 +200,8 @@ bazel build -c opt --config=create_static_linked_executables //... ``` +See [Installation](#installation-1) for install. + ### Optionally using local flex/bison for build Flex and Bison, that are needed for the parser generation, are compiled as part @@ -283,6 +286,7 @@ [UHDM] format. If you are interested in collaborating, contact us. [bazel]: https://bazel.build/ +[bazel-install]: https://bazel.build/install [SV-LRM]: https://ieeexplore.ieee.org/document/8299595 [lint-rule-list]: https://chipsalliance.github.io/verible/lint.html [github-lint-action]: https://github.com/chipsalliance/verible-linter-action
diff --git a/shell.nix b/shell.nix index 85163eb..408340b 100644 --- a/shell.nix +++ b/shell.nix
@@ -8,6 +8,17 @@ #verible_used_stdenv = pkgs.gcc15Stdenv; #verible_used_stdenv = pkgs.clang19Stdenv; bazel = pkgs.bazel_8; + + + userNixPath = ./user.nix; # optional user config + userPackages = + if builtins.pathExists userNixPath + then + let loaded = import userNixPath; + in if builtins.isFunction loaded + then loaded { inherit pkgs; } + else loaded + else []; in verible_used_stdenv.mkDerivation { name = "verible-build-environment"; @@ -40,7 +51,7 @@ llvmPackages_22.clang-tools # for clang-tidy llvmPackages_19.clang-tools # for clang-format - ]; + ] ++ userPackages; shellHook = '' # clang tidy: use latest. export CLANG_TIDY=${pkgs.llvmPackages_22.clang-tools}/bin/clang-tidy
diff --git a/verible/common/util/BUILD b/verible/common/util/BUILD index 48a2105..db499b4 100644 --- a/verible/common/util/BUILD +++ b/verible/common/util/BUILD
@@ -22,7 +22,6 @@ hdrs = ["auto-pop-stack.h"], deps = [ ":logging", - "@abseil-cpp//absl/base:core_headers", ], )
diff --git a/verible/common/util/auto-pop-stack.h b/verible/common/util/auto-pop-stack.h index 024ef05..58628eb 100644 --- a/verible/common/util/auto-pop-stack.h +++ b/verible/common/util/auto-pop-stack.h
@@ -18,7 +18,6 @@ #include <cstddef> #include <vector> -#include "absl/base/attributes.h" #include "verible/common/util/logging.h" namespace verible { @@ -29,7 +28,7 @@ // implementing algorithms based on stack data structures like building // inheritance stack of traversed tree. template <typename T> -class AutoPopStack { +class [[maybe_unused]] AutoPopStack { public: using value_type = T; using this_type = AutoPopStack<value_type>; @@ -105,7 +104,7 @@ private: stack_type stack_; -} ABSL_ATTRIBUTE_UNUSED; +}; } // namespace verible
diff --git a/verible/common/util/container-proxy_test.cc b/verible/common/util/container-proxy_test.cc index 5e8220f..4e4cd75 100644 --- a/verible/common/util/container-proxy_test.cc +++ b/verible/common/util/container-proxy_test.cc
@@ -874,13 +874,14 @@ const int new_capacity = initial_capacity + 42; this->proxy.reserve(new_capacity); - EXPECT_EQ(this->proxy.capacity(), new_capacity); - EXPECT_EQ(this->container.capacity(), new_capacity); + EXPECT_GE(this->proxy.capacity(), new_capacity); + EXPECT_EQ(this->proxy.capacity(), this->container.capacity()); + const int capacity_after_first_reserve = this->proxy.capacity(); const int lower_capacity = 1; this->proxy.reserve(lower_capacity); - EXPECT_EQ(this->proxy.capacity(), new_capacity); - EXPECT_EQ(this->container.capacity(), new_capacity); + EXPECT_EQ(this->proxy.capacity(), capacity_after_first_reserve); + EXPECT_EQ(this->container.capacity(), capacity_after_first_reserve); } } // namespace
diff --git a/verible/verilog/analysis/checkers/BUILD b/verible/verilog/analysis/checkers/BUILD index bd3a98b..4681607 100644 --- a/verible/verilog/analysis/checkers/BUILD +++ b/verible/verilog/analysis/checkers/BUILD
@@ -817,6 +817,7 @@ "//verible/common/analysis:syntax-tree-lint-rule", "//verible/common/analysis/matcher", "//verible/common/analysis/matcher:bound-symbol-manager", + "//verible/common/text:config-utils", "//verible/common/text:symbol", "//verible/common/text:syntax-tree-context", "//verible/common/text:token-info", @@ -826,7 +827,9 @@ "//verible/verilog/CST:verilog-nonterminals", "//verible/verilog/analysis:descriptions", "//verible/verilog/analysis:lint-rule-registry", + "@abseil-cpp//absl/status", "@abseil-cpp//absl/strings", + "@re2", ], alwayslink = 1, ) @@ -1821,6 +1824,7 @@ "//verible/common/text:symbol", "//verible/common/text:syntax-tree-context", "//verible/common/text:token-info", + "//verible/common/util:container-util", "//verible/verilog/CST:port", "//verible/verilog/CST:verilog-matchers", "//verible/verilog/analysis:descriptions",
diff --git a/verible/verilog/analysis/checkers/generate-label-prefix-rule.cc b/verible/verilog/analysis/checkers/generate-label-prefix-rule.cc index c8822ce..e678ad5 100644 --- a/verible/verilog/analysis/checkers/generate-label-prefix-rule.cc +++ b/verible/verilog/analysis/checkers/generate-label-prefix-rule.cc
@@ -14,12 +14,17 @@ #include "verible/verilog/analysis/checkers/generate-label-prefix-rule.h" +#include <memory> +#include <string> #include <string_view> -#include "absl/strings/match.h" +#include "absl/status/status.h" +#include "absl/strings/str_cat.h" +#include "re2/re2.h" #include "verible/common/analysis/lint-rule-status.h" #include "verible/common/analysis/matcher/bound-symbol-manager.h" #include "verible/common/analysis/matcher/matcher.h" +#include "verible/common/text/config-utils.h" #include "verible/common/text/symbol.h" #include "verible/common/text/syntax-tree-context.h" #include "verible/common/text/token-info.h" @@ -38,22 +43,36 @@ // Register the lint rule VERILOG_REGISTER_LINT_RULE(GenerateLabelPrefixRule); -static constexpr std::string_view kMessage = - "All generate block labels must start with g_ or gen_"; +static constexpr std::string_view kDefaultStyleRegex = "(g_|gen_).*"; -// TODO(fangism): and be lower_snake_case? -// TODO(fangism): generalize to a configurable pattern and -// rename this class/rule to GenerateLabelNamingStyle? +GenerateLabelPrefixRule::GenerateLabelPrefixRule() + : style_regex_( + std::make_unique<re2::RE2>(kDefaultStyleRegex, re2::RE2::Quiet)) {} const LintRuleDescriptor &GenerateLabelPrefixRule::GetDescriptor() { static const LintRuleDescriptor d{ .name = "generate-label-prefix", .topic = "generate-constructs", - .desc = "Checks that every generate block label starts with g_ or gen_.", + .desc = + "Checks that every generate block label matches the regex defined by " + "style_regex. The default regex requires labels to start with g_ or " + "gen_. Refer to https://github.com/chipsalliance/verible/tree/master/" + "verilog/tools/lint#readme for more detail on verible regex " + "patterns.", + // NOLINTNEXTLINE(misc-include-cleaner) + .param = {{"style_regex", std::string(kDefaultStyleRegex), + "A regex used to check generate label style."}}, }; return d; } +std::string GenerateLabelPrefixRule::CreateViolationMessage() const { + return absl::StrCat( + "Generate block label does not match the naming convention defined by " + "regex pattern: ", + style_regex_->pattern()); +} + // Matches begin statements static const Matcher &BlockMatcher() { static const Matcher matcher(NodekGenerateBlock()); @@ -84,15 +103,24 @@ } if (label != nullptr) { - if (!(absl::StartsWith(label->text(), "g_") || - absl::StartsWith(label->text(), "gen_"))) { - violations_.insert(verible::LintViolation(*label, kMessage, context)); + if (!RE2::FullMatch(label->text(), *style_regex_)) { + violations_.insert(verible::LintViolation( + *label, CreateViolationMessage(), context)); } } } } } +// NOLINTNEXTLINE(misc-include-cleaner) +absl::Status GenerateLabelPrefixRule::Configure( + std::string_view configuration) { + using verible::config::SetRegex; + absl::Status s = verible::ParseNameValues( + configuration, {{"style_regex", SetRegex(&style_regex_)}}); + return s; +} + verible::LintRuleStatus GenerateLabelPrefixRule::Report() const { return verible::LintRuleStatus(violations_, GetDescriptor()); }
diff --git a/verible/verilog/analysis/checkers/generate-label-prefix-rule.h b/verible/verilog/analysis/checkers/generate-label-prefix-rule.h index 9090cf3..e18df9c 100644 --- a/verible/verilog/analysis/checkers/generate-label-prefix-rule.h +++ b/verible/verilog/analysis/checkers/generate-label-prefix-rule.h
@@ -15,8 +15,13 @@ #ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_GENERATE_LABEL_PREFIX_RULE_H_ #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_GENERATE_LABEL_PREFIX_RULE_H_ +#include <memory> #include <set> +#include <string> +#include <string_view> +#include "absl/status/status.h" +#include "re2/re2.h" #include "verible/common/analysis/lint-rule-status.h" #include "verible/common/analysis/syntax-tree-lint-rule.h" #include "verible/common/text/symbol.h" @@ -27,20 +32,28 @@ namespace analysis { // GenerateLabelPrefixRule checks that all generate block labels start -// with g_ or gen_ +// with g_ or gen_ (configurable via style_regex) class GenerateLabelPrefixRule : public verible::SyntaxTreeLintRule { public: using rule_type = verible::SyntaxTreeLintRule; + GenerateLabelPrefixRule(); + static const LintRuleDescriptor &GetDescriptor(); + std::string CreateViolationMessage() const; + void HandleSymbol(const verible::Symbol &symbol, const verible::SyntaxTreeContext &context) final; verible::LintRuleStatus Report() const final; + absl::Status Configure(std::string_view configuration) final; + private: std::set<verible::LintViolation> violations_; + + std::unique_ptr<re2::RE2> style_regex_; }; } // namespace analysis
diff --git a/verible/verilog/analysis/checkers/generate-label-prefix-rule_test.cc b/verible/verilog/analysis/checkers/generate-label-prefix-rule_test.cc index b46bfe4..cb43501 100644 --- a/verible/verilog/analysis/checkers/generate-label-prefix-rule_test.cc +++ b/verible/verilog/analysis/checkers/generate-label-prefix-rule_test.cc
@@ -27,6 +27,7 @@ namespace { using verible::LintTestCase; +using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; TEST(GenerateLabelPrefixRuleTest, Various) { @@ -229,6 +230,40 @@ RunLintTestCases<VerilogAnalyzer, GenerateLabelPrefixRule>(kTestCases); } +TEST(GenerateLabelPrefixRuleTest, CustomRegex) { + const std::initializer_list<LintTestCase> kTestCases = { + {"module m;\n" + "generate\n" + "if (1) begin : my_custom_label\n" + "end\n" + "endgenerate\nendmodule\n"}, + {"module m;\n" + "generate\n" + "for (genvar i=0; i<5; i++) begin : my_custom_label\n" + "end\n" + "endgenerate\nendmodule\n"}, + }; + RunConfiguredLintTestCases<VerilogAnalyzer, GenerateLabelPrefixRule>( + kTestCases, "style_regex:my_.*"); + + const std::initializer_list<LintTestCase> kFailCases = { + {"module m;\n" + "generate\n" + "if (1) begin : ", + {SymbolIdentifier, "g_label"}, + "\nend\n" + "endgenerate\nendmodule\n"}, + {"module m;\n" + "generate\n" + "if (1) begin : ", + {SymbolIdentifier, "gen_label"}, + "\nend\n" + "endgenerate\nendmodule\n"}, + }; + RunConfiguredLintTestCases<VerilogAnalyzer, GenerateLabelPrefixRule>( + kFailCases, "style_regex:my_.*"); +} + } // namespace } // namespace analysis } // namespace verilog
diff --git a/verible/verilog/analysis/checkers/port-name-suffix-rule.cc b/verible/verilog/analysis/checkers/port-name-suffix-rule.cc index 85a0db3..ffecef3 100644 --- a/verible/verilog/analysis/checkers/port-name-suffix-rule.cc +++ b/verible/verilog/analysis/checkers/port-name-suffix-rule.cc
@@ -28,6 +28,7 @@ #include "verible/common/text/symbol.h" #include "verible/common/text/syntax-tree-context.h" #include "verible/common/text/token-info.h" +#include "verible/common/util/container-util.h" #include "verible/verilog/CST/port.h" #include "verible/verilog/CST/verilog-matchers.h" #include "verible/verilog/analysis/descriptions.h" @@ -90,10 +91,16 @@ {"output", {"o", "no", "po"}}, {"inout", {"io", "nio", "pio"}}}; - // At this point it is guaranteed that the direction will be set to - // one of the expected values (used as keys in the map above). - // Therefore checking the suffix like this is safe - return suffixes.at(direction).count(suffix) == 1; + // `direction` is usually one of the map keys, but the grammar also permits a + // `ref` port direction, which has no suffix convention. FindWithDefault looks + // the direction up with an empty-set fallback, so an unknown direction (e.g. + // `ref`) has no required suffixes and is treated as "correct" (no violation), + // consistent with Violation() which also ignores non-input/output/inout + // directions. + static const std::set<std::string_view> kNoConvention; + const std::set<std::string_view> &valid = + verible::container::FindWithDefault(suffixes, direction, kNoConvention); + return valid.empty() || valid.count(suffix) == 1; } void PortNameSuffixRule::HandleSymbol(const Symbol &symbol, @@ -113,8 +120,11 @@ absl::StrSplit(name, '_', absl::SkipEmpty()); if (name_parts.size() < 2) { - // No suffix at all + // No suffix at all. This also covers an all-underscore name (e.g. "_"), + // for which SkipEmpty leaves name_parts empty; return here so the + // name_parts.back() access below is not reached on an empty vector. Violation(direction, token, context); + return; } if (!IsSuffixCorrect(name_parts.back(), direction)) {
diff --git a/verible/verilog/analysis/checkers/port-name-suffix-rule_test.cc b/verible/verilog/analysis/checkers/port-name-suffix-rule_test.cc index 69f18f4..db69d71 100644 --- a/verible/verilog/analysis/checkers/port-name-suffix-rule_test.cc +++ b/verible/verilog/analysis/checkers/port-name-suffix-rule_test.cc
@@ -57,6 +57,9 @@ {"module t (input bit name_i); endmodule;"}, {"module t (output bit abc_o); endmodule;"}, {"module t (inout bit xyz_io); endmodule;"}, + // A `ref` port has no suffix convention and must not be flagged (and must + // not crash the rule via std::map::at). + {"module t (ref logic data_x); endmodule;"}, {"module t (input logic name_i,\n" "output logic abc_o,\n" "inout logic xyz_io,\n" @@ -90,6 +93,10 @@ {"module t (output logic ", {kToken, "_o"}, "); endmodule;"}, {"module t (inout logic ", {kToken, "_io"}, "); endmodule;"}, + // An all-underscore name splits (SkipEmpty) to an empty parts list; it + // must report a suffix violation, not dereference an empty vector. + {"module t (input logic ", {kToken, "_"}, "); endmodule;"}, + {"module t (input logic ", {kToken, "namei"}, "); endmodule;"}, {"module t (input logic ", {kToken, "nam_ei"}, "); endmodule;"}, {"module t (input logic ", {kToken, "name_o"}, "); endmodule;"},
diff --git a/verible/verilog/analysis/symbol-table_test.cc b/verible/verilog/analysis/symbol-table_test.cc index e15ec5f..3e6b1a6 100644 --- a/verible/verilog/analysis/symbol-table_test.cc +++ b/verible/verilog/analysis/symbol-table_test.cc
@@ -73,8 +73,7 @@ const auto found_##dest(map.find(key)); /* iterator */ \ ASSERT_NE(found_##dest, map.end()) \ << "No element at \"" << key << "\" in " #map; \ - const auto &dest ABSL_ATTRIBUTE_UNUSED( \ - found_##dest->second); /* mapped_type */ + const auto &dest [[maybe_unused]] (found_##dest->second); /* mapped_type */ // Assert that container is not empty, and reference its first element. // Works on any container type with .begin(). @@ -103,7 +102,7 @@ << "No symbol at \"" << key << "\" in " << ScopePathPrinter{scope}; \ EXPECT_EQ(found_##dest->first, key); \ const SymbolTableNode &dest(found_##dest->second); \ - const SymbolInfo &dest##_info ABSL_ATTRIBUTE_UNUSED(dest.Value()) + const SymbolInfo &dest##_info [[maybe_unused]] (dest.Value()) // For SymbolInfo::references_map_view_type only: Assert that there is exactly // one element at 'key' in 'map' and assign it to 'dest' (DependentReferences).
diff --git a/verible/verilog/formatting/BUILD b/verible/verilog/formatting/BUILD index 6150594..6431226 100644 --- a/verible/verilog/formatting/BUILD +++ b/verible/verilog/formatting/BUILD
@@ -163,7 +163,6 @@ "//verible/common/util:expandable-tree-view", "//verible/common/util:interval", "//verible/common/util:interval-set", - "//verible/common/util:iterator-range", "//verible/common/util:logging", "//verible/common/util:spacer", "//verible/common/util:tree-operations", @@ -175,7 +174,6 @@ "//verible/verilog/analysis:verilog-equivalence", "//verible/verilog/parser:verilog-token-enum", "//verible/verilog/preprocessor:verilog-preprocess", - "@abseil-cpp//absl/base:core_headers", "@abseil-cpp//absl/log:die_if_null", "@abseil-cpp//absl/status", "@abseil-cpp//absl/status:statusor",
diff --git a/verible/verilog/formatting/format-style-init.cc b/verible/verilog/formatting/format-style-init.cc index 2dde818..a6d010b 100644 --- a/verible/verilog/formatting/format-style-init.cc +++ b/verible/verilog/formatting/format-style-init.cc
@@ -140,6 +140,13 @@ "Use compact binary expressions inside indexing / bit selection " "operators"); +ABSL_FLAG(bool, class_parameter_space, false, + "If true, keep/insert a space before '#' in a class" + "parameterized typedef, e.g. \"typedef my_class #(.P(P)) " + "my_class_t;\". If false (default), no space is inserted, " + "matching the convention used for IEEE parameterized class " + "instantiations, e.g. \"type#(params...)::method(...)\"."); + ABSL_FLAG(bool, wrap_end_else_clauses, false, "Split end and else keywords into separate lines"); @@ -197,6 +204,7 @@ STYLE_FROM_FLAG(try_wrap_long_lines); STYLE_FROM_FLAG(expand_coverpoints); STYLE_FROM_FLAG(compact_indexing_and_selections); + STYLE_FROM_FLAG(class_parameter_space); STYLE_FROM_FLAG(wrap_end_else_clauses); STYLE_FROM_FLAG(alignment_group_boundary);
diff --git a/verible/verilog/formatting/format-style.h b/verible/verilog/formatting/format-style.h index 2fad329..55347c5 100644 --- a/verible/verilog/formatting/format-style.h +++ b/verible/verilog/formatting/format-style.h
@@ -146,6 +146,9 @@ // Compact binary expressions inside indexing / bit selection operators bool compact_indexing_and_selections = true; + // Keep/insert a space before '#' in a parameterized class typedef + bool class_parameter_space = false; + // Split with a \n end and else clauses bool wrap_end_else_clauses = false;
diff --git a/verible/verilog/formatting/formatter.cc b/verible/verilog/formatting/formatter.cc index 9893b60..9bd3dd2 100644 --- a/verible/verilog/formatting/formatter.cc +++ b/verible/verilog/formatting/formatter.cc
@@ -26,7 +26,6 @@ #include <string_view> #include <vector> -#include "absl/base/attributes.h" #include "absl/log/die_if_null.h" #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -50,7 +49,6 @@ #include "verible/common/util/expandable-tree-view.h" #include "verible/common/util/interval-set.h" #include "verible/common/util/interval.h" -#include "verible/common/util/iterator-range.h" #include "verible/common/util/logging.h" #include "verible/common/util/spacer.h" #include "verible/common/util/tree-operations.h" @@ -749,15 +747,24 @@ case verible::SpacingDecision::kPreserve: { if (token.before.preserved_space_start != verible::string_view_null_iterator()) { - *column += token.OriginalLeadingSpaces().length(); + const std::string_view leading = token.OriginalLeadingSpaces(); + const auto last_nl = leading.find_last_of('\n'); + if (last_nl == std::string_view::npos) { + *column += leading.length(); + } else { + // Reset column after the last newline, same as FormattedToken + // emit (GitHub issue 2542). + *column = static_cast<int>(leading.length() - last_nl - 1); + } } else { *column += token.before.spaces; } break; } case verible::SpacingDecision::kWrap: - *column = 0; - ABSL_FALLTHROUGH_INTENDED; + // Newline then only the wrap indent (same as FormattedToken emit). + *column = token.before.spaces; + break; case verible::SpacingDecision::kAlign: case verible::SpacingDecision::kAppend: *column += token.before.spaces; @@ -766,6 +773,15 @@ } static int CalculateEolCommentColumn(const verible::FormattedExcerpt &line) { + // Compute the starting column of the trailing EOL comment the same way + // FormattedExcerpt::FormattedText emits spaces, including: + // * wrap indents (SpacingDecision::kWrap), and + // * preserved leading whitespace that may contain newlines (common when + // an original line break is kept). Counting those newlines as width + // made continuation comments land on the wrong column and fail to + // converge on re-format (GitHub issue 2542). + if (line.Tokens().empty()) return 0; + int column = 0; const auto &front = line.Tokens().front(); @@ -777,12 +793,16 @@ } column += front.token->text().length(); - for (const auto &ftoken : verible::make_range(line.Tokens().begin() + 1, - line.Tokens().end() - 1)) { + const auto &tokens = line.Tokens(); + for (size_t i = 1; i < tokens.size(); ++i) { + const auto &ftoken = tokens[i]; AdjustColumnUsingTokenSpacing(ftoken, &column); - column += ftoken.token->text().length(); + // Do not add the last token's length: that is the EOL comment whose + // starting column we want. + if (i + 1 < tokens.size()) { + column += ftoken.token->text().length(); + } } - AdjustColumnUsingTokenSpacing(line.Tokens().back(), &column); CHECK_GE(column, 0); return column;
diff --git a/verible/verilog/formatting/formatter_test.cc b/verible/verilog/formatting/formatter_test.cc index ea76237..16e0572 100644 --- a/verible/verilog/formatting/formatter_test.cc +++ b/verible/verilog/formatting/formatter_test.cc
@@ -4478,19 +4478,42 @@ " .L(L),\n" " .W(W)\n" ") bar_t;\n"}, - // unqualified parameterized type keeps a space before '#' + // By default (class_parameter_space == false), no space before '#' {"typedef dv_base_env_cov #(.CFG_T(tl_agent_env_cfg)) tl_agent_env_cov;\n", - "typedef dv_base_env_cov #(\n" + "typedef dv_base_env_cov#(\n" " .CFG_T(tl_agent_env_cfg)\n" ") tl_agent_env_cov;\n"}, - // ... and is inserted when absent {"typedef dv_base_env_cov#(.CFG_T(tl_agent_env_cfg)) tl_agent_env_cov;\n", - "typedef dv_base_env_cov #(\n" + "typedef dv_base_env_cov#(\n" " .CFG_T(tl_agent_env_cfg)\n" ") tl_agent_env_cov;\n"}, // single short parameter stays on one line {"typedef my_class #(.P(P)) my_class_t;\n", - "typedef my_class #(.P(P)) my_class_t;\n"}, + "typedef my_class#(.P(P)) my_class_t;\n"}, + + // let declarations each stay on their own line + {"module t;\n" + "let OFF = 4;\n" + "let UNIQUE = 32;\n" + "let PP(a) = 30 + a;\n" + "endmodule\n", + "module t;\n" + " let OFF = 4;\n" + " let UNIQUE = 32;\n" + " let PP(a) = 30 + a;\n" + "endmodule\n"}, + + // let declarations each stay on their own line + {"module t;\n" + "let OFF = 4;\n" + "let UNIQUE = 32;\n" + "let PP(a) = 30 + a;\n" + "endmodule\n", + "module t;\n" + " let OFF = 4;\n" + " let UNIQUE = 32;\n" + " let PP(a) = 30 + a;\n" + "endmodule\n"}, // package test cases {"package fedex;localparam int www=3 ;endpackage : fedex\n", @@ -4707,6 +4730,15 @@ " for (int i = 0; i < f(m); i--) begin\n" " end\n" "endfunction\n"}, + {// for loop with an attribute instance in the initializer. + // Regression: this used to abort with a CHECK failure while reshaping + // the kForSpec partitions when an attribute appears in the header. + "module m; initial for(int i=0(* a *);i<4;i++) x=i; endmodule", + "module m;\n" + " initial\n" + " for (int i = 0 (* a *); i < 4; i++)\n" + " x = i;\n" + "endmodule\n"}, {// forever loop "function\nvoid\tforevah;forever begin " "++k\n;end endfunction\n", @@ -18788,6 +18820,50 @@ } } +static constexpr FormatterTestCase + kSpaceBeforeHashInUnqualifiedTypedefTestCases[] = { + // unqualified parameterized type keeps a space before '#' + {"typedef dv_base_env_cov #(.CFG_T(tl_agent_env_cfg)) " + "tl_agent_env_cov;\n", + "typedef dv_base_env_cov #(\n" + " .CFG_T(tl_agent_env_cfg)\n" + ") tl_agent_env_cov;\n"}, + // ... and is inserted when absent + {"typedef dv_base_env_cov#(.CFG_T(tl_agent_env_cfg)) " + "tl_agent_env_cov;\n", + "typedef dv_base_env_cov #(\n" + " .CFG_T(tl_agent_env_cfg)\n" + ") tl_agent_env_cov;\n"}, + // single short parameter stays on one line + {"typedef my_class #(.P(P)) my_class_t;\n", + "typedef my_class #(.P(P)) my_class_t;\n"}, + // package-qualified types are unaffected (no space before '#') + {"typedef foo_pkg::baz_t#(.L(L), .W(W)) bar_t;\n", + "typedef foo_pkg::baz_t#(\n" + " .L(L),\n" + " .W(W)\n" + ") bar_t;\n"}, +}; + +TEST(FormatterEndToEndTest, SpaceBeforeHashInUnqualifiedTypedefTestCases) { + // Use a fixed style. + FormatStyle style; + style.column_limit = 40; + style.indentation_spaces = 2; + style.wrap_spaces = 4; + style.class_parameter_space = true; + + for (const auto &test_case : kSpaceBeforeHashInUnqualifiedTypedefTestCases) { + VLOG(1) << "code-to-format:\n" << test_case.input << "<EOF>"; + std::ostringstream stream; + const auto status = + FormatVerilog(test_case.input, "<filename>", style, stream); + // Require these test cases to be valid. + EXPECT_OK(status) << status.message(); + EXPECT_EQ(stream.str(), test_case.expected) << "code:\n" << test_case.input; + } +} + static constexpr FormatterTestCase kFunctionCallsWithComments[] = { {// no comments "module foo;\n" @@ -19424,6 +19500,68 @@ } } +// Regression for https://github.com/chipsalliance/verible/issues/2544: +// Wrapping a $bits(...)'(...) cast may leave `MACRO at EOL, reclassifying +// MacroIdentifier as MacroIdItem. FormatEquivalent must accept that, and +// formatting must still pass verification. +TEST(FormatterEndToEndTest, MacroBeforeCloseParenFormatEquivalent) { + static constexpr std::string_view kInput = + "module m;\n" + " assign result_value = $bits(result_value)'( " + "compare_bytes(input_data[DATA_WIDTH_INT-1:0], " + "input_datak[STROBE_WIDTH_INT-1:0], `TOKEN_BYTE) );\n" + "endmodule\n"; + FormatStyle style; + std::ostringstream stream; + const auto status = FormatVerilog(kInput, "<filename>", style, stream); + EXPECT_OK(status) << status.message(); + EXPECT_THAT(stream.str(), testing::HasSubstr("`TOKEN_BYTE")); +} + +// Regression for https://github.com/chipsalliance/verible/issues/2542: +// Continuation EOL comments after a wrapped assign must keep a stable column +// across re-format (convergence). +TEST(FormatterEndToEndTest, ContinuationCommentAfterWrappedAssignConverges) { + static constexpr FormatterTestCase kTestCases[] = { + {// Comments originally column-aligned after a wrapped assign + "module m;\n" + " assign status_ur = !(status_sc || status_ca ||\n" + " status_crs); // Completions with a Reserved Completion\n" + " // Status value are treated as UR\n" + "endmodule\n", + "module m;\n" + " assign status_ur =\n" + " !(status_sc || status_ca || status_crs); // Completions with a " + "Reserved Completion\n" + " // Status value are " + "treated as UR\n" + "endmodule\n"}, + {// Previously mis-aligned continuation is not treated as a continuation + // (column delta > 1) and must still converge + "module m;\n" + " assign status_ur = !(status_sc || status_ca ||\n" + " status_crs); // Completions with a Reserved Completion\n" + " " + "// Status value are treated as UR\n" + "endmodule\n", + "module m;\n" + " assign status_ur =\n" + " !(status_sc || status_ca || status_crs); // Completions with a " + "Reserved Completion\n" + " // Status value are treated as UR\n" + "endmodule\n"}, + }; + FormatStyle style; // default column_limit (100) + for (const auto &test_case : kTestCases) { + VLOG(1) << "code-to-format:\n" << test_case.input << "<EOF>"; + std::ostringstream stream; + const auto status = + FormatVerilog(test_case.input, "<filename>", style, stream); + EXPECT_OK(status) << status.message(); + EXPECT_EQ(stream.str(), test_case.expected) << "code:\n" << test_case.input; + } +} + // Regression for https://github.com/chipsalliance/verible/issues/2540: // Trailing EOL comment after `end` before `else if` must not change whether // the else-if assignment stays on one line across re-format (convergence). @@ -21003,6 +21141,114 @@ } } +// Regression for https://github.com/chipsalliance/verible/issues/2008 +// (also https://github.com/chipsalliance/verible/issues/2474 and +// https://github.com/chipsalliance/verible/issues/2063): +// Non-ANSI "input wire signed" used to abort in the tree-unwrapper because +// the CST visited "signed" before "wire", which is the reverse of source +// order. +TEST(FormatterEndToEndTest, NonAnsiWireSignedModulePortDoesNotAbort) { + static constexpr FormatterTestCase kTestCases[] = { + {// Original issue #2008 sample + "module uut( sig1 );\n" + "\n" + "input wire signed [15:0] sig1;\n" + "\n" + "endmodule\n", + "module uut (\n" + " sig1\n" + ");\n" + "\n" + " input wire signed [15:0] sig1;\n" + "\n" + "endmodule\n"}, + {// Issue #2474 sample + "module myModule (\n" + " myinput\n" + ");\n" + "input wire signed [7:0] myInput;\n" + "endmodule\n", + "module myModule (\n" + " myinput\n" + ");\n" + " input wire signed [7:0] myInput;\n" + "endmodule\n"}, + {// Issue #2063 sample: signed wire with no packed dimensions + "module top(a);\n" + " input wire signed a;\n" + "endmodule\n", + "module top (\n" + " a\n" + ");\n" + " input wire signed a;\n" + "endmodule\n"}, + {// Same production with logic instead of wire + "module uut(sig1);\n" + "input logic signed [15:0] sig1;\n" + "endmodule\n", + "module uut (\n" + " sig1\n" + ");\n" + " input logic signed [15:0] sig1;\n" + "endmodule\n"}, + {// output / inout net types + "module uut(sig1, sig2);\n" + "output wire signed [15:0] sig1;\n" + "inout wire signed [7:0] sig2;\n" + "endmodule\n", + "module uut (\n" + " sig1,\n" + " sig2\n" + ");\n" + " output wire signed [15:0] sig1;\n" + " inout wire signed [7:0] sig2;\n" + "endmodule\n"}, + {// unsigned is the same production + "module uut(sig1);\n" + "input wire unsigned [15:0] sig1;\n" + "endmodule\n", + "module uut (\n" + " sig1\n" + ");\n" + " input wire unsigned [15:0] sig1;\n" + "endmodule\n"}, + {// ANSI form already worked; keep as a regression + "module uut(input wire signed [15:0] sig1);\n" + "endmodule\n", + "module uut (\n" + " input wire signed [15:0] sig1\n" + ");\n" + "endmodule\n"}, + {// Non-ANSI without signed still works + "module uut(sig1);\n" + "input wire [15:0] sig1;\n" + "endmodule\n", + "module uut (\n" + " sig1\n" + ");\n" + " input wire [15:0] sig1;\n" + "endmodule\n"}, + {// Non-ANSI signed without net type still works + "module uut(sig1);\n" + "input signed [15:0] sig1;\n" + "endmodule\n", + "module uut (\n" + " sig1\n" + ");\n" + " input signed [15:0] sig1;\n" + "endmodule\n"}, + }; + FormatStyle style; // default column_limit (100) + for (const auto &test_case : kTestCases) { + VLOG(1) << "code-to-format:\n" << test_case.input << "<EOF>"; + std::ostringstream stream; + const auto status = + FormatVerilog(test_case.input, "<filename>", style, stream); + EXPECT_OK(status) << status.message(); + EXPECT_EQ(stream.str(), test_case.expected) << "code:\n" << test_case.input; + } +} + } // namespace } // namespace formatter } // namespace verilog
diff --git a/verible/verilog/formatting/token-annotator.cc b/verible/verilog/formatting/token-annotator.cc index def1349..ebaeb21 100644 --- a/verible/verilog/formatting/token-annotator.cc +++ b/verible/verilog/formatting/token-annotator.cc
@@ -510,7 +510,8 @@ // This may be controversial or context-dependent, as parameterized // classes often appear with method calls like: // type#(params...)::method(...); - // A parameterized type in a typedef keeps the space before '#': + // If style.class_parameter_space is enabled, a + // parameterized type in a typedef keeps the space before '#': // typedef my_class #(.P(P)) my_class_t; // but a package-qualified type does not, matching the existing // "type#(params...)::method(...)" convention: @@ -518,6 +519,7 @@ // Kept as separate IsInsideFirst() calls because MatchesTagAnyOf() // only unrolls up to four tags. const bool inside_unqualified_typedef = + style.class_parameter_space && left_context.IsInsideFirst({NodeEnum::kTypeDeclaration}, {}) && !left_context.IsInsideFirst({NodeEnum::kQualifiedId}, {});
diff --git a/verible/verilog/formatting/tree-unwrapper.cc b/verible/verilog/formatting/tree-unwrapper.cc index fddc50d..09fa8c0 100644 --- a/verible/verilog/formatting/tree-unwrapper.cc +++ b/verible/verilog/formatting/tree-unwrapper.cc
@@ -801,6 +801,7 @@ case NodeEnum::kPreprocessorUndef: case NodeEnum::kTFPortDeclaration: case NodeEnum::kTypeDeclaration: + case NodeEnum::kLetDeclaration: case NodeEnum::kNetTypeDeclaration: case NodeEnum::kForwardDeclaration: case NodeEnum::kInterfaceClassMethod: @@ -3037,10 +3038,15 @@ auto &children = partition.Children(); const auto iter1 = std::find_if(children.begin(), children.end(), PartitionStartsWithSemicolon); - CHECK(iter1 != children.end()); + // An attribute instance ((* ... *)) in the for-loop header can change + // the partition structure so that the expected semicolon-leading + // partitions are not present. When they are missing, leave the + // partitions unreshaped rather than aborting (avoids a CHECK-failure + // crash on attributed/unusual for-headers). + if (iter1 == children.end()) break; const auto iter2 = std::find_if(iter1 + 1, children.end(), PartitionStartsWithSemicolon); - CHECK(iter2 != children.end()); + if (iter2 == children.end()) break; const int dist1 = std::distance(children.begin(), iter1); const int dist2 = std::distance(children.begin(), iter2); VLOG(4) << "kForSpec got ';' at child " << dist1 << " and " << dist2;
diff --git a/verible/verilog/parser/verilog.y b/verible/verilog/parser/verilog.y index f1c24a8..a6de1a8 100644 --- a/verible/verilog/parser/verilog.y +++ b/verible/verilog/parser/verilog.y
@@ -5623,15 +5623,23 @@ | port_direction signed_unsigned_opt list_of_module_item_identifiers ';' { $$ = MakeTaggedNode(N::kModulePortDeclaration, $1, MakeDataType($2, nullptr, nullptr), $3, $4);} /* implicit type */ + /* Keep net/var type before signing so CST leaf order matches source + * ("wire signed", not "signed" then "wire"). The formatter walks CST + * order and CHECK-fails when a later leaf appears earlier in the file. + * Wrap as kDataTypePrimitive like the TK_bit rule so + * GetBaseTypeFromDataType still treats child 1 as the type. + */ | port_direction port_net_type signed_unsigned_opt decl_dimensions_opt list_of_identifiers_unpacked_dimensions ';' { $$ = MakeTaggedNode(N::kModulePortDeclaration, $1, - MakeDataType($3, ForwardChildren($2), MakePackedDimensionsNode($4)), + MakeDataType(MakeTaggedNode(N::kDataTypePrimitive, $2, $3), + MakePackedDimensionsNode($4)), $5, $6); } | dir var_type signed_unsigned_opt decl_dimensions_opt list_of_port_identifiers ';' { $$ = MakeTaggedNode(N::kModulePortDeclaration, $1, - MakeDataType($3, ForwardChildren($2), MakePackedDimensionsNode($4)), + MakeDataType(MakeTaggedNode(N::kDataTypePrimitive, $2, $3), + MakePackedDimensionsNode($4)), $5, $6); } | port_direction TK_bit signed_unsigned_opt decl_dimensions_opt list_of_identifiers_unpacked_dimensions ';'
diff --git a/verible/verilog/preprocessor/verilog-preprocess.cc b/verible/verilog/preprocessor/verilog-preprocess.cc index 2f77111..1d847fb 100644 --- a/verible/verilog/preprocessor/verilog-preprocess.cc +++ b/verible/verilog/preprocessor/verilog-preprocess.cc
@@ -245,11 +245,15 @@ if ((*token_iter)->text() == "(") { token_iter = GenerateBypassWhiteSpaces(generator); // skip the "(" } else { + preprocess_data_.errors.emplace_back( + **token_iter, + "Error it is illegal to call a callable macro without ()."); return absl::InvalidArgumentError( "Error it is illegal to call a callable macro without ()."); } while (parameters_size > 0) { + if ((*token_iter)->isEOF()) break; // truncated call; stop scanning args if ((*token_iter)->token_enum() == MacroArg) { macro_call->positional_arguments.emplace_back(**token_iter); token_iter = GenerateBypassWhiteSpaces(generator); @@ -342,6 +346,10 @@ lexer.DoNextToken()) { lexed_sequence.push_back(lexer.GetLastToken()); } + // Retain the EOF token as an end sentinel so a truncated callable-macro + // invocation stops at EOF in GenerateBypassWhiteSpaces instead of + // dereferencing past the end of the stream view. + lexed_sequence.push_back(lexer.GetLastToken()); verible::TokenStreamView lexed_streamview; // Initializing the lexed token stream view. InitTokenStreamView(lexed_sequence, &lexed_streamview); @@ -352,6 +360,7 @@ // Token-pulling loop. for (auto iter = iter_generator(); iter != end; iter = iter_generator()) { auto &last_token = **iter; + if (last_token.isEOF()) break; // end sentinel; nothing to forward // TODO: handle lexical error if (lexer.GetLastToken().token_enum() == TK_SPACE) { continue; // don't forward spaces @@ -396,6 +405,8 @@ lexer.DoNextToken()) { lexed_sequence.push_back(lexer.GetLastToken()); } + // Retain EOF end sentinel (see ExpandText). + lexed_sequence.push_back(lexer.GetLastToken()); verible::TokenStreamView lexed_streamview; // Initializing the lexed token stream view. InitTokenStreamView(lexed_sequence, &lexed_streamview); @@ -407,6 +418,7 @@ for (auto iter = iter_generator(); iter != end; iter = iter_generator()) { // TODO: handle lexical error auto &last_token = **iter; + if (last_token.isEOF()) break; // end sentinel; nothing to forward if (last_token.token_enum() == TK_SPACE) continue; // don't forward spaces // If the expanded token is another macro identifier that needs to be // expanded. @@ -635,6 +647,9 @@ lexer.DoNextToken()) { included_sequence.push_back(lexer.GetLastToken()); } + // Retain EOF end sentinel; the child ScanStream expects an EOF-terminated + // stream. + included_sequence.push_back(lexer.GetLastToken()); // Preprocessing the included file tokens. verible::TokenStreamView lexed_streamview; @@ -657,8 +672,11 @@ preprocess_data_.included_text_structure.push_back(std::move(u)); } - // Forwarding the included preprocessed view. + // Forwarding the included preprocessed view. The EOF end sentinel appended + // above is consumed by the child ScanStream and must not be spliced into the + // middle of the parent's token stream. for (const auto &u : child_preprocessed_data.preprocessed_token_stream) { + if (u->isEOF()) continue; preprocess_data_.preprocessed_token_stream.push_back(u); }
diff --git a/verible/verilog/preprocessor/verilog-preprocess.h b/verible/verilog/preprocessor/verilog-preprocess.h index 2d134ae..0d932f0 100644 --- a/verible/verilog/preprocessor/verilog-preprocess.h +++ b/verible/verilog/preprocessor/verilog-preprocess.h
@@ -164,9 +164,11 @@ absl::Status HandleElse(TokenStreamView::const_iterator else_pos); absl::Status HandleEndif(TokenStreamView::const_iterator endif_pos); - static absl::Status ConsumeAndParseMacroCall( - TokenStreamView::const_iterator, const StreamIteratorGenerator &, - verible::MacroCall *, const verible::MacroDefinition &); + // Non-static so it can record diagnostics into preprocess_data_.errors. + absl::Status ConsumeAndParseMacroCall(TokenStreamView::const_iterator, + const StreamIteratorGenerator &, + verible::MacroCall *, + const verible::MacroDefinition &); // The following functions return nullptr when there is no error: absl::Status ConsumeMacroDefinition(const StreamIteratorGenerator &,
diff --git a/verible/verilog/preprocessor/verilog-preprocess_test.cc b/verible/verilog/preprocessor/verilog-preprocess_test.cc index ae33567..a8e41d9 100644 --- a/verible/verilog/preprocessor/verilog-preprocess_test.cc +++ b/verible/verilog/preprocessor/verilog-preprocess_test.cc
@@ -1033,5 +1033,30 @@ << error.error_message; } +// Regression: a callable-macro invocation truncated at end-of-stream (no '(', +// or '(' with no matching ')') must not crash or hang the preprocessor. Before +// the fix these inputs dereferenced past the end of the token stream view +// (SIGSEGV) or spun forever scanning arguments. With error-surfacing enabled +// the no-'(' cases also report a preprocessor diagnostic. +TEST(VerilogPreprocessTest, TruncatedCallableMacroDoesNotCrash) { + constexpr std::string_view kNoParenInputs[] = { + "`define A(x) hello `A\n`A(1)\n", // truncated callable ref in macro body + "`define A(x) x\n`A\n", // truncated callable ref at top level + }; + for (std::string_view input : kNoParenInputs) { + PreprocessorTester tester( + input, VerilogPreprocess::Config({.expand_macros = true})); + EXPECT_FALSE(tester.Status().ok()) << input; + EXPECT_GE(tester.PreprocessorData().errors.size(), 1) << input; + } + + // '(' with no matching ')': must terminate (was an infinite loop). The + // residue is rejected downstream, so only assert non-OK here. + PreprocessorTester open_paren( + "`define C(z) z\n`define A(x) hello `C(\n`A(1)\n", + VerilogPreprocess::Config({.expand_macros = true})); + EXPECT_FALSE(open_paren.Status().ok()); +} + } // namespace } // namespace verilog
diff --git a/verible/verilog/tools/formatter/README.md b/verible/verilog/tools/formatter/README.md index 01b2869..a3ffb45 100644 --- a/verible/verilog/tools/formatter/README.md +++ b/verible/verilog/tools/formatter/README.md
@@ -42,6 +42,11 @@ {align,flush-left,preserve,infer}); default: infer; --class_member_variable_alignment (Format class member variables: {align,flush-left,preserve,infer}); default: infer; + --class_parameter_space (If true, keep/insert a space + before '#' in a class parameterized typedef, e.g. "typedef + my_class #(.P(P)) my_class_t;". If false (default), no space is + inserted, matching the IEEE convention used for parameterized class + instantiations, e.g. "type#(params...)::method(...)".); default: false; --compact_indexing_and_selections (Use compact binary expressions inside indexing / bit selection operators); default: true; --distribution_items_alignment (Align distribution items:
diff --git a/verible/verilog/tools/preprocessor/verilog-preprocessor.cc b/verible/verilog/tools/preprocessor/verilog-preprocessor.cc index c23fdf7..26907dc 100644 --- a/verible/verilog/tools/preprocessor/verilog-preprocessor.cc +++ b/verible/verilog/tools/preprocessor/verilog-preprocessor.cc
@@ -122,13 +122,17 @@ // source code just like it was, but with conditionals filtered. lexed_sequence.push_back(lexer.GetLastToken()); } + lexed_sequence.push_back(lexer.GetLastToken()); // EOF end sentinel verible::TokenStreamView lexed_streamview; // Initializing the lexed token stream view. InitTokenStreamView(lexed_sequence, &lexed_streamview); verilog::VerilogPreprocessData preprocessed_data = preprocessor.ScanStream(lexed_streamview); auto &preprocessed_stream = preprocessed_data.preprocessed_token_stream; - for (auto u : preprocessed_stream) outs << u->text(); + for (auto u : preprocessed_stream) { + if (u->isEOF()) continue; // end sentinel, not part of the source + outs << u->text(); + } for (auto &u : preprocessed_data.errors) outs << u.error_message << '\n'; if (!preprocessed_data.errors.empty()) { return absl::InvalidArgumentError("Error: The preprocessing has failed.");