[analyzer]Skip unstable CSA tests failing on several platforms

[stevewan]

Summary

Clang static analyzer uses bitwidth to infer the integer value type, that is, any 32-bit integer is considered of type int, and any 64-bit integer is considered of type long. This isn't always true, for instance, in ILP32 (e.g., 32-bit AIX), 32-bit could be long, and in LP64 (e.g., 64-bit wasm64), 64-bit could be long long.

[martong]

vsavchenko is inactive, presumably he is no longer working with CSA

[steakhal]

Thanks for fixing this @stevewan!

I think disabling a test is worse than actually fixing it, although I can see the frustration it causes.
Yet, I would propose something else.

This is just a static analyzer checker doing its own thing. What if we were pinning the target triple before conducting the analysis run? The platform on which the test runs shouldn't matter.
Or even better, setting a bunch of target triples and letting gtest to run each one to see if it still passes on all triples.

I haven't checked, but I think clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp does something similar by using the INSTANTIATE_TEST_SUITE_P macro.

---

If you insist, I think a conditional GTEST_SKIP() << "my message"; should be prefered to simply disable a test. By doing this, you can specify the reason why was the test disabled.
That being said, the macro stuff would get much cleaner this way.
But I still prefer pinning the triple. See my previous comment.

[steakhal]

I think D21810 (commit) was related to a triple issue in the past.
Let me summon them. @chandlerc @jlebar

[jlebar]

Let me summon them.

*I have no memory of this place.*

Sorry to disappoint. That blame was four years ago, not counting covid-related time dilation.

I do agree that disabling a test just to please a static analyzer is probably not the best way to go.

[stevewan]

Pin target triple before analysis

[stevewan]

Finally had some time to get back to this. Thanks @steakhal for your suggestion, the sample code in ASTMatchersNodeTest.cpp was helpful. I was actually looking for a good way of querying the target triple as I do agree that it is an better way of fixing this. Hopefully I understood what you had suggested correctly.

Also by

setting a bunch of target triples and letting gtest to run each one to see if it still passes on all triples.

Did you mean adding all the triples that we're interested in to allTestClangConfigs()? If so, do you have a list in mind? Sorry if this is a dumb question, I'm not very familiar with CSA and parameterized gtest.

[steakhal]

In D114454#3162490, @stevewan wrote:

setting a bunch of target triples and letting gtest to run each one to see if it still passes on all triples.

Did you mean adding all the triples that we're interested in to allTestClangConfigs()? If so, do you have a list in mind? Sorry if this is a dumb question, I'm not very familiar with CSA and parameterized gtest.

Yes. I don't know any specific, but we could pick a couple of esoteric targets where integrals differ significantly from what we have on x86_64.
That said, I would test for i686-pc-windows-msvc, i686-apple-darwin9, x86_64-apple-darwin9, x86_64-scei-ps4, x86_64-windows-msvc, x86_64-unknown-linux, x86_64-apple-macosx, x86_64-apple-ios14.0, wasm32-unknown-unknown, wasm64-unknown-unknown, thumb-pc-win32, sparc64-none-openbsd, sparc-none-none, riscv64-unknown-linux, ppc64-windows-msvc, powerpc-ibm-aix, powerpc64-ibm-aix, s390x-ibm-zos, armv7-pc-windows-msvc, aarch64-pc-windows-msvc, xcore-xmos-elf.
Please note that I've just grepped for the triples and picked some interesting-sounding ones. I did nothing scientific. We could change this in the future.

About the content of the test, I think it looks good. It would be nice to have the test case fixed, instead of skipping it but I'm not expecting you to get into the details of doing that. It's not critical.

clang/unittests/StaticAnalyzer/SValTest.cpp

152

Please use GTEST_SKIP() << "on powerpc-ibm-aix int and long has the same bitwidth"; return;

[stevewan]

Add targets we are interested in testing

[stevewan]

but we could pick a couple of esoteric targets where integrals differ significantly from what we have on x86_64

Turns out wasm and darwin also have a similar problem, I added them to the skip list for now.

Error message for reference:

Expected equality of these values:
  Context.getUIntPtrType()
    Which is: unsigned long
  Y.getType(Context)
    Which is: unsigned int
[  FAILED  ] SValTests/SValTest.GetConstType/1, where GetParam() = { Language=4, Target=i686-apple-darwin9 } (31 ms)

Expected equality of these values:
  Context.getUIntPtrType()
    Which is: unsigned long
  Y.getType(Context)
    Which is: unsigned int
[  FAILED  ] SValTests/SValTest.GetConstType/8, where GetParam() = { Language=4, Target=wasm32-unknown-unknown } (21 ms)

Expected equality of these values:
  Context.getUIntPtrType()
    Which is: unsigned long
  Y.getType(Context)
    Which is: unsigned long long
[  FAILED  ] SValTests/SValTest.GetConstType/9, where GetParam() = { Language=4, Target=wasm64-unknown-unknown } (21 ms)

[steakhal]

I think it looks good. Sorry for the delay.
I'll fix the test later, to pass on all targets :)
Thanks again for making it support multiple triples.

[steakhal]

Ah, please retitle this to have the [analyzer] tag. I think we dont use the CSA tag or any other.

clang/unittests/StaticAnalyzer/CMakeLists.txt

clang_target_link_libraries(StaticAnalysisTests
   clangAnalysis
   clangAST
   clangASTMatchers
   clangCrossTU
   clangFrontend
   clangSerialization
   clangStaticAnalyzerCore
   clangStaticAnalyzerFrontend
+  clangTesting
   clangTooling
   )

clang/unittests/StaticAnalyzer/SValTest.cpp

 #include "clang/AST/Type.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "clang/Testing/TestClangConfig.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 
 namespace clang {

 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: This is a hack to work around the fact that there is no way to
+  // differentiate int and long on 32-bit AIX solely based on bitwidth.
+  bool skipOnPPC32AIX(StringRef TestName) {
+    return GetParam().Target == "powerpc-ibm-aix" &&
+           (TestName == "GetConstType" || TestName == "GetLocAsIntType");
+  }
+};
+
 // 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
 // valid C/C++, but have ONLY ONE defined function.  There are no requirements
void add##NAME##SValCollector(AnalysisASTConsumer &AnalysisConsumer,         \
                                 AnalyzerOptions &AnOpts) {                     \
     AnOpts.CheckersAndPackages = {{"test.##NAME##SValCollector", true}};       \
     AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {  \
       Registry.addChecker<NAME##SValCollector>("test.##NAME##SValCollector",   \
                                                "Description", "");             \
     });                                                                        \
   }                                                                            \
                                                                                \
-  TEST(SValTest, NAME) { runCheckerOnCode<add##NAME##SValCollector>(CODE); }   \
+  TEST_P(SValTest, NAME) {                                                     \
+    if (skipOnPPC32AIX(#NAME))                                                 \
+      return;                                                                  \
+    runCheckerOnCodeWithArgs<add##NAME##SValCollector>(                        \
+        CODE, GetParam().getCommandLineArgs());                                \
+  }                                                                            \
   void NAME##SValCollector::test(ExprEngine &Engine,                           \
                                  const ASTContext &Context) const
 
 //===----------------------------------------------------------------------===//
 //                                 Actual tests
 //===----------------------------------------------------------------------===//
 
 SVAL_TEST(GetConstType, R"(
)") {
   EXPECT_EQ(Context.VoidPtrTy, A.getType(Context));
 
   SVal B = getByName("a");
   ASSERT_FALSE(B.getType(Context).isNull());
   // TODO: Change to CharTy when we support symbolic casts
   EXPECT_EQ(Context.VoidPtrTy, B.getType(Context));
 }
 
+std::vector<TestClangConfig> allTestClangConfigs() {
+  std::vector<TestClangConfig> all_configs;
+  TestClangConfig config;
+  config.Language = Lang_CXX14;
+  config.Target = "powerpc-ibm-aix";
+  all_configs.push_back(config);
+  config.Target = "powerpc-ibm-linux";
+  all_configs.push_back(config);
+  return all_configs;
+}
+
+INSTANTIATE_TEST_SUITE_P(SValTests, SValTest,
+                         testing::ValuesIn(allTestClangConfigs()));
+
 } // namespace
 } // namespace ento
 } // namespace clang