From 6fcb6d78137a91c254135227d83169d3e87326f8 Mon Sep 17 00:00:00 2001 From: MikePopoloski Date: Sat, 6 Apr 2024 10:16:27 -0400 Subject: [PATCH] Fix a couple of places where range selects were not properly checked for overflow --- include/slang/numeric/ConstantValue.h | 15 +++++++++++---- source/ast/ASTContext.cpp | 20 +++++++++++--------- source/ast/expressions/SelectExpressions.cpp | 14 ++++++++++++++ tests/unittests/ast/ExpressionTests.cpp | 16 ++++++++++++++++ tests/unittests/ast/TypeTests.cpp | 19 +++++++++++++++++-- tests/unittests/util/NumericTests.cpp | 2 ++ 6 files changed, 71 insertions(+), 15 deletions(-) diff --git a/include/slang/numeric/ConstantValue.h b/include/slang/numeric/ConstantValue.h index c0c6183d1..852118c33 100644 --- a/include/slang/numeric/ConstantValue.h +++ b/include/slang/numeric/ConstantValue.h @@ -316,10 +316,17 @@ struct SLANG_EXPORT ConstantRange { /// Gets the width of the range, regardless of the order in which /// the bounds are specified. - bitwidth_t width() const { - auto ul = uint32_t(left); - auto ur = uint32_t(right); - return bitwidth_t(left > right ? ul - ur : ur - ul) + 1; + /// + /// @note the width is truncated to fit in a bitwidth_t. Use @a fullWidth + /// if there is a possibility of overflow. + bitwidth_t width() const { return bitwidth_t(fullWidth()); } + + /// Gets the full width of the range, which may not fit into a + /// 32-bit integer. + uint64_t fullWidth() const { + auto ul = uint64_t(left); + auto ur = uint64_t(right); + return (left > right ? ul - ur : ur - ul) + 1; } /// Gets the lower bound of the range, regardless of the order in which diff --git a/source/ast/ASTContext.cpp b/source/ast/ASTContext.cpp index 9d3b91a47..5a575226d 100644 --- a/source/ast/ASTContext.cpp +++ b/source/ast/ASTContext.cpp @@ -523,15 +523,17 @@ void ASTContext::evalRangeDimension(const SelectorSyntax& syntax, bool isPacked, addDiag(diag::PackedDimsRequireFullRange, syntax.sourceRange()); result.kind = DimensionKind::Unknown; } - else if (isPacked && result.range.width() > SVInt::MAX_BITS) { - addDiag(diag::PackedTypeTooLarge, syntax.sourceRange()) - << result.range.width() << (int)SVInt::MAX_BITS; - result.kind = DimensionKind::Unknown; - } - else if (!isPacked && result.range.width() > INT32_MAX) { - addDiag(diag::ArrayDimTooLarge, syntax.sourceRange()) - << result.range.width() << INT32_MAX; - result.kind = DimensionKind::Unknown; + else { + auto fullWidth = result.range.fullWidth(); + if (isPacked && fullWidth > SVInt::MAX_BITS) { + addDiag(diag::PackedTypeTooLarge, syntax.sourceRange()) + << fullWidth << (int)SVInt::MAX_BITS; + result.kind = DimensionKind::Unknown; + } + else if (!isPacked && fullWidth > INT32_MAX) { + addDiag(diag::ArrayDimTooLarge, syntax.sourceRange()) << fullWidth << INT32_MAX; + result.kind = DimensionKind::Unknown; + } } } } diff --git a/source/ast/expressions/SelectExpressions.cpp b/source/ast/expressions/SelectExpressions.cpp index 3748739cd..6ac0959ef 100644 --- a/source/ast/expressions/SelectExpressions.cpp +++ b/source/ast/expressions/SelectExpressions.cpp @@ -417,6 +417,15 @@ void ElementSelectExpression::serializeTo(ASTSerializer& serializer) const { serializer.write("selector", selector()); } +template +static bool checkRangeOverflow(ConstantRange range, TContext& context, SourceRange sourceRange) { + if (range.fullWidth() > INT32_MAX) { + context.addDiag(diag::RangeWidthOverflow, sourceRange); + return true; + } + return false; +} + Expression& RangeSelectExpression::fromSyntax(Compilation& compilation, Expression& value, const RangeSelectSyntax& syntax, SourceRange fullRange, const ASTContext& context) { @@ -528,6 +537,9 @@ Expression& RangeSelectExpression::fromSyntax(Compilation& compilation, Expressi return badExpr(compilation, result); selectionRange = {*lv, *rv}; + if (checkRangeOverflow(selectionRange, context, errorRange)) + return badExpr(compilation, result); + if (selectionRange.isLittleEndian() != valueRange.isLittleEndian() && selectionRange.width() > 1) { auto& diag = context.addDiag(diag::SelectEndianMismatch, errorRange); @@ -740,6 +752,8 @@ std::optional RangeSelectExpression::evalRange(EvalContext& conte ConstantRange result; if (selectionKind == RangeSelectionKind::Simple) { result = {*li, *ri}; + if (checkRangeOverflow(result, context, sourceRange)) + return std::nullopt; } else { bool isLittleEndian = false; diff --git a/tests/unittests/ast/ExpressionTests.cpp b/tests/unittests/ast/ExpressionTests.cpp index c16d78d83..a0ee81c4f 100644 --- a/tests/unittests/ast/ExpressionTests.cpp +++ b/tests/unittests/ast/ExpressionTests.cpp @@ -3631,3 +3631,19 @@ endmodule CHECK(diags[1].code == diag::AutoVarToRefStatic); CHECK(diags[2].code == diag::AutoVarToRefStatic); } + +TEST_CASE("Out of range select regress") { + auto tree = SyntaxTree::fromText(R"( +localparam p = -4'd8; +$info(p[-2147483648:-2147483649]); +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + + auto& diags = compilation.getAllDiagnostics(); + REQUIRE(diags.size() == 3); + CHECK(diags[0].code == diag::RangeWidthOverflow); + CHECK(diags[1].code == diag::SignedIntegerOverflow); + CHECK(diags[2].code == diag::SignedIntegerOverflow); +} diff --git a/tests/unittests/ast/TypeTests.cpp b/tests/unittests/ast/TypeTests.cpp index e2e400f1e..02a715b69 100644 --- a/tests/unittests/ast/TypeTests.cpp +++ b/tests/unittests/ast/TypeTests.cpp @@ -2037,7 +2037,7 @@ module m; struct packed { logic [16777214:0] a; logic [16777214:0] b; } boz; initial begin - $display(foo[2147483647:-2147483647]); + $display(foo[2147483647:1]); $display(bar[-2147483647:2147483647]); end endmodule @@ -2189,7 +2189,7 @@ parameter real r = $; CHECK(diags[0].code == diag::BadAssignment); } -TEST_CASE("Soft packed unions") { +TEST_CASE("v1800-2023: Soft packed unions") { auto options = optionsFor(LanguageVersion::v1800_2023); auto tree = SyntaxTree::fromText(R"( function automatic logic[7:0] foo; @@ -2209,3 +2209,18 @@ endmodule compilation.addSyntaxTree(tree); NO_COMPILATION_ERRORS; } + +TEST_CASE("Type dimension overflow regress") { + auto tree = SyntaxTree::fromText(R"( +logic [-2147483648:-2147483649] a; +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + + auto& diags = compilation.getAllDiagnostics(); + REQUIRE(diags.size() == 3); + CHECK(diags[0].code == diag::PackedTypeTooLarge); + CHECK(diags[1].code == diag::SignedIntegerOverflow); + CHECK(diags[2].code == diag::SignedIntegerOverflow); +} diff --git a/tests/unittests/util/NumericTests.cpp b/tests/unittests/util/NumericTests.cpp index 6a58afe0a..8b81a892d 100644 --- a/tests/unittests/util/NumericTests.cpp +++ b/tests/unittests/util/NumericTests.cpp @@ -205,6 +205,7 @@ TEST_CASE("SVInt to string (and back)") { checkRoundTrip("-999989", LiteralBase::Decimal); checkRoundTrip("12'b101x101z1", LiteralBase::Binary); checkRoundTrip("999999999", LiteralBase::Decimal); + checkRoundTrip("-2147483648", LiteralBase::Decimal); std::ostringstream ss; ss << "96'd192834"_si; @@ -391,6 +392,7 @@ TEST_CASE("Division") { CHECK("-50"_si / "25"_si == -2); CHECK("-50"_si % "-40"_si == -10); CHECK("-50"_si % "40"_si == -10); + CHECK("-4'sd8"_si / "-4'sd7"_si == 1); SVInt v7 = "19823'd234098234098234098234"_si; CHECK("300'd0"_si / "10"_si == 0);