[msan] Eliminate non-deterministic behavior in the pass

[vitalybuka]

Almost NFC, instrumentation is as correct as it was before.

We need InstrumentationList grouped by origin instruction,
so we used stable_sort. However these objects already grouped
because we never interleave sequences of insertShadowCheck
of different instrunction.

Pointer sort has artifact that it was deppendent on allocator behavior,
so we could inserted checks in a different order.

There is no test, as I failed to reproduce this with opt. My guess
is that for reproducer we need to increase fragmentation in the
allocator.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

@llvm/pr-subscribers-llvm-transforms

Author: Vitaly Buka (vitalybuka)

Changes

Almost NFC, instrumentation is as correct as it was before.

We need InstrumentationList grouped by origin instruction,
so we used stable_sort. However these objects already grouped
because we never interleave sequences of insertShadowCheck
of different instrunction.

Pointer sort has artifact that it was deppendent on allocator behavior,
so we could inserted checks in a different order.

There is no test, as I failed to reproduce this with opt. My guess
is that for reproducer we need to increase fragmentation in the
allocator.

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp (+12-10)

diff --git a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
index 824cbee4eca578..bfa10baeb8697a 100644
--- a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -152,6 +152,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
@@ -1464,19 +1465,20 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
   }
 
   void materializeChecks() {
-    llvm::stable_sort(InstrumentationList,
-                      [](const ShadowOriginAndInsertPoint &L,
-                         const ShadowOriginAndInsertPoint &R) {
-                        return L.OrigIns < R.OrigIns;
-                      });
+#ifndef NDEBUG
+    SmallPtrSet<Instruction *, 16> Done;
+#endif
 
     for (auto I = InstrumentationList.begin();
          I != InstrumentationList.end();) {
-      auto J =
-          std::find_if(I + 1, InstrumentationList.end(),
-                       [L = I->OrigIns](const ShadowOriginAndInsertPoint &R) {
-                         return L != R.OrigIns;
-                       });
+      auto OrigIns = I->OrigIns;
+      // Checks are grouped by the original instruction. We call all
+      // `insertShadowCheck` for an instruction atonce.
+      //assert(Done.insert(OrigIns).second);
+      auto J = std::find_if(I + 1, InstrumentationList.end(),
+                            [OrigIns](const ShadowOriginAndInsertPoint &R) {
+                              return OrigIns != R.OrigIns;
+                            });
       // Process all checks of instruction at once.
       materializeInstructionChecks(ArrayRef<ShadowOriginAndInsertPoint>(I, J));
       I = J;

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[MaskRay]

There is no test, as I failed to reproduce this with opt. My guess is that for reproducer we need to increase fragmentation in the allocator.

Perhaps changing the memory allocator with LD_PRELOAD can more reliably reproduce, but official bots using glibc may not be idiosyncratic enough to reproduce.