Skip to content

Commit

Permalink
Add Wcase-outside-range warning
Browse files Browse the repository at this point in the history
  • Loading branch information
MikePopoloski committed Nov 23, 2024
1 parent 39c2540 commit d917e79
Show file tree
Hide file tree
Showing 12 changed files with 186 additions and 33 deletions.
2 changes: 2 additions & 0 deletions bindings/python/NumericBindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@ void registerNumeric(py::module_& m) {
.def("getMinRepresentedBits", &SVInt::getMinRepresentedBits)
.def("countLeadingZeros", &SVInt::countLeadingZeros)
.def("countLeadingOnes", &SVInt::countLeadingOnes)
.def("countLeadingUnknowns", &SVInt::countLeadingUnknowns)
.def("countLeadingZs", &SVInt::countLeadingZs)
.def("countOnes", &SVInt::countOnes)
.def("countZeros", &SVInt::countZeros)
.def("countXs", &SVInt::countXs)
Expand Down
2 changes: 2 additions & 0 deletions include/slang/numeric/ConstantValue.h
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,8 @@ class SLANG_EXPORT ConstantValue {
/// Gets the size of this value when converted to a bitstream.
uint64_t getBitstreamWidth() const;

std::optional<bitwidth_t> getEffectiveWidth() const;

static const ConstantValue Invalid;

SLANG_EXPORT friend std::ostream& operator<<(std::ostream& os, const ConstantValue& cv);
Expand Down
6 changes: 6 additions & 0 deletions include/slang/numeric/SVInt.h
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,12 @@ class SLANG_EXPORT SVInt : SVIntStorage {
return countLeadingOnesSlowCase();
}

/// Count the number of leading unknown bits.
bitwidth_t countLeadingUnknowns() const;

/// Count the number of leading Z bits.
bitwidth_t countLeadingZs() const;

/// Count the number of 1 bits in the number.
bitwidth_t countOnes() const;

Expand Down
3 changes: 2 additions & 1 deletion scripts/diagnostics.txt
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,7 @@ warning comparison-mismatch ComparisonMismatch "comparison between operands of d
warning sign-compare SignCompare "comparison of differently signed types ({} and {})"
warning case-type CaseTypeMismatch "comparison of different types in '{}' statement ({} and {})"
warning case-default CaseDefault "'{}' missing 'default' label"
warning case-outside-range CaseOutsideRange "'{}' item with {} bits can never match the {} bit case expression"
warning bitwise-rel-precedence BitwiseRelPrecedence "'{}' has lower precedence than '{}'; '{}' will be evaluated first"
note NotePrecedenceBitwiseFirst "place parentheses around the '{}' expression to evaluate it first"
note NotePrecedenceSilence "place parentheses around the '{}' expression to silence this warning"
Expand Down Expand Up @@ -1171,7 +1172,7 @@ group extra = { empty-member empty-stmt dup-import pointless-void-cast case-gen-
ineffective-sign port-width-trunc constant-conversion float-bool-conv int-bool-conv
useless-cast unsigned-arith-shift static-init-order static-init-value float-int-conv
float-narrow bitwise-rel-precedence arith-in-shift logical-not-parentheses
consecutive-comparison }
consecutive-comparison case-outside-range }

group pedantic = { empty-pattern implicit-net-port nonstandard-escape-code nonstandard-generate format-multibit-strength
nonstandard-sys-func nonstandard-foreach nonstandard-dist isunbounded-param-arg udp-coverage }
Expand Down
14 changes: 14 additions & 0 deletions scripts/warning_docs.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1516,3 +1516,17 @@ module m;
end
endmodule
```

-Wcase-outside-range
A case item has a known value that makes it impossible to ever match
the case condition (or vice versa).
```
module m;
logic [2:0] a;
initial begin
case (a)
4'b1000: ;
endcase
end
endmodule
```
9 changes: 1 addition & 8 deletions source/ast/expressions/LiteralExpressions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -88,14 +88,7 @@ ConstantValue IntegerLiteral::evalImpl(EvalContext&) const {
}

std::optional<bitwidth_t> IntegerLiteral::getEffectiveWidthImpl() const {
auto&& val = getValue();
if (val.hasUnknown())
return val.getBitWidth();

if (val.isNegative())
return val.getMinRepresentedBits();

return val.getActiveBits();
return ConstantValue(getValue()).getEffectiveWidth();
}

Expression::EffectiveSign IntegerLiteral::getEffectiveSignImpl(bool isForConversion) const {
Expand Down
20 changes: 3 additions & 17 deletions source/ast/expressions/MiscExpressions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -316,27 +316,13 @@ bool ValueExpressionBase::checkVariableAssignment(const ASTContext& context,
}

std::optional<bitwidth_t> ValueExpressionBase::getEffectiveWidthImpl() const {
auto cvToWidth = [this](const ConstantValue& cv) -> std::optional<bitwidth_t> {
if (!cv.isInteger())
return std::nullopt;

auto& sv = cv.integer();
if (sv.hasUnknown())
return type->getBitWidth();

if (sv.isNegative())
return sv.getMinRepresentedBits();

return sv.getActiveBits();
};

switch (symbol.kind) {
case SymbolKind::Parameter:
return cvToWidth(symbol.as<ParameterSymbol>().getValue(sourceRange));
return symbol.as<ParameterSymbol>().getValue(sourceRange).getEffectiveWidth();
case SymbolKind::EnumValue:
return cvToWidth(symbol.as<EnumValueSymbol>().getValue(sourceRange));
return symbol.as<EnumValueSymbol>().getValue(sourceRange).getEffectiveWidth();
case SymbolKind::Specparam:
return cvToWidth(symbol.as<SpecparamSymbol>().getValue(sourceRange));
return symbol.as<SpecparamSymbol>().getValue(sourceRange).getEffectiveWidth();
default:
return type->getBitWidth();
}
Expand Down
50 changes: 43 additions & 7 deletions source/ast/expressions/OperatorExpressions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,25 +30,44 @@ namespace slang::ast {
using namespace parsing;
using namespace syntax;

static std::optional<bitwidth_t> evalEffectiveWidth(const ASTContext& context,
const Expression& expr, TokenKind keyword) {
auto cv = context.tryEval(expr);
auto width = cv.getEffectiveWidth();
if (!width)
return std::nullopt;

// If the case statement we are evaluating allows wildcards,
// don't count a wildcard in a top bit as part of the effective
// width, since it can match zeros.
if (keyword == TokenKind::CaseXKeyword)
return *width - cv.integer().countLeadingUnknowns();
if (keyword == TokenKind::CaseZKeyword)
return *width - cv.integer().countLeadingZs();
return width;
}

bool Expression::bindMembershipExpressions(const ASTContext& context, TokenKind keyword,
bool requireIntegral, bool unwrapUnpacked,
bool allowTypeReferences, bool allowValueRange,
const ExpressionSyntax& valueExpr,
std::span<const ExpressionSyntax* const> expressions,
SmallVectorBase<const Expression*>& results) {
const auto keywordStr = LexerFacts::getTokenKindText(keyword);
auto extraFlags = allowTypeReferences ? ASTFlags::AllowTypeReferences : ASTFlags::None;
Compilation& comp = context.getCompilation();
Expression& valueRes = create(comp, valueExpr, context, extraFlags);
results.push_back(&valueRes);

const Type* type = valueRes.type;
auto valueWidth = evalEffectiveWidth(context, valueRes, keyword);
bool bad = valueRes.bad();
bool canBeStrings = valueRes.isImplicitString();

if ((!requireIntegral && type->isAggregate()) || (requireIntegral && !type->isIntegral())) {
if (!bad) {
context.addDiag(diag::BadSetMembershipType, valueRes.sourceRange)
<< *type << LexerFacts::getTokenKindText(keyword);
<< *type << keywordStr;
bad = true;
}
}
Expand Down Expand Up @@ -85,14 +104,13 @@ bool Expression::bindMembershipExpressions(const ASTContext& context, TokenKind
}
else if (bt.isAggregate()) {
// Aggregates are just never allowed in membership expressions.
context.addDiag(diag::BadSetMembershipType, expr.sourceRange)
<< bt << LexerFacts::getTokenKindText(keyword);
context.addDiag(diag::BadSetMembershipType, expr.sourceRange) << bt << keywordStr;
bad = true;
}
else {
// Couldn't find a common type.
context.addDiag(diag::NoCommonComparisonType, expr.sourceRange)
<< LexerFacts::getTokenKindText(keyword) << bt << *type;
<< keywordStr << bt << *type;
bad = true;
}

Expand All @@ -101,8 +119,26 @@ bool Expression::bindMembershipExpressions(const ASTContext& context, TokenKind
auto& rct = valueRes.type->getCanonicalType();
BinaryExpression::analyzeOpTypes(lct, rct, bt, *valueRes.type, expr, valueRes, context,
expr.sourceRange, diag::CaseTypeMismatch,
/* isComparison */ false,
LexerFacts::getTokenKindText(keyword));
/* isComparison */ false, keywordStr);

if (expr.type->isIntegral() && valueRes.type->isIntegral()) {
// Check whether either the item or the value expression is
// a constant integral (but not both). If so, check that the
// constant value's effective width is not larger than the
// bit width of the other expression, which would make it
// impossible to ever match.
auto exprWidth = evalEffectiveWidth(context, expr, keyword);
auto vw = valueWidth.value_or(valueRes.type->getBitWidth());

if (valueWidth > expr.type->getBitWidth() && !exprWidth) {
auto& diag = context.addDiag(diag::CaseOutsideRange, expr.sourceRange);
diag << keywordStr << expr.type->getBitWidth() << *valueWidth;
}
else if (exprWidth > vw && !valueWidth) {
auto& diag = context.addDiag(diag::CaseOutsideRange, expr.sourceRange);
diag << keywordStr << *exprWidth << vw;
}
}
}
};

Expand Down Expand Up @@ -132,7 +168,7 @@ bool Expression::bindMembershipExpressions(const ASTContext& context, TokenKind
if (requireIntegral) {
if (!bt->isIntegral()) {
context.addDiag(diag::BadSetMembershipType, bound->sourceRange)
<< *bt << LexerFacts::getTokenKindText(keyword);
<< *bt << keywordStr;
bad = true;
}
else {
Expand Down
14 changes: 14 additions & 0 deletions source/numeric/ConstantValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,20 @@ uint64_t ConstantValue::getBitstreamWidth() const {
return width;
}

std::optional<bitwidth_t> ConstantValue::getEffectiveWidth() const {
if (!isInteger())
return std::nullopt;

auto& sv = integer();
if (sv.hasUnknown())
return sv.getBitWidth();

if (sv.isNegative())
return sv.getMinRepresentedBits();

return sv.getActiveBits();
}

std::ostream& operator<<(std::ostream& os, const ConstantValue& cv) {
return os << cv.toString();
}
Expand Down
57 changes: 57 additions & 0 deletions source/numeric/SVInt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1954,6 +1954,63 @@ bitwidth_t SVInt::countLeadingOnesSlowCase() const {
return count;
}

bitwidth_t SVInt::countLeadingUnknowns() const {
if (!hasUnknown())
return 0;

bitwidth_t bitsInMsw = bitWidth % BITS_PER_WORD;
uint32_t shift = 0;
if (!bitsInMsw)
bitsInMsw = BITS_PER_WORD;
else
shift = BITS_PER_WORD - bitsInMsw;

uint32_t words = getNumWords(bitWidth, false);
int i = int(words - 1);
bitwidth_t count = (bitwidth_t)std::countl_one(pVal[i + words] << shift);
if (count == bitsInMsw) {
for (i--; i >= 0; i--) {
if (pVal[i + words] == UINT64_MAX)
count += BITS_PER_WORD;
else {
count += (bitwidth_t)std::countl_one(pVal[i + words]);
break;
}
}
}

return count;
}

bitwidth_t SVInt::countLeadingZs() const {
if (!hasUnknown())
return 0;

bitwidth_t bitsInMsw = bitWidth % BITS_PER_WORD;
uint32_t shift = 0;
if (!bitsInMsw)
bitsInMsw = BITS_PER_WORD;
else
shift = BITS_PER_WORD - bitsInMsw;

uint32_t words = getNumWords(bitWidth, false);
int i = int(words - 1);
bitwidth_t count = (bitwidth_t)std::countl_one((pVal[i + words] & pVal[i]) << shift);
if (count == bitsInMsw) {
for (i--; i >= 0; i--) {
auto elem = pVal[i + words] & pVal[i];
if (elem == UINT64_MAX)
count += BITS_PER_WORD;
else {
count += (bitwidth_t)std::countl_one(elem);
break;
}
}
}

return count;
}

bitwidth_t SVInt::countOnes() const {
if (isSingleWord())
return (bitwidth_t)std::popcount(val);
Expand Down
38 changes: 38 additions & 0 deletions tests/unittests/ast/WarningTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1086,3 +1086,41 @@ endmodule
CHECK(diags[5].code == diag::CaseTypeMismatch);
CHECK(diags[6].code == diag::CaseDefault);
}

TEST_CASE("Case statement out of range") {
auto tree = SyntaxTree::fromText(R"(
module m;
logic [2:0] a;
initial begin
casex (a)
4'b01: ;
4'b1000: ;
-1: ;
4'bx001: ;
default;
endcase
case (4'b1000)
a: ;
default;
endcase
casez (a)
4'b?001: ;
default;
endcase
end
endmodule
)");

Compilation compilation;
compilation.addSyntaxTree(tree);

auto& diags = compilation.getAllDiagnostics();
REQUIRE(diags.size() == 7);
CHECK(diags[0].code == diag::CaseOutsideRange);
CHECK(diags[1].code == diag::CaseTypeMismatch);
CHECK(diags[2].code == diag::CaseTypeMismatch);
CHECK(diags[3].code == diag::CaseTypeMismatch);
CHECK(diags[4].code == diag::CaseOutsideRange);
CHECK(diags[5].code == diag::CaseTypeMismatch);
CHECK(diags[6].code == diag::CaseTypeMismatch);
}
4 changes: 4 additions & 0 deletions tests/unittests/util/NumericTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,10 @@ TEST_CASE("SVInt misc functions") {
CHECK("64'd1"_si.reverse() == 1ull << 63);
CHECK_THAT("129'b1x10"_si.shl(125).reverse(), exactlyEquals("129'b1x1"_si));
CHECK_THAT("128'b1x10"_si.shl(124).reverse(), exactlyEquals("128'b1x1"_si));

CHECK("192'hzzxx000000zz0000"_si.countLeadingUnknowns() == 144);
CHECK("192'hzzxx000000zz0000"_si.countLeadingZs() == 136);
CHECK(a.countLeadingZs() == 0);
}

TEST_CASE("Double conversions") {
Expand Down

0 comments on commit d917e79

Please sign in to comment.