Make Wsign-conversion apply to constant expressions as well

MikePopoloski · Dec 23, 2023 · 1c5df1b · 1c5df1b
1 parent ac66531
commit 1c5df1b
Show file tree

Hide file tree

Showing 13 changed files with 177 additions and 14 deletions.
diff --git a/include/slang/ast/Expression.h b/include/slang/ast/Expression.h
@@ -323,6 +323,10 @@ class SLANG_EXPORT Expression {
     /// represent them. If any encountered expressions have errors, returns nullopt.
     std::optional<bitwidth_t> getEffectiveWidth() const;
 
+    /// Traverses the expression tree and determines whether all operands are known
+    /// to be signed, even if the types involved end up being computed as unsigned.
+    bool getEffectiveSign() const;
+
     /// If this expression is a reference to a symbol, returns a pointer to that symbol.
     /// If the expression is a member access of a struct or class, returns the member
     /// being accessed. If it's a select of an array, returns the root array variable.

diff --git a/include/slang/ast/expressions/AssignmentExpressions.h b/include/slang/ast/expressions/AssignmentExpressions.h
@@ -96,6 +96,7 @@ class SLANG_EXPORT ConversionExpression : public Expression {
 
     ConstantValue evalImpl(EvalContext& context) const;
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
+    bool getEffectiveSignImpl() const;
 
     void serializeTo(ASTSerializer& serializer) const;
 
@@ -112,7 +113,7 @@ class SLANG_EXPORT ConversionExpression : public Expression {
 
     static ConstantValue convert(EvalContext& context, const Type& from, const Type& to,
                                  SourceRange sourceRange, ConstantValue&& value,
-                                 ConversionKind conversionKind);
+                                 ConversionKind conversionKind, const Expression* expr = nullptr);
 
     static bool isKind(ExpressionKind kind) { return kind == ExpressionKind::Conversion; }
 

diff --git a/include/slang/ast/expressions/LiteralExpressions.h b/include/slang/ast/expressions/LiteralExpressions.h
@@ -26,6 +26,7 @@ class SLANG_EXPORT IntegerLiteral : public Expression {
 
     ConstantValue evalImpl(EvalContext& context) const;
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
+    bool getEffectiveSignImpl() const;
 
     void serializeTo(ASTSerializer&) const;
 
@@ -95,6 +96,7 @@ class SLANG_EXPORT UnbasedUnsizedIntegerLiteral : public Expression {
     ConstantValue evalImpl(EvalContext& context) const;
     bool propagateType(const ASTContext& context, const Type& newType);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
+    bool getEffectiveSignImpl() const;
 
     void serializeTo(ASTSerializer&) const;
 

diff --git a/include/slang/ast/expressions/MiscExpressions.h b/include/slang/ast/expressions/MiscExpressions.h
@@ -262,6 +262,7 @@ class SLANG_EXPORT MinTypMaxExpression : public Expression {
     ConstantValue evalImpl(EvalContext& context) const;
     bool propagateType(const ASTContext& context, const Type& newType);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
+    bool getEffectiveSignImpl() const;
 
     void serializeTo(ASTSerializer& serializer) const;
 

diff --git a/include/slang/ast/expressions/OperatorExpressions.h b/include/slang/ast/expressions/OperatorExpressions.h
@@ -31,6 +31,7 @@ class SLANG_EXPORT UnaryExpression : public Expression {
     ConstantValue evalImpl(EvalContext& context) const;
     bool propagateType(const ASTContext& context, const Type& newType);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
+    bool getEffectiveSignImpl() const;
 
     void serializeTo(ASTSerializer& serializer) const;
 
@@ -72,6 +73,7 @@ class SLANG_EXPORT BinaryExpression : public Expression {
     ConstantValue evalImpl(EvalContext& context) const;
     bool propagateType(const ASTContext& context, const Type& newType);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
+    bool getEffectiveSignImpl() const;
 
     void serializeTo(ASTSerializer& serializer) const;
 
@@ -119,6 +121,12 @@ class SLANG_EXPORT ConditionalExpression : public Expression {
     ConstantValue evalImpl(EvalContext& context) const;
     bool propagateType(const ASTContext& context, const Type& newType);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
+    bool getEffectiveSignImpl() const;
+
+    /// If the condition for this expression is a known constant value,
+    /// this method returns the side of the expression that will be selected
+    /// (i.e. the left or right expression). Otherwise returns nullptr.
+    const Expression* knownSide() const { return isConst ? (isTrue ? left_ : right_) : nullptr; }
 
     void serializeTo(ASTSerializer& serializer) const;
 

diff --git a/source/ast/Expression.cpp b/source/ast/Expression.cpp
@@ -84,6 +84,22 @@ class EffectiveWidthVisitor {
     }
 };
 
+class EffectiveSignVisitor {
+public:
+    template<typename T>
+    bool visit(const T& expr) {
+        if constexpr (requires { expr.getEffectiveSignImpl(); }) {
+            if (expr.bad())
+                return false;
+
+            return expr.getEffectiveSignImpl();
+        }
+        else {
+            return expr.type->isSigned();
+        }
+    }
+};
+
 struct HierarchicalVisitor {
     bool any = false;
 
@@ -555,6 +571,11 @@ std::optional<bitwidth_t> Expression::getEffectiveWidth() const {
     return visit(visitor);
 }
 
+bool Expression::getEffectiveSign() const {
+    EffectiveSignVisitor visitor;
+    return visit(visitor);
+}
+
 const Symbol* Expression::getSymbolReference(bool allowPacked) const {
     switch (kind) {
         case ExpressionKind::NamedValue:

diff --git a/source/ast/expressions/AssignmentExpressions.cpp b/source/ast/expressions/AssignmentExpressions.cpp
@@ -886,12 +886,12 @@ Expression& ConversionExpression::makeImplicit(const ASTContext& context, const
 
 ConstantValue ConversionExpression::evalImpl(EvalContext& context) const {
     return convert(context, *operand().type, *type, sourceRange, operand().eval(context),
-                   conversionKind);
+                   conversionKind, &operand());
 }
 
 ConstantValue ConversionExpression::convert(EvalContext& context, const Type& from, const Type& to,
                                             SourceRange sourceRange, ConstantValue&& value,
-                                            ConversionKind conversionKind) {
+                                            ConversionKind conversionKind, const Expression* expr) {
     if (!value)
         return nullptr;
 
@@ -914,15 +914,19 @@ ConstantValue ConversionExpression::convert(EvalContext& context, const Type& fr
         auto result = value.convertToInt(to.getBitWidth(), to.isSigned(), to.isFourState());
         if (conversionKind == ConversionKind::Implicit) {
             if (value.isInteger()) {
-                // We warn if the conversion changes the value, but not if
-                // that value change is only due to a sign conversion.
-                // (Sign conversion warnings are handled elsewhere).
                 auto& oldInt = value.integer();
                 auto& newInt = result.integer();
-                if (!oldInt.hasUnknown() && !newInt.hasUnknown() &&
-                    oldInt.getBitWidth() != newInt.getBitWidth() && oldInt != newInt) {
-                    context.addDiag(diag::ConstantConversion, sourceRange)
-                        << from << to << oldInt << newInt;
+                if (!oldInt.hasUnknown() && !newInt.hasUnknown() && oldInt != newInt) {
+                    if (oldInt.getBitWidth() != newInt.getBitWidth()) {
+                        context.addDiag(diag::ConstantConversion, sourceRange)
+                            << from << to << oldInt << newInt;
+                    }
+                    else {
+                        SLANG_ASSERT(oldInt.isSigned() != newInt.isSigned());
+                        if (!expr || expr->getEffectiveSign() != newInt.isSigned()) {
+                            context.addDiag(diag::SignConversion, sourceRange) << from << to;
+                        }
+                    }
                 }
             }
             else if (value.isReal() || value.isShortReal()) {
@@ -1016,6 +1020,12 @@ std::optional<bitwidth_t> ConversionExpression::getEffectiveWidthImpl() const {
     return type->getBitWidth();
 }
 
+bool ConversionExpression::getEffectiveSignImpl() const {
+    if (isImplicit())
+        return operand().getEffectiveSign();
+    return type->isSigned();
+}
+
 void ConversionExpression::serializeTo(ASTSerializer& serializer) const {
     serializer.write("operand", operand());
 }

diff --git a/source/ast/expressions/LiteralExpressions.cpp b/source/ast/expressions/LiteralExpressions.cpp
@@ -85,6 +85,18 @@ std::optional<bitwidth_t> IntegerLiteral::getEffectiveWidthImpl() const {
     return val.getActiveBits();
 }
 
+bool IntegerLiteral::getEffectiveSignImpl() const {
+    // We will say that this literal could have been signed if doing
+    // so would not change its logical value (i.e. it either already
+    // was marked signed, or it's a positive value that would remain
+    // positive given a cast to a signed type).
+    auto&& val = getValue();
+    if (val.isSigned())
+        return true;
+
+    return val.getActiveBits() < type->getBitWidth() - 1;
+}
+
 void IntegerLiteral::serializeTo(ASTSerializer& serializer) const {
     serializer.write("value", getValue());
 }
@@ -179,6 +191,13 @@ std::optional<bitwidth_t> UnbasedUnsizedIntegerLiteral::getEffectiveWidthImpl()
     return 1;
 }
 
+bool UnbasedUnsizedIntegerLiteral::getEffectiveSignImpl() const {
+    // We don't really want warnings for things like this:
+    // logic signed [1:0] k = '1;
+    // ...so we'll just say this could always be signed.
+    return true;
+}
+
 void UnbasedUnsizedIntegerLiteral::serializeTo(ASTSerializer& serializer) const {
     serializer.write("value", getValue());
 }

diff --git a/source/ast/expressions/MiscExpressions.cpp b/source/ast/expressions/MiscExpressions.cpp
@@ -1278,6 +1278,10 @@ std::optional<bitwidth_t> MinTypMaxExpression::getEffectiveWidthImpl() const {
     return selected().getEffectiveWidth();
 }
 
+bool MinTypMaxExpression::getEffectiveSignImpl() const {
+    return selected().getEffectiveSign();
+}
+
 void MinTypMaxExpression::serializeTo(ASTSerializer& serializer) const {
     serializer.write("selected", selected());
 }

diff --git a/source/ast/expressions/OperatorExpressions.cpp b/source/ast/expressions/OperatorExpressions.cpp
@@ -450,6 +450,17 @@ std::optional<bitwidth_t> UnaryExpression::getEffectiveWidthImpl() const {
     }
 }
 
+bool UnaryExpression::getEffectiveSignImpl() const {
+    switch (op) {
+        case UnaryOperator::Plus:
+        case UnaryOperator::Minus:
+        case UnaryOperator::BitwiseNot:
+            return operand().getEffectiveSign();
+        default:
+            return type->isSigned();
+    }
+}
+
 ConstantValue UnaryExpression::evalImpl(EvalContext& context) const {
     // Handle operations that require an lvalue up front.
     if (isLValueOp(op)) {
@@ -871,15 +882,66 @@ std::optional<bitwidth_t> BinaryExpression::getEffectiveWidthImpl() const {
         case BinaryOperator::BinaryXor:
         case BinaryOperator::BinaryXnor:
             return std::max(left().getEffectiveWidth(), right().getEffectiveWidth());
+        case BinaryOperator::Equality:
+        case BinaryOperator::Inequality:
+        case BinaryOperator::CaseEquality:
+        case BinaryOperator::CaseInequality:
+        case BinaryOperator::GreaterThanEqual:
+        case BinaryOperator::GreaterThan:
+        case BinaryOperator::LessThanEqual:
+        case BinaryOperator::LessThan:
+        case BinaryOperator::WildcardEquality:
+        case BinaryOperator::WildcardInequality:
+        case BinaryOperator::LogicalAnd:
+        case BinaryOperator::LogicalOr:
+        case BinaryOperator::LogicalImplication:
+        case BinaryOperator::LogicalEquivalence:
+            return 1;
         case BinaryOperator::LogicalShiftLeft:
         case BinaryOperator::LogicalShiftRight:
         case BinaryOperator::ArithmeticShiftLeft:
         case BinaryOperator::ArithmeticShiftRight:
         case BinaryOperator::Power:
             return left().getEffectiveWidth();
-        default:
-            return type->getBitWidth();
     }
+    SLANG_UNREACHABLE;
+}
+
+bool BinaryExpression::getEffectiveSignImpl() const {
+    switch (op) {
+        case BinaryOperator::Add:
+        case BinaryOperator::Subtract:
+        case BinaryOperator::Multiply:
+        case BinaryOperator::Divide:
+        case BinaryOperator::Mod:
+        case BinaryOperator::BinaryAnd:
+        case BinaryOperator::BinaryOr:
+        case BinaryOperator::BinaryXor:
+        case BinaryOperator::BinaryXnor:
+            return left().getEffectiveSign() && right().getEffectiveSign();
+        case BinaryOperator::Equality:
+        case BinaryOperator::Inequality:
+        case BinaryOperator::CaseEquality:
+        case BinaryOperator::CaseInequality:
+        case BinaryOperator::GreaterThanEqual:
+        case BinaryOperator::GreaterThan:
+        case BinaryOperator::LessThanEqual:
+        case BinaryOperator::LessThan:
+        case BinaryOperator::WildcardEquality:
+        case BinaryOperator::WildcardInequality:
+        case BinaryOperator::LogicalAnd:
+        case BinaryOperator::LogicalOr:
+        case BinaryOperator::LogicalImplication:
+        case BinaryOperator::LogicalEquivalence:
+            return true;
+        case BinaryOperator::LogicalShiftLeft:
+        case BinaryOperator::LogicalShiftRight:
+        case BinaryOperator::ArithmeticShiftLeft:
+        case BinaryOperator::ArithmeticShiftRight:
+        case BinaryOperator::Power:
+            return left().getEffectiveSign();
+    }
+    SLANG_UNREACHABLE;
 }
 
 ConstantValue BinaryExpression::evalImpl(EvalContext& context) const {
@@ -1076,9 +1138,17 @@ bool ConditionalExpression::propagateType(const ASTContext& context, const Type&
 }
 
 std::optional<bitwidth_t> ConditionalExpression::getEffectiveWidthImpl() const {
+    if (auto branch = knownSide())
+        return branch->getEffectiveWidth();
     return std::max(left().getEffectiveWidth(), right().getEffectiveWidth());
 }
 
+bool ConditionalExpression::getEffectiveSignImpl() const {
+    if (auto branch = knownSide())
+        return branch->getEffectiveSign();
+    return left().getEffectiveSign() && right().getEffectiveSign();
+}
+
 ConstantValue ConditionalExpression::evalImpl(EvalContext& context) const {
     ConstantValue cp;
     for (auto& cond : conditions) {

diff --git a/tests/unittests/ast/EvalTests.cpp b/tests/unittests/ast/EvalTests.cpp
@@ -1953,7 +1953,7 @@ typedef union {
 } u_t;
 
 function u_t f;
-    f.b = 8'b10011100;
+    f.b = byte'(8'b10011100);
 endfunction
 
 function u_t g;

diff --git a/tests/unittests/ast/MemberTests.cpp b/tests/unittests/ast/MemberTests.cpp
@@ -1038,7 +1038,7 @@ module m;
     parameter [9:1] a = 9'b0;
     parameter b = '1;
     parameter c = 3.4;
-    parameter signed d = 2'b10;
+    parameter signed d = 2'b01;
     parameter signed e = 3.4;
     parameter unsigned f = 3.4;
     parameter signed [3:5] g = 3.4;

diff --git a/tests/unittests/ast/WarningTests.cpp b/tests/unittests/ast/WarningTests.cpp
@@ -705,3 +705,26 @@ endmodule
     REQUIRE(diags.size() == 1);
     CHECK(diags[0].code == diag::ImplicitConvert);
 }
+
+TEST_CASE("Sign conversion warnings in constexprs") {
+    auto tree = SyntaxTree::fromText(R"(
+module m;
+    typedef logic [1:0] l2;
+    logic signed [1:0] i = l2'(3);
+    logic signed [1:0] j = 2'd3;
+    logic signed [1:0] k = '1;
+    logic signed [1:0] l = (2'd3:2'd1:-3);
+    logic signed m = (2'd1 == 2'd1);
+    logic signed [1:0] n = 2'd1 << 1;
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+
+    auto& diags = compilation.getAllDiagnostics();
+    REQUIRE(diags.size() == 3);
+    CHECK(diags[0].code == diag::SignConversion);
+    CHECK(diags[0].code == diag::SignConversion);
+    CHECK(diags[0].code == diag::SignConversion);
+}