[llvm] Fix non-determinism in debuginfo (PR #68332)
Paul Kirth via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 6 12:28:42 PDT 2023
https://github.com/ilovepi updated https://github.com/llvm/llvm-project/pull/68332
>From bad7cc5eaa1ca0dfd04f92ecb053ff6ed93daba7 Mon Sep 17 00:00:00 2001
From: Paul Kirth <paulkirth at google.com>
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 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
>From 81ad540477ef1b27b63fe5448400ea1b49c39a4e Mon Sep 17 00:00:00 2001
From: Paul Kirth <paulkirth at google.com>
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 c5ded71406d885e..42bfba1c085b747 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<at::VarRecord> {
static inline at::VarRecord getEmptyKey() {
- return at::VarRecord{nullptr, nullptr};
+ return at::VarRecord{DenseMapInfo<DILocalVariable *>::getEmptyKey(),
+ DenseMapInfo<DILocation *>::getEmptyKey()};
}
static inline at::VarRecord getTombstoneKey() {
- return at::VarRecord{nullptr, nullptr};
+ return at::VarRecord{DenseMapInfo<DILocalVariable *>::getTombstoneKey(),
+ DenseMapInfo<DILocation *>::getTombstoneKey()};
}
static unsigned getHashValue(const at::VarRecord &Var) {
>From 4f6d66ce1490d54920f006cd6aa46d822bc9fc4d Mon Sep 17 00:00:00 2001
From: Paul Kirth <paulkirth at google.com>
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 42bfba1c085b747..4720290e9a66666 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<DILocalVariable *, DILocation *>;
/// 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<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{DenseMapInfo<DILocalVariable *>::getEmptyKey(),
- DenseMapInfo<DILocation *>::getEmptyKey()};
- }
-
- static inline at::VarRecord getTombstoneKey() {
- return at::VarRecord{DenseMapInfo<DILocalVariable *>::getTombstoneKey(),
- DenseMapInfo<DILocation *>::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 <paulkirth at google.com>
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 4720290e9a66666..92beebed8ad51df 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<DILocalVariable *, DILocation *>;
+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<at::VarRecord> {
+ static inline at::VarRecord getEmptyKey() {
+ return at::VarRecord(DenseMapInfo<DILocalVariable *>::getEmptyKey(),
+ DenseMapInfo<DILocation *>::getEmptyKey());
+ }
+
+ static inline at::VarRecord getTombstoneKey() {
+ return at::VarRecord(DenseMapInfo<DILocalVariable *>::getTombstoneKey(),
+ DenseMapInfo<DILocation *>::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
More information about the llvm-commits
mailing list