From bad7cc5eaa1ca0dfd04f92ecb053ff6ed93daba7 Mon Sep 17 00:00:00 2001 From: Paul Kirth Date: Wed, 4 Oct 2023 16:21:19 -0700 Subject: [PATCH 1/4] Fix non-determinism in debuginfo 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. Fixes: #63921 --- llvm/include/llvm/IR/DebugInfo.h | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h index 26a7cfbbb3502..c5ded71406d88 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>; +using StorageToVarsMap = + DenseMap>; /// Track assignments to \p Vars between \p Start and \p End. @@ -314,6 +317,25 @@ class AssignmentTrackingPass : public PassInfoMixin { /// Return true if assignment tracking is enabled for module \p M. bool isAssignmentTrackingEnabled(const Module &M); + +template <> struct DenseMapInfo { + 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 From 81ad540477ef1b27b63fe5448400ea1b49c39a4e Mon Sep 17 00:00:00 2001 From: Paul Kirth Date: Thu, 5 Oct 2023 14:05:09 -0700 Subject: [PATCH 2/4] [llvm][debuginfo] Use DenseMapInfo keys from component types The naive implementation would break DenseMap, and yield wrong results. Instead use the Tobstone and Empty Keys from the field types of VarRecord. --- llvm/include/llvm/IR/DebugInfo.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h index c5ded71406d88..42bfba1c085b7 100644 --- a/llvm/include/llvm/IR/DebugInfo.h +++ b/llvm/include/llvm/IR/DebugInfo.h @@ -320,11 +320,13 @@ bool isAssignmentTrackingEnabled(const Module &M); template <> struct DenseMapInfo { static inline at::VarRecord getEmptyKey() { - return at::VarRecord{nullptr, nullptr}; + return at::VarRecord{DenseMapInfo::getEmptyKey(), + DenseMapInfo::getEmptyKey()}; } static inline at::VarRecord getTombstoneKey() { - return at::VarRecord{nullptr, nullptr}; + return at::VarRecord{DenseMapInfo::getTombstoneKey(), + DenseMapInfo::getTombstoneKey()}; } static unsigned getHashValue(const at::VarRecord &Var) { From 4f6d66ce1490d54920f006cd6aa46d822bc9fc4d Mon Sep 17 00:00:00 2001 From: Paul Kirth Date: Fri, 6 Oct 2023 09:54:45 -0700 Subject: [PATCH 3/4] Replace VarRecord implementation with std::pair as suggested --- llvm/include/llvm/IR/DebugInfo.h | 35 +------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-) diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h index 42bfba1c085b7..4720290e9a666 100644 --- a/llvm/include/llvm/IR/DebugInfo.h +++ b/llvm/include/llvm/IR/DebugInfo.h @@ -246,20 +246,7 @@ bool calculateFragmentIntersect( /// explicit using types. In addition, eventually we will want to understand /// expressions that modify the base address too, which a DebugVariable doesn't /// capture. -struct VarRecord { - DILocalVariable *Var; - DILocation *DL; - - VarRecord(DbgVariableIntrinsic *DVI) - : Var(DVI->getVariable()), DL(getDebugValueLoc(DVI)) {} - VarRecord(DILocalVariable *Var, DILocation *DL) : Var(Var), DL(DL) {} - friend bool operator<(const VarRecord &LHS, const VarRecord &RHS) { - return std::tie(LHS.Var, LHS.DL) < std::tie(RHS.Var, RHS.DL); - } - friend bool operator==(const VarRecord &LHS, const VarRecord &RHS) { - return std::tie(LHS.Var, LHS.DL) == std::tie(RHS.Var, RHS.DL); - } -}; +using VarRecord = std::pair; /// Map of backing storage to a set of variables that are stored to it. /// TODO: Backing storage shouldn't be limited to allocas only. Some local @@ -318,26 +305,6 @@ class AssignmentTrackingPass : public PassInfoMixin { /// Return true if assignment tracking is enabled for module \p M. bool isAssignmentTrackingEnabled(const Module &M); -template <> struct DenseMapInfo { - static inline at::VarRecord getEmptyKey() { - return at::VarRecord{DenseMapInfo::getEmptyKey(), - DenseMapInfo::getEmptyKey()}; - } - - static inline at::VarRecord getTombstoneKey() { - return at::VarRecord{DenseMapInfo::getTombstoneKey(), - DenseMapInfo::getTombstoneKey()}; - } - - 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 From 9ec25137d218750712e8e700263fea4238af3a9b Mon Sep 17 00:00:00 2001 From: Paul Kirth Date: Fri, 6 Oct 2023 12:27:36 -0700 Subject: [PATCH 4/4] Restore VarRecord, to minimize changes across the codebase. --- llvm/include/llvm/IR/DebugInfo.h | 38 +++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h index 4720290e9a666..92beebed8ad51 100644 --- a/llvm/include/llvm/IR/DebugInfo.h +++ b/llvm/include/llvm/IR/DebugInfo.h @@ -246,8 +246,44 @@ bool calculateFragmentIntersect( /// explicit using types. In addition, eventually we will want to understand /// expressions that modify the base address too, which a DebugVariable doesn't /// capture. -using VarRecord = std::pair; +struct VarRecord { + DILocalVariable *Var; + DILocation *DL; + + VarRecord(DbgVariableIntrinsic *DVI) + : Var(DVI->getVariable()), DL(getDebugValueLoc(DVI)) {} + VarRecord(DILocalVariable *Var, DILocation *DL) : Var(Var), DL(DL) {} + friend bool operator<(const VarRecord &LHS, const VarRecord &RHS) { + return std::tie(LHS.Var, LHS.DL) < std::tie(RHS.Var, RHS.DL); + } + friend bool operator==(const VarRecord &LHS, const VarRecord &RHS) { + return std::tie(LHS.Var, LHS.DL) == std::tie(RHS.Var, RHS.DL); + } +}; + +} // namespace at + +template <> struct DenseMapInfo { + static inline at::VarRecord getEmptyKey() { + return at::VarRecord(DenseMapInfo::getEmptyKey(), + DenseMapInfo::getEmptyKey()); + } + + static inline at::VarRecord getTombstoneKey() { + return at::VarRecord(DenseMapInfo::getTombstoneKey(), + DenseMapInfo::getTombstoneKey()); + } + + 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; + } +}; + +namespace at { /// Map of backing storage to a set of variables that are stored to it. /// TODO: Backing storage shouldn't be limited to allocas only. Some local /// variables have their storage allocated by the calling function (addresses