tomasz-kaminski-sonarsource created this revision.
tomasz-kaminski-sonarsource added reviewers: vsavchenko, NoQ, steakhal.
Herald added a subscriber: martong.
Herald added a project: All.
tomasz-kaminski-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This PR changes the SymIntExpr so the expression that uses
negative value as RHS, for example: x +/- (-N), are modeled as
as x -/+ N. This avoid producing very large RHS in case when the
symbol is cased to unsigned number, and as consequence makes the
value more robust in presence of cast.
This change is not apllied if N is lowest negative value for
which negation would not be representable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124658

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/additive-op-on-sym-int-expr.c
  clang/test/Analysis/expr-inspection.c

Index: clang/test/Analysis/expr-inspection.c
===================================================================
--- clang/test/Analysis/expr-inspection.c
+++ clang/test/Analysis/expr-inspection.c
@@ -11,7 +11,7 @@
 
 void foo(int x) {
   clang_analyzer_dump(x); // expected-warning{{reg_$0<int x>}}
-  clang_analyzer_dump(x + (-1)); // expected-warning{{(reg_$0<int x>) + -1}}
+  clang_analyzer_dump(x + (-1)); // expected-warning{{(reg_$0<int x>) - 1}}
   int y = 1;
   for (; y < 3; ++y) {
     clang_analyzer_numTimesReached(); // expected-warning{{2}}
Index: clang/test/Analysis/additive-op-on-sym-int-expr.c
===================================================================
--- /dev/null
+++ clang/test/Analysis/additive-op-on-sym-int-expr.c
@@ -0,0 +1,53 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config eagerly-assume=false -verify %s
+
+void clang_analyzer_dump(int);
+void clang_analyzer_dumpL(long);
+void clang_analyzer_warnIfReached();
+
+void testInspect(int x) {
+  if ((x < 10) || (x > 100)) {
+    return;
+  }
+
+  int i = x + 1;
+  long l = i - 10U;
+  clang_analyzer_dump(i);       // expected-warning {{(reg_$0<int x>) + 1}}
+  clang_analyzer_dumpL(l);      // expected-warning {{(reg_$0<int x>) - 9U}}
+  clang_analyzer_dumpL(l + 0L); // expected-warning {{(reg_$0<int x>) - 9}}
+  if ((l - 1000) > 0) {
+    clang_analyzer_warnIfReached();
+  }
+  if (l > 1000) {
+    clang_analyzer_warnIfReached();
+  }
+  if (l > 1000L) {
+    clang_analyzer_warnIfReached();
+  }
+  if ((l + 0L) > 1000) {
+    clang_analyzer_warnIfReached();
+  }
+
+  i = x - 1;
+  l = i + 10U;
+  clang_analyzer_dump(i);  // expected-warning {{(reg_$0<int x>) - 1}}
+  clang_analyzer_dumpL(l); // expected-warning {{(reg_$0<int x>) + 9U}}
+
+  l = -1l;
+  i = l;
+  clang_analyzer_dump(x + i); // expected-warning {{(reg_$0<int x>) - 1}}
+}
+
+void testMin(int i, long long l) {
+  clang_analyzer_dump(i + (-1));  // expected-warning {{(reg_$0<int i>) - 1}}
+  clang_analyzer_dump(i - (-1));  // expected-warning {{(reg_$0<int i>) + 1}}
+  clang_analyzer_dumpL(l + (-1)); // expected-warning {{(reg_$1<long long l>) - 1}}
+  clang_analyzer_dumpL(l - (-1)); // expected-warning {{(reg_$1<long long l>) + 1}}
+
+  int intMin = 1 << (sizeof(int) * 8 - 1); // INT_MIN, negative value is not representable
+  // Do not normalize representation if negation would not be representable
+  clang_analyzer_dump(i + intMin); // expected-warning {{(reg_$0<int i>) + -2147483648}}
+  clang_analyzer_dump(i - intMin); // expected-warning {{(reg_$0<int i>) - -2147483648}}
+  // Produced value has higher bit with (long) so negation if representable
+  clang_analyzer_dumpL(l + intMin); // expected-warning {{(reg_$1<long long l>) - 2147483648}}
+  clang_analyzer_dumpL(l - intMin); // expected-warning {{(reg_$1<long long l>) + 2147483648}}
+}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -197,8 +197,30 @@
       if (RHS.isSigned() && !SymbolType->isSignedIntegerOrEnumerationType())
         ConvertedRHS = &BasicVals.Convert(SymbolType, RHS);
     }
-  } else
+  } else if (BinaryOperator::isAdditiveOp(op)) {
+    // Change a+(-N) into a-N, and a-(-N) into a+N
+    // Adjust addition/subtraction of negative value, to
+    // subtraction/addition of the negated value.
+    if (!RHS.isNegative()) {
+      ConvertedRHS = &BasicVals.Convert(resultTy, RHS);
+    } else {
+      APSIntType resultIntTy = BasicVals.getAPSIntType(resultTy);
+      assert(resultIntTy.getBitWidth() >= RHS.getBitWidth() &&
+             "The result operation type must have at least the same "
+             "number of bits as its operands.");
+
+      llvm::APSInt ConvertedRHSValue = resultIntTy.convert(RHS);
+      // Check if the negation of the RHS is representable,
+      // i.e., the resultTy is signed, and it is not the lowest
+      // representable negative value.
+      if (ConvertedRHSValue > resultIntTy.getMinValue()) {
+        ConvertedRHS = &BasicVals.getValue(-ConvertedRHSValue);
+        op = (op == BO_Add) ? BO_Sub : BO_Add;
+      }
+    }
+  } else {
     ConvertedRHS = &BasicVals.Convert(resultTy, RHS);
+  }
 
   return makeNonLoc(LHS, op, *ConvertedRHS, resultTy);
 }
@@ -636,16 +658,27 @@
               const llvm::APSInt &first = IntType.convert(symIntExpr->getRHS());
               const llvm::APSInt &second = IntType.convert(*RHSValue);
 
+              // If the op and lop agrees, then we just need to
+              // sum the constants. Otherwise, we change to operation
+              // type if substraction would produce negative value
+              // (and cause overflow for unsigned integers),
+              // as consequence x+1U-10 produces x-9U, instead
+              // of x+4294967287U, that would be produced without this
+              // additional check.
               const llvm::APSInt *newRHS;
-              if (lop == op)
+              if (lop == op) {
                 newRHS = BasicVals.evalAPSInt(BO_Add, first, second);
-              else
+              } else if (first >= second) {
                 newRHS = BasicVals.evalAPSInt(BO_Sub, first, second);
+                op = lop;
+              } else {
+                newRHS = BasicVals.evalAPSInt(BO_Sub, second, first);
+              }
 
               assert(newRHS && "Invalid operation despite common type!");
               rhs = nonloc::ConcreteInt(*newRHS);
               lhs = nonloc::SymbolVal(symIntExpr->getLHS());
-              op = lop;
+
               continue;
             }
           }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D124658: CSA Norm... Tomasz Kamiński via Phabricator via cfe-commits

Reply via email to