Merge pull request #2581 from kbrunham-intel/fix/2008
Fix formatter abort on non-ANSI wire signed ports
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..6e4c080 100755
--- a/.github/bin/smoke-test.sh
+++ b/.github/bin/smoke-test.sh
@@ -39,6 +39,7 @@
TMPDIR="${TMPDIR:-/tmp}"
readonly BASE_TEST_DIR="${TMPDIR}/test/verible-smoke-test"
+readonly DEFAULT_HASH_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 +82,12 @@
#
# 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"
+readonly PROJECT_HASHES_FILE="${1:-${DEFAULT_HASH_FILE}}"
+
+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 +119,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[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 +148,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 +171,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 +221,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 +237,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 +254,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 +342,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
@@ -388,18 +375,20 @@
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}")"
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}")"
PROJECT_DIR="${BASE_TEST_DIR}/${PROJECT_NAME}"
# Already cloned above
@@ -412,14 +401,14 @@
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
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/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/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/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 45e1e77..5205a01 100644
--- a/verible/verilog/formatting/formatter_test.cc
+++ b/verible/verilog/formatting/formatter_test.cc
@@ -19380,6 +19380,50 @@
}
}
+// 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).
