Fix non-determinism in debuginfo

[ilovepi]

Assignment tracking iterates over a SmallSet when adding metadata, which eventually results in debug metadata being added to the module in non-deterministic order.

As reported in #63921, we saw some cases where DWARF DebugLoc entries could have their order reversed, even though there was no functional difference.

This patch replaces the SmallSet with a SmallVector, and adds the required DenseMapInfo specialization to make the ordering deterministic.

Fixes #63921

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-ir

Changes

Assignment tracking iterates over a SmallSet when adding metadata, which eventually results in debug metadata being added to the module in non-deterministic order.

As reported in #63921, we saw some cases where DWARF DebugLoc entries could have their order reversed, even though there was no functional difference.

This patch replaces the SmallSet with a SmallVector, and adds the required DenseMapInfo specialization to make the ordering deterministic.

Fixes #63921

---

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

1 Files Affected:

• (modified) llvm/include/llvm/IR/DebugInfo.h (+23-1)

diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h
index 26a7cfbbb350234..c5ded71406d885e 100644
--- a/llvm/include/llvm/IR/DebugInfo.h
+++ b/llvm/include/llvm/IR/DebugInfo.h
@@ -16,7 +16,9 @@
 #ifndef LLVM_IR_DEBUGINFO_H
 #define LLVM_IR_DEBUGINFO_H
 
+#include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
@@ -263,7 +265,8 @@ struct VarRecord {
 /// TODO: Backing storage shouldn't be limited to allocas only. Some local
 /// variables have their storage allocated by the calling function (addresses
 /// passed in with sret & byval parameters).
-using StorageToVarsMap = DenseMap<const AllocaInst *, SmallSet<VarRecord, 2>>;
+using StorageToVarsMap =
+    DenseMap<const AllocaInst *, SmallSetVector<VarRecord, 2>>;
 
 /// Track assignments to \p Vars between \p Start and \p End.
 
@@ -314,6 +317,25 @@ class AssignmentTrackingPass : public PassInfoMixin<AssignmentTrackingPass> {
 
 /// Return true if assignment tracking is enabled for module \p M.
 bool isAssignmentTrackingEnabled(const Module &M);
+
+template <> struct DenseMapInfo<at::VarRecord> {
+  static inline at::VarRecord getEmptyKey() {
+    return at::VarRecord{nullptr, nullptr};
+  }
+
+  static inline at::VarRecord getTombstoneKey() {
+    return at::VarRecord{nullptr, nullptr};
+  }
+
+  static unsigned getHashValue(const at::VarRecord &Var) {
+    return hash_combine(Var.Var, Var.DL);
+  }
+
+  static bool isEqual(const at::VarRecord &A, const at::VarRecord &B) {
+    return A == B;
+  }
+};
+
 } // end namespace llvm
 
 #endif // LLVM_IR_DEBUGINFO_H

[ilovepi]

I tried to think of a way to test this, but since its a non-determinism issue, the only way I was reliably able to trigger this was to LD_PRELOAD a different default allocator, which isn't something I think we can really do. If there is a good way to add a test for this, I'm happy to add one.

[OCHyams]

Thanks for hunting this down. LGTM with an inline comment. I'll let @fangism accept when they're happy.

[OCHyams]

(apparently I didn't submit my comments)

[fangism]

I'm not familiar enough with the code base for my approval to be worth anything, but nevertheless I am thrilled this is getting fixed!

I only wish there was a better way to test for determinism in isolation.

[ilovepi]

huh, not sure why check-llvm was passing for me locally, since it looks like the change w/ std::pair is just broken.

I'm probably going to go back to the DenseMapInfo Specialization, unless this ends up being simple to fix.