[analyzer][NFC] Re-enable skipped SValTests by relaxing expectations

[steakhal]

Summary

Some tests were skipped in D114454 to resolve test failures on some
platforms, where the pointers have different bitwidth than expected.
This patch re-enables these tests, by relaxing the requirements on the
types of the SVal.

The issue:
There is no way to reconstruct the type of the SVal perfectly
accurately, since there could be multiple types having the required
bitwidth and signedness.
Consider platforms where int and long have the same bitwidth.
Additionally, we need to be careful about casting a pointer to an
integral representation, because we don't know what smallest integral
type can represent that.

To workaround these issues, I propose enforcing a type that has the
same signedness and bitwidth as the expected type, instead of perfect
equality.

In the GetLocAsIntType test, in case of pointer-to-integral casts
I'm using the widest standard integral type (long long) to make sure
that the pointer can be represented by the type without losing
precision. This won't affect the test in any meaningful way, since the
type of the lvalue remained the same.

In one case, I had to replace getUIntPtrType() with UnsignedLongTy
because on some platforms getUIntPtrType() is different then `long
int`.

In this patch, I also enforce that the tests must compile without
errors, to prevent narrowing conversions in the future.

[steakhal]

Ping

[stevewan]

clang/unittests/StaticAnalyzer/SValTest.cpp

177–179

Okay this reminded me that I wanted to ask this dumb question. Don't the assignments eventually do the implicit down-casting? Is it fine because we are not dealing with anything concrete and just doing static analysis over the code?

[stevewan]

As emphasized in the description, we can't get the exact type of the SVal, and I agree that checking for the same signedness and bitwidth is our best bet. LGTM given that my question is addressed. Thanks.

[steakhal]

Thanks for the review @stevewan!
Let me know if I managed to answer your question.

clang/unittests/StaticAnalyzer/SValTest.cpp

177–179

I think the problem is that this is a c-style reinterpret cast. And that must operate on equal bitwidth (or casting to something even larger) types.
So, if I recall correctly, this code did not compile on all targetted platforms, thus I decided to use a slightly wider type to accommodate for that.

From the CSA's standpoint, it shouldn't matter. The value of x will be eventually narrowed to a long int.
If the pointer has the same bitwidth as long int, then the widening is compensated by the narrowing, binding a directly to x.
If the pointer has the same bitwidth as long long int, then there is no widening, only a narrowing, thus CSA will bind a to (long int)x.

Anyway, since x is a pointer, and AFAIK CSA models pointers as unsigned values, we should get an unsigned value at the end of the day.
But don't quote me on that :D

[stevewan]

Thanks @steakhal for the clarification. Yes my question is answered.

clang/unittests/StaticAnalyzer/SValTest.cpp

 private:
   /// Entry point for tests.
   virtual void test(ExprEngine &Engine, const ASTContext &Context) const = 0;
 
   mutable bool Tested = false;
   mutable SVals CollectedSVals;
 };
 
-// Fixture class for parameterized SValTest
-class SValTest : public testing::TestWithParam<TestClangConfig> {
-protected:
-  // FIXME: The tests "GetConstType" and "GetLocAsIntType" infer the type of
-  // integrals based on their bitwidth. This is not a reliable method on
-  // platforms where different integrals have the same width.
-  bool skipTest(StringRef TestName) {
-    std::string target = GetParam().Target;
-    return (target == "powerpc-ibm-aix" || target == "i686-apple-darwin9" ||
-            target == "wasm32-unknown-unknown" ||
-            target == "wasm64-unknown-unknown") &&
-           (TestName == "GetConstType" || TestName == "GetLocAsIntType");
-  }
-};
+static void expectSameSignAndBitWidth(QualType ExpectedTy, QualType ActualTy,
+                                      const ASTContext &Context) {
+  EXPECT_EQ(ExpectedTy->isUnsignedIntegerType(),
+            ActualTy->isUnsignedIntegerType());
+  EXPECT_EQ(Context.getTypeSize(ExpectedTy), Context.getTypeSize(ActualTy));
+
+// Fixture class for parameterized SValTest
+class SValTest : public testing::TestWithParam<TestClangConfig> {};
 
 // SVAL_TEST is a combined way of providing a short code snippet and
 // to test some programmatic predicates on symbolic values produced by the
 // engine for the actual code.
 //
 // Each test has a NAME.  One can think of it as a name for normal gtests.
 //
 // Each test should provide a CODE snippet.  Code snippets might contain any
void add##NAME##SValCollector(AnalysisASTConsumer &AnalysisConsumer,         \
     AnOpts.CheckersAndPackages = {{"test.##NAME##SValCollector", true}};       \
     AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {  \
       Registry.addChecker<NAME##SValCollector>("test.##NAME##SValCollector",   \
                                                "Description", "");             \
     });                                                                        \
   }                                                                            \
                                                                                \
   TEST_P(SValTest, NAME) {                                                     \
-    if (skipTest(#NAME)) {                                                     \
-      std::string target = GetParam().Target;                                  \
-      GTEST_SKIP() << "certain integrals have the same bitwidth on "           \
-                   << target;                                                  \
-      return;                                                                  \
-    }                                                                          \
-    runCheckerOnCodeWithArgs<add##NAME##SValCollector>(                        \
-        CODE, GetParam().getCommandLineArgs());                                \
+    EXPECT_TRUE(runCheckerOnCodeWithArgs<add##NAME##SValCollector>(            \
+        CODE, GetParam().getCommandLineArgs()));                               \
   }                                                                            \
   void NAME##SValCollector::test(ExprEngine &Engine,                           \
                                  const ASTContext &Context) const
 
 //===----------------------------------------------------------------------===//
 //                                 Actual tests
 //===----------------------------------------------------------------------===//
 
 SVAL_TEST(GetConstType, R"(
 void foo() {
   int x = 42;
   int *y = nullptr;
 })") {
   SVal X = getByName("x");
   ASSERT_FALSE(X.getType(Context).isNull());
   EXPECT_EQ(Context.IntTy, X.getType(Context));
 
   SVal Y = getByName("y");
   ASSERT_FALSE(Y.getType(Context).isNull());
-  EXPECT_EQ(Context.getUIntPtrType(), Y.getType(Context));
+  expectSameSignAndBitWidth(Context.getUIntPtrType(), Y.getType(Context),
+                            Context);
 }
 
 SVAL_TEST(GetLocAsIntType, R"(
 void foo(int *x) {
-  long int a = (long int)x;
-  unsigned b = (long unsigned)&a;
-  int c = (long int)nullptr;
+  long int a = (long long int)x;
+  unsigned b = (long long unsigned)&a;
+  int c = (long long int)nullptr;
 })") {
   SVal A = getByName("a");
   ASSERT_FALSE(A.getType(Context).isNull());
+
   // TODO: Turn it into signed long
-  EXPECT_EQ(Context.getUIntPtrType(), A.getType(Context));
+  expectSameSignAndBitWidth(Context.UnsignedLongTy, A.getType(Context),
+                            Context);
 
   SVal B = getByName("b");
   ASSERT_FALSE(B.getType(Context).isNull());
-  EXPECT_EQ(Context.UnsignedIntTy, B.getType(Context));
+  expectSameSignAndBitWidth(Context.UnsignedIntTy, B.getType(Context), Context);
 
   SVal C = getByName("c");
   ASSERT_FALSE(C.getType(Context).isNull());
-  EXPECT_EQ(Context.IntTy, C.getType(Context));
+  expectSameSignAndBitWidth(Context.IntTy, C.getType(Context), Context);
 }
 
 SVAL_TEST(GetSymExprType, R"(
 void foo(int a, int b) {
   int x = a;
   int y = a + b;
   long z = a;
 })") {