Skip to content

Conversation

@steakhal
Copy link
Contributor

Reverts #115615

There are two problems with this PR:

  1. If any of the dumps contains a store with a symbolic binding, we crash.
  2. The memory space clusters come last among the clusters, which is not what I intended.

I'm reverting because of the crash.

@steakhal steakhal merged commit 469520e into main Nov 12, 2024
4 of 7 checks passed
@steakhal steakhal deleted the revert-115615-bb/region-store-deterministic-dumps branch November 12, 2024 15:24
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Nov 12, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 12, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balazs Benics (steakhal)

Changes

Reverts llvm/llvm-project#115615

There are two problems with this PR:

  1. If any of the dumps contains a store with a symbolic binding, we crash.
  2. The memory space clusters come last among the clusters, which is not what I intended.

I'm reverting because of the crash.


Full diff: https://github.com/llvm/llvm-project/pull/115881.diff

1 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Core/RegionStore.cpp (+13-61)
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 085f0ef9a5fb96..674099dd7e1f0f 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -67,8 +67,8 @@ class BindingKey {
             isa<ObjCIvarRegion, CXXDerivedObjectRegion>(r)) &&
            "Not a base");
   }
-
 public:
+
   bool isDirect() const { return P.getInt() & Direct; }
   bool hasSymbolicOffset() const { return P.getInt() & Symbolic; }
 
@@ -232,75 +232,27 @@ class RegionBindingsRef : public llvm::ImmutableMapRef<const MemRegion *,
 
   void printJson(raw_ostream &Out, const char *NL = "\n",
                  unsigned int Space = 0, bool IsDot = false) const {
-    using namespace llvm;
-    DenseMap<const MemRegion *, std::string> StringifyCache;
-    auto ToString = [&StringifyCache](const MemRegion *R) {
-      auto [Place, Inserted] = StringifyCache.try_emplace(R);
-      if (!Inserted)
-        return Place->second;
-      std::string Res;
-      raw_string_ostream OS(Res);
-      OS << R;
-      Place->second = Res;
-      return Res;
-    };
-
-    using Cluster =
-        std::pair<const MemRegion *, ImmutableMap<BindingKey, SVal>>;
-    using Binding = std::pair<BindingKey, SVal>;
-
-    const auto ClusterSortKey = [&ToString](const Cluster *C) {
-      const MemRegion *Key = C->first;
-      return std::tuple{isa<MemSpaceRegion>(Key), ToString(Key)};
-    };
-
-    const auto MemSpaceBeforeRegionName = [&ClusterSortKey](const Cluster *L,
-                                                            const Cluster *R) {
-      return ClusterSortKey(L) < ClusterSortKey(R);
-    };
-
-    const auto BindingSortKey = [&ToString](const Binding *BPtr) {
-      const BindingKey &Key = BPtr->first;
-      return std::tuple{Key.isDirect(), !Key.hasSymbolicOffset(),
-                        ToString(Key.getRegion()), Key.getOffset()};
-    };
-
-    const auto DefaultBindingBeforeDirectBindings =
-        [&BindingSortKey](const Binding *LPtr, const Binding *RPtr) {
-          return BindingSortKey(LPtr) < BindingSortKey(RPtr);
-        };
-
-    const auto AddrOf = [](const auto &Item) { return &Item; };
-
-    std::vector<const Cluster *> SortedClusters;
-    SortedClusters.reserve(std::distance(begin(), end()));
-    append_range(SortedClusters, map_range(*this, AddrOf));
-    llvm::sort(SortedClusters, MemSpaceBeforeRegionName);
-
-    for (auto [Idx, C] : llvm::enumerate(SortedClusters)) {
-      const auto &[BaseRegion, Bindings] = *C;
+    for (iterator I = begin(), E = end(); I != E; ++I) {
+      // TODO: We might need a .printJson for I.getKey() as well.
       Indent(Out, Space, IsDot)
-          << "{ \"cluster\": \"" << BaseRegion << "\", \"pointer\": \""
-          << (const void *)BaseRegion << "\", \"items\": [" << NL;
-
-      std::vector<const Binding *> SortedBindings;
-      SortedBindings.reserve(std::distance(Bindings.begin(), Bindings.end()));
-      append_range(SortedBindings, map_range(Bindings, AddrOf));
-      llvm::sort(SortedBindings, DefaultBindingBeforeDirectBindings);
+          << "{ \"cluster\": \"" << I.getKey() << "\", \"pointer\": \""
+          << (const void *)I.getKey() << "\", \"items\": [" << NL;
 
       ++Space;
-      for (auto [Idx, B] : llvm::enumerate(SortedBindings)) {
-        const auto &[Key, Value] = *B;
-        Indent(Out, Space, IsDot) << "{ " << Key << ", \"value\": ";
-        Value.printJson(Out, /*AddQuotes=*/true);
+      const ClusterBindings &CB = I.getData();
+      for (ClusterBindings::iterator CI = CB.begin(), CE = CB.end(); CI != CE;
+           ++CI) {
+        Indent(Out, Space, IsDot) << "{ " << CI.getKey() << ", \"value\": ";
+        CI.getData().printJson(Out, /*AddQuotes=*/true);
         Out << " }";
-        if (Idx != SortedBindings.size() - 1)
+        if (std::next(CI) != CE)
           Out << ',';
         Out << NL;
       }
+
       --Space;
       Indent(Out, Space, IsDot) << "]}";
-      if (Idx != SortedClusters.size() - 1)
+      if (std::next(I) != E)
         Out << ',';
       Out << NL;
     }

@steakhal
Copy link
Contributor Author

We crash because even calling the getRegion() raises an assert that the binding should be a symbolic binding.

@llvm-ci
Copy link
Collaborator

llvm-ci commented Nov 12, 2024

LLVM Buildbot has detected a new failure on builder libc-x86_64-debian-dbg running on libc-x86_64-debian while building clang at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/93/builds/9969

Here is the relevant piece of the build log for the reference
Step 4 (annotate) failure: 'python ../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py ...' (failure)
...
[       OK ] LlvmLibcFStatTest.NonExistentFile (3 us)
Ran 2 tests.  PASS: 2  FAIL: 0
[919/984] Running unit test libc.test.src.sys.statvfs.linux.statvfs_test
[==========] Running 2 tests from 1 test suite.
[ RUN      ] LlvmLibcSysStatvfsTest.StatvfsBasic
[       OK ] LlvmLibcSysStatvfsTest.StatvfsBasic (17 us)
[ RUN      ] LlvmLibcSysStatvfsTest.StatvfsInvalidPath
[       OK ] LlvmLibcSysStatvfsTest.StatvfsInvalidPath (101 us)
Ran 2 tests.  PASS: 2  FAIL: 0
[920/984] Running unit test libc.test.src.sys.statvfs.linux.fstatvfs_test
FAILED: projects/libc/test/src/sys/statvfs/linux/CMakeFiles/libc.test.src.sys.statvfs.linux.fstatvfs_test /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian/libc-x86_64-debian-dbg/build/projects/libc/test/src/sys/statvfs/linux/CMakeFiles/libc.test.src.sys.statvfs.linux.fstatvfs_test 
cd /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian/libc-x86_64-debian-dbg/build/projects/libc/test/src/sys/statvfs/linux && /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian/libc-x86_64-debian-dbg/build/projects/libc/test/src/sys/statvfs/linux/libc.test.src.sys.statvfs.linux.fstatvfs_test.__build__
[==========] Running 2 tests from 1 test suite.
[ RUN      ] LlvmLibcSysFStatvfsTest.FStatvfsBasic
[       OK ] LlvmLibcSysFStatvfsTest.FStatvfsBasic (25 us)
[ RUN      ] LlvmLibcSysFStatvfsTest.FStatvfsInvalidPath
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian/libc-x86_64-debian-dbg/llvm-project/libc/test/src/sys/statvfs/linux/fstatvfs_test.cpp:40: FAILURE
Failed to match LIBC_NAMESPACE::mkdirat(AT_FDCWD, TEST_DIR, S_IRWXU) against Succeeds(0).
Expected return value to be equal to 0 but got -1.
Expected errno to be equal to "Success" but got "File exists".
[  FAILED  ] LlvmLibcSysFStatvfsTest.FStatvfsInvalidPath
Ran 2 tests.  PASS: 1  FAIL: 1
[921/984] Running unit test libc.test.src.sys.utsname.uname_test
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcUnameTest.GetMachineName
[       OK ] LlvmLibcUnameTest.GetMachineName (7 us)
Ran 1 tests.  PASS: 1  FAIL: 0
[922/984] Running unit test libc.test.src.sys.wait.waitpid_test
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcWaitPidTest.NoHangTest
[       OK ] LlvmLibcWaitPidTest.NoHangTest (4 us)
Ran 1 tests.  PASS: 1  FAIL: 0
[923/984] Running unit test libc.test.src.sys.wait.wait4_test
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcwait4Test.NoHangTest
[       OK ] LlvmLibcwait4Test.NoHangTest (4 us)
Ran 1 tests.  PASS: 1  FAIL: 0
[924/984] Running unit test libc.test.src.sys.prctl.linux.prctl_test
[==========] Running 2 tests from 1 test suite.
[ RUN      ] LlvmLibcSysPrctlTest.GetSetName
[       OK ] LlvmLibcSysPrctlTest.GetSetName (12 us)
[ RUN      ] LlvmLibcSysPrctlTest.GetTHPDisable
[       OK ] LlvmLibcSysPrctlTest.GetTHPDisable (5 us)
Ran 2 tests.  PASS: 2  FAIL: 0
[925/984] Running unit test libc.test.src.sys.auxv.linux.getauxval_test
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcGetauxvalTest.Basic
[       OK ] LlvmLibcGetauxvalTest.Basic (43 us)
Ran 1 tests.  PASS: 1  FAIL: 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:static analyzer clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants