[DLCov 3/5] Implement DebugLoc origin-tracking #107369
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SLTozer
requested review from
jmorse,
adrian-prantl,
dwblaikie,
pogo59 and
djtodoro
llvmbot
added
cmake
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-platform-windows Author: Stephen Tozer (SLTozer) ChangesThis is part of a series of patches that tries to improve DILocation bug detection in Debugify; see below for more details. This patch adds an origin-tracking feature, which collects a stack trace at the point that unintentionally empty DebugLocs are created (e.g. in the Instruction constructor), and storing that trace in the DebugLoc so that Debugify can symbolize it and print it as part of the generated JSON/HTML report for each reported bug. This is obviously very expensive; for the builds I used this with locally, it resulted in compile times around 9x slower. This is not necessarily a problem however, as the goal of this feature is to enable very fast diagnosis of DebugLoc errors by developers on their own machines, not to be part of any automated test environment. Most DebugLoc errors occur when we create an instruction and forget to assign it a DILocation. These errors are trivial to fix once the responsible code has been identified, but finding them can be tedious and time consuming. Sometimes knowing the pass and instruction type makes it obvious, but in other cases it can take a more thorough reading of the pass to identify the source of the error. Since the root of the error always exists at the point where an empty DebugLoc is constructed however, seeing the call stack at that point can either provide an immediate fix or otherwise cut through the initial stages of investigation. As far as I'm aware we don't currently use any introspection similar to this in LLVM; I've therefore kept all functionality inside of conditionally-compiled blocks and made it an error to enable it in This series of patches adds a "DebugLoc coverage tracking" feature, that inserts conditionally-compiled tracking information into DebugLocs (and by extension, to Instructions), which is used by Debugify to provide more accurate and detailed coverage reports. When enabled, this features tracks whether and why we have intentionally dropped a DebugLoc, allowing Debugify to ignore false positives. An optional additional feature allows also storing a stack trace of the point where a DebugLoc was unintentionally dropped/not generated, which is used to make fixing detected errors significantly easier. The goal of these features is to provide useful tools for developers to fix existing DebugLoc errors and allow reliable detection of regressions by either manual inspection or an automated script. Patch is 35.49 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/107369.diff 18 Files Affected:
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index e765bbf637a661..20653daff7d4ae 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -911,6 +911,22 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
Debugify.setOrigDIVerifyBugsReportFilePath(
CodeGenOpts.DIBugsReportFilePath);
Debugify.registerCallbacks(PIC, MAM);
+
+#if ENABLE_DEBUGLOC_COVERAGE_TRACKING
+ // If we're using debug location coverage tracking, mark all the
+ // instructions coming out of the frontend without a DebugLoc as being
+ // intentional line-zero locations, to prevent both those instructions and
+ // new instructions that inherit their location from being treated as
+ // incorrectly empty locations.
+ for (Function &F : *TheModule) {
+ if (!F.getSubprogram())
+ continue;
+ for (BasicBlock &BB : F)
+ for (Instruction &I : BB)
+ if (!I.getDebugLoc())
+ I.setDebugLoc(DebugLoc::getLineZero());
+ }
+#endif
}
// Attempt to load pass plugins and register their callbacks with PB.
for (auto &PluginFN : CodeGenOpts.PassPlugins) {
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index 12618966c4adfd..3e2e90f5adad2e 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -524,6 +524,10 @@ endif()
option(LLVM_ENABLE_CRASH_DUMPS "Turn on memory dumps on crashes. Currently only implemented on Windows." OFF)
+set(LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING "DISABLED" CACHE STRING
+ "Enhance debugify's line number coverage tracking; enabling this is abi-breaking. Can be DISABLED, COVERAGE, or COVERAGE_AND_ORIGIN.")
+set_property(CACHE LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING PROPERTY STRINGS DISABLED COVERAGE COVERAGE_AND_ORIGIN)
+
set(WINDOWS_PREFER_FORWARD_SLASH_DEFAULT OFF)
if (MINGW)
# Cygwin doesn't identify itself as Windows, and thus gets path::Style::posix
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index 5ca580fbb59c59..7f66e55dca13b1 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -196,6 +196,19 @@ else()
message(FATAL_ERROR "Unknown value for LLVM_ABI_BREAKING_CHECKS: \"${LLVM_ABI_BREAKING_CHECKS}\"!")
endif()
+string(TOUPPER "${LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING}" uppercase_LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING)
+
+if( uppercase_LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING STREQUAL "COVERAGE" )
+ set( ENABLE_DEBUGLOC_COVERAGE_TRACKING 1 )
+elseif( uppercase_LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING STREQUAL "COVERAGE_AND_ORIGIN" )
+ set( ENABLE_DEBUGLOC_COVERAGE_TRACKING 1 )
+ set( ENABLE_DEBUGLOC_ORIGIN_TRACKING 1 )
+elseif( uppercase_LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING STREQUAL "DISABLED" OR NOT DEFINED LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING )
+ # The DISABLED setting is default and requires no additional defines.
+else()
+ message(FATAL_ERROR "Unknown value for LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING: \"${LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING}\"!")
+endif()
+
if( LLVM_REVERSE_ITERATION )
set( LLVM_ENABLE_REVERSE_ITERATION 1 )
endif()
diff --git a/llvm/docs/CMake.rst b/llvm/docs/CMake.rst
index 2a80813999ea1e..304e22759770d9 100644
--- a/llvm/docs/CMake.rst
+++ b/llvm/docs/CMake.rst
@@ -475,6 +475,17 @@ enabled sub-projects. Nearly all of these variable names begin with
**LLVM_ENABLE_BINDINGS**:BOOL
If disabled, do not try to build the OCaml bindings.
+**LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING**:STRING
+ Enhances Debugify's ability to detect line number errors by storing extra
+ information inside Instructions, removing false positives from Debugify's
+ results at the cost of performance. Allowed values are `DISABLED` (default),
+ `COVERAGE`, and `COVERAGE_AND_ORIGIN`. `COVERAGE` tracks whether and why a
+ line number was intentionally dropped or not generated for an instruction,
+ allowing Debugify to avoid reporting these as errors. `COVERAGE_AND_ORIGIN`
+ additionally stores a stacktrace of the point where each DebugLoc is
+ unintentionally dropped, allowing for much easier bug triaging at the cost of
+ a ~10x performance slowdown.
+
**LLVM_ENABLE_DIA_SDK**:BOOL
Enable building with MSVC DIA SDK for PDB debugging support. Available
only with MSVC. Defaults to ON.
diff --git a/llvm/include/llvm/Config/config.h.cmake b/llvm/include/llvm/Config/config.h.cmake
index ff30741c8f360a..7e8f1aa9474654 100644
--- a/llvm/include/llvm/Config/config.h.cmake
+++ b/llvm/include/llvm/Config/config.h.cmake
@@ -19,6 +19,14 @@
/* Define to 1 to enable crash memory dumps, and to 0 otherwise. */
#cmakedefine01 LLVM_ENABLE_CRASH_DUMPS
+/* Define to 1 to enable expensive checks for debug location coverage checking,
+ and to 0 otherwise. */
+#cmakedefine01 ENABLE_DEBUGLOC_COVERAGE_TRACKING
+
+/* Define to 1 to enable expensive tracking of the origin of debug location
+ coverage bugs, and to 0 otherwise. */
+#cmakedefine01 ENABLE_DEBUGLOC_ORIGIN_TRACKING
+
/* Define to 1 to prefer forward slashes on Windows, and to 0 prefer
backslashes. */
#cmakedefine01 LLVM_WINDOWS_PREFER_FORWARD_SLASH
diff --git a/llvm/include/llvm/IR/DebugLoc.h b/llvm/include/llvm/IR/DebugLoc.h
index c22d3e9b10d27f..5f45d853a92d53 100644
--- a/llvm/include/llvm/IR/DebugLoc.h
+++ b/llvm/include/llvm/IR/DebugLoc.h
@@ -14,6 +14,7 @@
#ifndef LLVM_IR_DEBUGLOC_H
#define LLVM_IR_DEBUGLOC_H
+#include "llvm/Config/config.h"
#include "llvm/IR/TrackingMDRef.h"
#include "llvm/Support/DataTypes.h"
@@ -22,6 +23,87 @@ namespace llvm {
class LLVMContext;
class raw_ostream;
class DILocation;
+ class Function;
+
+#if ENABLE_DEBUGLOC_COVERAGE_TRACKING
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+ struct DbgLocOrigin {
+ static constexpr unsigned long MaxDepth = 16;
+ using StackTracesTy =
+ SmallVector<std::pair<int, std::array<void *, MaxDepth>>, 0>;
+ StackTracesTy StackTraces;
+ DbgLocOrigin(bool ShouldCollectTrace);
+ void addTrace();
+ const StackTracesTy &getOriginStackTraces() const { return StackTraces; };
+ };
+#else
+ struct DbgLocOrigin {
+ DbgLocOrigin(bool) {}
+ }
+#endif
+
+ // Used to represent different "kinds" of DebugLoc, expressing that a DebugLoc
+ // is either ordinary, containing a valid DILocation, or otherwise describing
+ // the reason why the DebugLoc does not contain a valid DILocation.
+ enum class DebugLocKind : uint8_t {
+ // DebugLoc is expected to contain a valid DILocation.
+ Normal,
+ // DebugLoc intentionally does not have a valid DILocation; may be for a
+ // compiler-generated instruction, or an explicitly dropped location.
+ LineZero,
+ // DebugLoc does not have a known or currently knowable source location,
+ // e.g. the attribution is ambiguous in a way that can't be represented, or
+ // determining the correct location is complicated and requires future
+ // developer effort.
+ Unknown,
+ // DebugLoc is attached to an instruction that we don't expect to be
+ // emitted, and so can omit a valid DILocation; we don't expect to ever try
+ // and emit these into the line table, and trying to do so is a sign that
+ // something has gone wrong (most likely a DebugLoc leaking from a transient
+ // compiler-generated instruction).
+ Temporary
+ };
+
+ // Extends TrackingMDNodeRef to also store a DebugLocKind and Origin,
+ // allowing Debugify to ignore intentionally-empty DebugLocs and display the
+ // code responsible for generating unintentionally-empty DebugLocs.
+ // Currently we only need to track the Origin of this DILoc when using a
+ // DebugLoc that is Normal and empty, so only collect the origin stacktrace in
+ // those cases.
+ class DILocAndCoverageTracking : public TrackingMDNodeRef, public DbgLocOrigin {
+ public:
+ DebugLocKind Kind;
+ // Default constructor for empty DebugLocs.
+ DILocAndCoverageTracking()
+ : TrackingMDNodeRef(nullptr), DbgLocOrigin(true), Kind(DebugLocKind::Normal) {}
+ // Valid or nullptr MDNode*, normal DebugLocKind
+ DILocAndCoverageTracking(const MDNode *Loc)
+ : TrackingMDNodeRef(const_cast<MDNode *>(Loc)), DbgLocOrigin(!Loc),
+ Kind(DebugLocKind::Normal) {}
+ DILocAndCoverageTracking(const DILocation *Loc);
+ // Always nullptr MDNode*, any DebugLocKind
+ DILocAndCoverageTracking(DebugLocKind Kind)
+ : TrackingMDNodeRef(nullptr), DbgLocOrigin(Kind == DebugLocKind::Normal), Kind(Kind) {}
+ };
+ template <> struct simplify_type<DILocAndCoverageTracking> {
+ using SimpleType = MDNode *;
+
+ static MDNode *getSimplifiedValue(DILocAndCoverageTracking &MD) {
+ return MD.get();
+ }
+ };
+ template <> struct simplify_type<const DILocAndCoverageTracking> {
+ using SimpleType = MDNode *;
+
+ static MDNode *getSimplifiedValue(const DILocAndCoverageTracking &MD) {
+ return MD.get();
+ }
+ };
+
+ using DebugLocTrackingRef = DILocAndCoverageTracking;
+#else
+ using DebugLocTrackingRef = TrackingMDNodeRef;
+#endif // ENABLE_DEBUGLOC_COVERAGE_TRACKING
/// A debug info location.
///
@@ -31,7 +113,8 @@ namespace llvm {
/// To avoid extra includes, \a DebugLoc doubles the \a DILocation API with a
/// one based on relatively opaque \a MDNode pointers.
class DebugLoc {
- TrackingMDNodeRef Loc;
+
+ DebugLocTrackingRef Loc;
public:
DebugLoc() = default;
@@ -47,6 +130,32 @@ namespace llvm {
/// IR.
explicit DebugLoc(const MDNode *N);
+#if ENABLE_DEBUGLOC_COVERAGE_TRACKING
+ DebugLoc(DebugLocKind Kind) : Loc(Kind) {}
+ DebugLocKind getKind() const { return Loc.Kind; }
+#endif
+
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+#if !ENABLE_DEBUGLOC_COVERAGE_TRACKING
+ #error Cannot enable DebugLoc origin-tracking without coverage-tracking!
+#endif
+
+ const DbgLocOrigin::StackTracesTy &getOriginStackTraces() const {
+ return Loc.getOriginStackTraces();
+ }
+ DebugLoc getCopied() const {
+ DebugLoc NewDL = *this;
+ NewDL.Loc.addTrace();
+ return NewDL;
+ }
+#else
+ DebugLoc getCopied() const { return *this; }
+#endif
+
+ static DebugLoc getTemporary();
+ static DebugLoc getUnknown();
+ static DebugLoc getLineZero();
+
/// Get the underlying \a DILocation.
///
/// \pre !*this or \c isa<DILocation>(getAsMDNode()).
diff --git a/llvm/include/llvm/Support/Signals.h b/llvm/include/llvm/Support/Signals.h
index 70749ce30184a7..6addb8212e20ac 100644
--- a/llvm/include/llvm/Support/Signals.h
+++ b/llvm/include/llvm/Support/Signals.h
@@ -14,6 +14,8 @@
#ifndef LLVM_SUPPORT_SIGNALS_H
#define LLVM_SUPPORT_SIGNALS_H
+#include "llvm/Config/config.h"
+#include <array>
#include <cstdint>
#include <string>
@@ -21,6 +23,22 @@ namespace llvm {
class StringRef;
class raw_ostream;
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+// Typedefs that are convenient but only used by the StackTrace-collection code
+// added if DebugLoc origin-tracking is enabled.
+template <typename T, typename Enable> struct DenseMapInfo;
+template <typename ValueT, typename ValueInfoT> class DenseSet;
+namespace detail {
+template <typename KeyT, typename ValueT> struct DenseMapPair;
+}
+template <typename KeyT, typename ValueT, typename KeyInfoT, typename BucketT>
+class DenseMap;
+using AddressSet = DenseSet<void *, DenseMapInfo<void *, void>>;
+using SymbolizedAddressMap =
+ DenseMap<void *, std::string, DenseMapInfo<void *, void>,
+ detail::DenseMapPair<void *, std::string>>;
+#endif
+
namespace sys {
/// This function runs all the registered interrupt handlers, including the
@@ -55,6 +73,28 @@ namespace sys {
/// specified, the entire frame is printed.
void PrintStackTrace(raw_ostream &OS, int Depth = 0);
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+#ifdef NDEBUG
+#error DebugLoc origin-tracking should not be enabled in Release builds.
+#endif
+ /// Populates the given array with a stacktrace of the current program, up to
+ /// MaxDepth frames. Returns the number of frames returned, which will be
+ /// inserted into \p StackTrace from index 0. All entries after the returned
+ /// depth will be unmodified. NB: This is only intended to be used for
+ /// introspection of LLVM by Debugify, will not be enabled in release builds,
+ /// and should not be relied on for other purposes.
+ template <unsigned long MaxDepth>
+ int getStackTrace(std::array<void *, MaxDepth> &StackTrace);
+
+ /// Takes a set of \p Addresses, symbolizes them and stores the result in the
+ /// provided \p SymbolizedAddresses map.
+ /// NB: This is only intended to be used for introspection of LLVM by
+ /// Debugify, will not be enabled in release builds, and should not be relied
+ /// on for other purposes.
+ void symbolizeAddresses(AddressSet &Addresses,
+ SymbolizedAddressMap &SymbolizedAddresses);
+#endif
+
// Run all registered signal handlers.
void RunSignalHandlers();
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index f88653146cc6ff..4ba8262259b112 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -31,6 +31,7 @@
#include "llvm/CodeGen/TargetLowering.h"
#include "llvm/CodeGen/TargetRegisterInfo.h"
#include "llvm/CodeGen/TargetSubtargetInfo.h"
+#include "llvm/Config/config.h"
#include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
#include "llvm/DebugInfo/DWARF/DWARFExpression.h"
#include "llvm/IR/Constants.h"
@@ -2080,6 +2081,10 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
}
if (!DL) {
+ // FIXME: We could assert that `DL.getKind() != DebugLocKind::Temporary`
+ // here, or otherwise record any temporary DebugLocs seen to ensure that
+ // transient compiler-generated instructions aren't leaking their DLs to
+ // other instructions.
// We have an unspecified location, which might want to be line 0.
// If we have already emitted a line-0 record, don't repeat it.
if (LastAsmLine == 0)
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index 92a03eb52e35d9..edd60c5ad4a18d 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -915,7 +915,7 @@ bool BranchFolder::TryTailMergeBlocks(MachineBasicBlock *SuccBB,
// Walk through equivalence sets looking for actual exact matches.
while (MergePotentials.size() > 1) {
unsigned CurHash = MergePotentials.back().getHash();
- const DebugLoc &BranchDL = MergePotentials.back().getBranchDebugLoc();
+ const DebugLoc BranchDL = MergePotentials.back().getBranchDebugLoc();
// Build SameTails, identifying the set of blocks with this hash code
// and with the maximum number of instructions in common.
diff --git a/llvm/lib/CodeGen/BranchFolding.h b/llvm/lib/CodeGen/BranchFolding.h
index ff2bbe06c04887..9638cfda1239d1 100644
--- a/llvm/lib/CodeGen/BranchFolding.h
+++ b/llvm/lib/CodeGen/BranchFolding.h
@@ -50,11 +50,15 @@ class TargetRegisterInfo;
class MergePotentialsElt {
unsigned Hash;
MachineBasicBlock *Block;
- DebugLoc BranchDebugLoc;
+ // We use MDNode rather than DebugLoc here because under certain CMake
+ // options*, DebugLoc may contain a SmallVector used for introspection
+ // purposes, which causes errors when stored here.
+ // *LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING=COVERAGE_AND_ORIGIN
+ MDNode *BranchDebugLoc;
public:
- MergePotentialsElt(unsigned h, MachineBasicBlock *b, DebugLoc bdl)
- : Hash(h), Block(b), BranchDebugLoc(std::move(bdl)) {}
+ MergePotentialsElt(unsigned h, MachineBasicBlock *b, MDNode *bdl)
+ : Hash(h), Block(b), BranchDebugLoc(bdl) {}
unsigned getHash() const { return Hash; }
MachineBasicBlock *getBlock() const { return Block; }
@@ -63,7 +67,7 @@ class TargetRegisterInfo;
Block = MBB;
}
- const DebugLoc &getBranchDebugLoc() { return BranchDebugLoc; }
+ const DebugLoc getBranchDebugLoc() { return DebugLoc(BranchDebugLoc); }
bool operator<(const MergePotentialsElt &) const;
};
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 7fa1f9696d43b2..86ac46540c5ef9 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -979,7 +979,7 @@ void Instruction::dropLocation() {
}
if (!MayLowerToCall) {
- setDebugLoc(DebugLoc());
+ setDebugLoc(DebugLoc::getLineZero());
return;
}
@@ -998,7 +998,7 @@ void Instruction::dropLocation() {
//
// One alternative is to set a line 0 location with the existing scope and
// inlinedAt info. The location might be sensitive to when inlining occurs.
- setDebugLoc(DebugLoc());
+ setDebugLoc(DebugLoc::getLineZero());
}
//===----------------------------------------------------------------------===//
diff --git a/llvm/lib/IR/DebugLoc.cpp b/llvm/lib/IR/DebugLoc.cpp
index bdea52180f74ae..9ad1d58dc5dcc0 100644
--- a/llvm/lib/IR/DebugLoc.cpp
+++ b/llvm/lib/IR/DebugLoc.cpp
@@ -9,8 +9,44 @@
#include "llvm/IR/DebugLoc.h"
#include "llvm/Config/llvm-config.h"
#include "llvm/IR/DebugInfo.h"
+
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+#include "llvm/Support/Signals.h"
+
+namespace llvm {
+DbgLocOrigin::DbgLocOrigin(bool ShouldCollectTrace) {
+ if (ShouldCollectTrace) {
+ auto &[Depth, StackTrace] = StackTraces.emplace_back();
+ Depth = sys::getStackTrace(StackTrace);
+ }
+}
+void DbgLocOrigin::addTrace() {
+ if (StackTraces.empty())
+ return;
+ auto &[Depth, StackTrace] = StackTraces.emplace_back();
+ Depth = sys::getStackTrace(StackTrace);
+}
+}
+#endif
+
using namespace llvm;
+#if ENABLE_DEBUGLOC_COVERAGE_TRACKING
+DILocAndCoverageTracking::DILocAndCoverageTracking(const DILocation *L)
+ : TrackingMDNodeRef(const_cast<DILocation *>(L)), DbgLocOrigin(!L),
+ Kind(DebugLocKind::Normal) {}
+
+DebugLoc DebugLoc::getTemporary() { return DebugLoc(DebugLocKind::Temporary); }
+DebugLoc DebugLoc::getUnknown() { return DebugLoc(DebugLocKind::Unknown); }
+DebugLoc DebugLoc::getLineZero() { return DebugLoc(DebugLocKind::LineZero); }
+
+#else
+
+DebugLoc DebugLoc::getTemporary() { return DebugLoc(); }
+DebugLoc DebugLoc::getUnknown() { return DebugLoc(); }
+DebugLoc DebugLoc::getLineZero() { return DebugLoc(); }
+#endif // ENABLE_DEBUGLOC_COVERAGE_TRACKING
+
//===----------------------------------------------------------------------===//
// DebugLoc Implementation
//===----------------------------------------------------------------------===//
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 6f0f3f244c050c..2c0713aa886412 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -1279,6 +1279,9 @@ void Instruction::swapProfMetadata() {
void Instruction::copyMetadata(const Instruction &SrcInst,
ArrayRef<unsigned> WL) {
+ if (WL.empty() || is_contained(WL, LLVMContext::MD_dbg))
+ setDebugLoc(SrcInst.getDebugLoc());
+
if (!SrcInst.hasMetadata())
return;
@@ -1292,8 +1295,6 @@ void Instruction::copyMetadata(const Instruction &SrcInst,
if (WL.empty() || WLS.count(MD.first))
setMetadata(MD.first, MD.second);
}
- if (WL.empty() || WLS.count(LLVMContext::MD_dbg))
- setDebugLoc(SrcInst.getDebugLoc());
}
Instruction *Instruction::clone() const {
@@ -1311,5 +1312,6 @@ Instruction *Instruction::clone() const {
New->SubclassOptionalData = SubclassOptionalData;
New->copyMetadata(*this);
+ New->setDebugLoc(getDebugLoc().getCopied());
return New;
}
diff --git a/llvm/lib/Support/Signals.cpp b/llvm/lib/Support/Signals.cpp
index 9f9030e79d1040..6825720f51e96d 100644
--- a/llvm/lib/Support/Signals.cpp
+++ b/llvm/lib/Support/Signals.cpp
@@ -253,6 +253,122 @@ static bool printSymbolizedStackTrace(StringRef Argv0, void **StackTrace,
return true;
}
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+void sys::symbolizeAddresses(AddressSet &Addresses,
+ SymbolizedAddressMap &SymbolizedAddresses) {
+ assert(!DisableSymbolicationFlag && !getenv(DisableSymbolizationEnv) &&
+ "Debugify origin stacktraces require symbolization to be enabled.");
+
+ // Convert Set of Addresses to ordered list.
+ SmallVector<void *, 0> AddressList(Addresses.begin(), Addresses.end());
+ if (AddressList.empty())
+ retur...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: Stephen Tozer (SLTozer) ChangesThis is part of a series of patches that tries to improve DILocation bug detection in Debugify; see below for more details. This patch adds an origin-tracking feature, which collects a stack trace at the point that unintentionally empty DebugLocs are created (e.g. in the Instruction constructor), and storing that trace in the DebugLoc so that Debugify can symbolize it and print it as part of the generated JSON/HTML report for each reported bug. This is obviously very expensive; for the builds I used this with locally, it resulted in compile times around 9x slower. This is not necessarily a problem however, as the goal of this feature is to enable very fast diagnosis of DebugLoc errors by developers on their own machines, not to be part of any automated test environment. Most DebugLoc errors occur when we create an instruction and forget to assign it a DILocation. These errors are trivial to fix once the responsible code has been identified, but finding them can be tedious and time consuming. Sometimes knowing the pass and instruction type makes it obvious, but in other cases it can take a more thorough reading of the pass to identify the source of the error. Since the root of the error always exists at the point where an empty DebugLoc is constructed however, seeing the call stack at that point can either provide an immediate fix or otherwise cut through the initial stages of investigation. As far as I'm aware we don't currently use any introspection similar to this in LLVM; I've therefore kept all functionality inside of conditionally-compiled blocks and made it an error to enable it in This series of patches adds a "DebugLoc coverage tracking" feature, that inserts conditionally-compiled tracking information into DebugLocs (and by extension, to Instructions), which is used by Debugify to provide more accurate and detailed coverage reports. When enabled, this features tracks whether and why we have intentionally dropped a DebugLoc, allowing Debugify to ignore false positives. An optional additional feature allows also storing a stack trace of the point where a DebugLoc was unintentionally dropped/not generated, which is used to make fixing detected errors significantly easier. The goal of these features is to provide useful tools for developers to fix existing DebugLoc errors and allow reliable detection of regressions by either manual inspection or an automated script. Patch is 35.49 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/107369.diff 18 Files Affected:
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index e765bbf637a661..20653daff7d4ae 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -911,6 +911,22 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
Debugify.setOrigDIVerifyBugsReportFilePath(
CodeGenOpts.DIBugsReportFilePath);
Debugify.registerCallbacks(PIC, MAM);
+
+#if ENABLE_DEBUGLOC_COVERAGE_TRACKING
+ // If we're using debug location coverage tracking, mark all the
+ // instructions coming out of the frontend without a DebugLoc as being
+ // intentional line-zero locations, to prevent both those instructions and
+ // new instructions that inherit their location from being treated as
+ // incorrectly empty locations.
+ for (Function &F : *TheModule) {
+ if (!F.getSubprogram())
+ continue;
+ for (BasicBlock &BB : F)
+ for (Instruction &I : BB)
+ if (!I.getDebugLoc())
+ I.setDebugLoc(DebugLoc::getLineZero());
+ }
+#endif
}
// Attempt to load pass plugins and register their callbacks with PB.
for (auto &PluginFN : CodeGenOpts.PassPlugins) {
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index 12618966c4adfd..3e2e90f5adad2e 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -524,6 +524,10 @@ endif()
option(LLVM_ENABLE_CRASH_DUMPS "Turn on memory dumps on crashes. Currently only implemented on Windows." OFF)
+set(LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING "DISABLED" CACHE STRING
+ "Enhance debugify's line number coverage tracking; enabling this is abi-breaking. Can be DISABLED, COVERAGE, or COVERAGE_AND_ORIGIN.")
+set_property(CACHE LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING PROPERTY STRINGS DISABLED COVERAGE COVERAGE_AND_ORIGIN)
+
set(WINDOWS_PREFER_FORWARD_SLASH_DEFAULT OFF)
if (MINGW)
# Cygwin doesn't identify itself as Windows, and thus gets path::Style::posix
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index 5ca580fbb59c59..7f66e55dca13b1 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -196,6 +196,19 @@ else()
message(FATAL_ERROR "Unknown value for LLVM_ABI_BREAKING_CHECKS: \"${LLVM_ABI_BREAKING_CHECKS}\"!")
endif()
+string(TOUPPER "${LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING}" uppercase_LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING)
+
+if( uppercase_LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING STREQUAL "COVERAGE" )
+ set( ENABLE_DEBUGLOC_COVERAGE_TRACKING 1 )
+elseif( uppercase_LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING STREQUAL "COVERAGE_AND_ORIGIN" )
+ set( ENABLE_DEBUGLOC_COVERAGE_TRACKING 1 )
+ set( ENABLE_DEBUGLOC_ORIGIN_TRACKING 1 )
+elseif( uppercase_LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING STREQUAL "DISABLED" OR NOT DEFINED LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING )
+ # The DISABLED setting is default and requires no additional defines.
+else()
+ message(FATAL_ERROR "Unknown value for LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING: \"${LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING}\"!")
+endif()
+
if( LLVM_REVERSE_ITERATION )
set( LLVM_ENABLE_REVERSE_ITERATION 1 )
endif()
diff --git a/llvm/docs/CMake.rst b/llvm/docs/CMake.rst
index 2a80813999ea1e..304e22759770d9 100644
--- a/llvm/docs/CMake.rst
+++ b/llvm/docs/CMake.rst
@@ -475,6 +475,17 @@ enabled sub-projects. Nearly all of these variable names begin with
**LLVM_ENABLE_BINDINGS**:BOOL
If disabled, do not try to build the OCaml bindings.
+**LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING**:STRING
+ Enhances Debugify's ability to detect line number errors by storing extra
+ information inside Instructions, removing false positives from Debugify's
+ results at the cost of performance. Allowed values are `DISABLED` (default),
+ `COVERAGE`, and `COVERAGE_AND_ORIGIN`. `COVERAGE` tracks whether and why a
+ line number was intentionally dropped or not generated for an instruction,
+ allowing Debugify to avoid reporting these as errors. `COVERAGE_AND_ORIGIN`
+ additionally stores a stacktrace of the point where each DebugLoc is
+ unintentionally dropped, allowing for much easier bug triaging at the cost of
+ a ~10x performance slowdown.
+
**LLVM_ENABLE_DIA_SDK**:BOOL
Enable building with MSVC DIA SDK for PDB debugging support. Available
only with MSVC. Defaults to ON.
diff --git a/llvm/include/llvm/Config/config.h.cmake b/llvm/include/llvm/Config/config.h.cmake
index ff30741c8f360a..7e8f1aa9474654 100644
--- a/llvm/include/llvm/Config/config.h.cmake
+++ b/llvm/include/llvm/Config/config.h.cmake
@@ -19,6 +19,14 @@
/* Define to 1 to enable crash memory dumps, and to 0 otherwise. */
#cmakedefine01 LLVM_ENABLE_CRASH_DUMPS
+/* Define to 1 to enable expensive checks for debug location coverage checking,
+ and to 0 otherwise. */
+#cmakedefine01 ENABLE_DEBUGLOC_COVERAGE_TRACKING
+
+/* Define to 1 to enable expensive tracking of the origin of debug location
+ coverage bugs, and to 0 otherwise. */
+#cmakedefine01 ENABLE_DEBUGLOC_ORIGIN_TRACKING
+
/* Define to 1 to prefer forward slashes on Windows, and to 0 prefer
backslashes. */
#cmakedefine01 LLVM_WINDOWS_PREFER_FORWARD_SLASH
diff --git a/llvm/include/llvm/IR/DebugLoc.h b/llvm/include/llvm/IR/DebugLoc.h
index c22d3e9b10d27f..5f45d853a92d53 100644
--- a/llvm/include/llvm/IR/DebugLoc.h
+++ b/llvm/include/llvm/IR/DebugLoc.h
@@ -14,6 +14,7 @@
#ifndef LLVM_IR_DEBUGLOC_H
#define LLVM_IR_DEBUGLOC_H
+#include "llvm/Config/config.h"
#include "llvm/IR/TrackingMDRef.h"
#include "llvm/Support/DataTypes.h"
@@ -22,6 +23,87 @@ namespace llvm {
class LLVMContext;
class raw_ostream;
class DILocation;
+ class Function;
+
+#if ENABLE_DEBUGLOC_COVERAGE_TRACKING
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+ struct DbgLocOrigin {
+ static constexpr unsigned long MaxDepth = 16;
+ using StackTracesTy =
+ SmallVector<std::pair<int, std::array<void *, MaxDepth>>, 0>;
+ StackTracesTy StackTraces;
+ DbgLocOrigin(bool ShouldCollectTrace);
+ void addTrace();
+ const StackTracesTy &getOriginStackTraces() const { return StackTraces; };
+ };
+#else
+ struct DbgLocOrigin {
+ DbgLocOrigin(bool) {}
+ }
+#endif
+
+ // Used to represent different "kinds" of DebugLoc, expressing that a DebugLoc
+ // is either ordinary, containing a valid DILocation, or otherwise describing
+ // the reason why the DebugLoc does not contain a valid DILocation.
+ enum class DebugLocKind : uint8_t {
+ // DebugLoc is expected to contain a valid DILocation.
+ Normal,
+ // DebugLoc intentionally does not have a valid DILocation; may be for a
+ // compiler-generated instruction, or an explicitly dropped location.
+ LineZero,
+ // DebugLoc does not have a known or currently knowable source location,
+ // e.g. the attribution is ambiguous in a way that can't be represented, or
+ // determining the correct location is complicated and requires future
+ // developer effort.
+ Unknown,
+ // DebugLoc is attached to an instruction that we don't expect to be
+ // emitted, and so can omit a valid DILocation; we don't expect to ever try
+ // and emit these into the line table, and trying to do so is a sign that
+ // something has gone wrong (most likely a DebugLoc leaking from a transient
+ // compiler-generated instruction).
+ Temporary
+ };
+
+ // Extends TrackingMDNodeRef to also store a DebugLocKind and Origin,
+ // allowing Debugify to ignore intentionally-empty DebugLocs and display the
+ // code responsible for generating unintentionally-empty DebugLocs.
+ // Currently we only need to track the Origin of this DILoc when using a
+ // DebugLoc that is Normal and empty, so only collect the origin stacktrace in
+ // those cases.
+ class DILocAndCoverageTracking : public TrackingMDNodeRef, public DbgLocOrigin {
+ public:
+ DebugLocKind Kind;
+ // Default constructor for empty DebugLocs.
+ DILocAndCoverageTracking()
+ : TrackingMDNodeRef(nullptr), DbgLocOrigin(true), Kind(DebugLocKind::Normal) {}
+ // Valid or nullptr MDNode*, normal DebugLocKind
+ DILocAndCoverageTracking(const MDNode *Loc)
+ : TrackingMDNodeRef(const_cast<MDNode *>(Loc)), DbgLocOrigin(!Loc),
+ Kind(DebugLocKind::Normal) {}
+ DILocAndCoverageTracking(const DILocation *Loc);
+ // Always nullptr MDNode*, any DebugLocKind
+ DILocAndCoverageTracking(DebugLocKind Kind)
+ : TrackingMDNodeRef(nullptr), DbgLocOrigin(Kind == DebugLocKind::Normal), Kind(Kind) {}
+ };
+ template <> struct simplify_type<DILocAndCoverageTracking> {
+ using SimpleType = MDNode *;
+
+ static MDNode *getSimplifiedValue(DILocAndCoverageTracking &MD) {
+ return MD.get();
+ }
+ };
+ template <> struct simplify_type<const DILocAndCoverageTracking> {
+ using SimpleType = MDNode *;
+
+ static MDNode *getSimplifiedValue(const DILocAndCoverageTracking &MD) {
+ return MD.get();
+ }
+ };
+
+ using DebugLocTrackingRef = DILocAndCoverageTracking;
+#else
+ using DebugLocTrackingRef = TrackingMDNodeRef;
+#endif // ENABLE_DEBUGLOC_COVERAGE_TRACKING
/// A debug info location.
///
@@ -31,7 +113,8 @@ namespace llvm {
/// To avoid extra includes, \a DebugLoc doubles the \a DILocation API with a
/// one based on relatively opaque \a MDNode pointers.
class DebugLoc {
- TrackingMDNodeRef Loc;
+
+ DebugLocTrackingRef Loc;
public:
DebugLoc() = default;
@@ -47,6 +130,32 @@ namespace llvm {
/// IR.
explicit DebugLoc(const MDNode *N);
+#if ENABLE_DEBUGLOC_COVERAGE_TRACKING
+ DebugLoc(DebugLocKind Kind) : Loc(Kind) {}
+ DebugLocKind getKind() const { return Loc.Kind; }
+#endif
+
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+#if !ENABLE_DEBUGLOC_COVERAGE_TRACKING
+ #error Cannot enable DebugLoc origin-tracking without coverage-tracking!
+#endif
+
+ const DbgLocOrigin::StackTracesTy &getOriginStackTraces() const {
+ return Loc.getOriginStackTraces();
+ }
+ DebugLoc getCopied() const {
+ DebugLoc NewDL = *this;
+ NewDL.Loc.addTrace();
+ return NewDL;
+ }
+#else
+ DebugLoc getCopied() const { return *this; }
+#endif
+
+ static DebugLoc getTemporary();
+ static DebugLoc getUnknown();
+ static DebugLoc getLineZero();
+
/// Get the underlying \a DILocation.
///
/// \pre !*this or \c isa<DILocation>(getAsMDNode()).
diff --git a/llvm/include/llvm/Support/Signals.h b/llvm/include/llvm/Support/Signals.h
index 70749ce30184a7..6addb8212e20ac 100644
--- a/llvm/include/llvm/Support/Signals.h
+++ b/llvm/include/llvm/Support/Signals.h
@@ -14,6 +14,8 @@
#ifndef LLVM_SUPPORT_SIGNALS_H
#define LLVM_SUPPORT_SIGNALS_H
+#include "llvm/Config/config.h"
+#include <array>
#include <cstdint>
#include <string>
@@ -21,6 +23,22 @@ namespace llvm {
class StringRef;
class raw_ostream;
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+// Typedefs that are convenient but only used by the StackTrace-collection code
+// added if DebugLoc origin-tracking is enabled.
+template <typename T, typename Enable> struct DenseMapInfo;
+template <typename ValueT, typename ValueInfoT> class DenseSet;
+namespace detail {
+template <typename KeyT, typename ValueT> struct DenseMapPair;
+}
+template <typename KeyT, typename ValueT, typename KeyInfoT, typename BucketT>
+class DenseMap;
+using AddressSet = DenseSet<void *, DenseMapInfo<void *, void>>;
+using SymbolizedAddressMap =
+ DenseMap<void *, std::string, DenseMapInfo<void *, void>,
+ detail::DenseMapPair<void *, std::string>>;
+#endif
+
namespace sys {
/// This function runs all the registered interrupt handlers, including the
@@ -55,6 +73,28 @@ namespace sys {
/// specified, the entire frame is printed.
void PrintStackTrace(raw_ostream &OS, int Depth = 0);
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+#ifdef NDEBUG
+#error DebugLoc origin-tracking should not be enabled in Release builds.
+#endif
+ /// Populates the given array with a stacktrace of the current program, up to
+ /// MaxDepth frames. Returns the number of frames returned, which will be
+ /// inserted into \p StackTrace from index 0. All entries after the returned
+ /// depth will be unmodified. NB: This is only intended to be used for
+ /// introspection of LLVM by Debugify, will not be enabled in release builds,
+ /// and should not be relied on for other purposes.
+ template <unsigned long MaxDepth>
+ int getStackTrace(std::array<void *, MaxDepth> &StackTrace);
+
+ /// Takes a set of \p Addresses, symbolizes them and stores the result in the
+ /// provided \p SymbolizedAddresses map.
+ /// NB: This is only intended to be used for introspection of LLVM by
+ /// Debugify, will not be enabled in release builds, and should not be relied
+ /// on for other purposes.
+ void symbolizeAddresses(AddressSet &Addresses,
+ SymbolizedAddressMap &SymbolizedAddresses);
+#endif
+
// Run all registered signal handlers.
void RunSignalHandlers();
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index f88653146cc6ff..4ba8262259b112 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -31,6 +31,7 @@
#include "llvm/CodeGen/TargetLowering.h"
#include "llvm/CodeGen/TargetRegisterInfo.h"
#include "llvm/CodeGen/TargetSubtargetInfo.h"
+#include "llvm/Config/config.h"
#include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
#include "llvm/DebugInfo/DWARF/DWARFExpression.h"
#include "llvm/IR/Constants.h"
@@ -2080,6 +2081,10 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
}
if (!DL) {
+ // FIXME: We could assert that `DL.getKind() != DebugLocKind::Temporary`
+ // here, or otherwise record any temporary DebugLocs seen to ensure that
+ // transient compiler-generated instructions aren't leaking their DLs to
+ // other instructions.
// We have an unspecified location, which might want to be line 0.
// If we have already emitted a line-0 record, don't repeat it.
if (LastAsmLine == 0)
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index 92a03eb52e35d9..edd60c5ad4a18d 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -915,7 +915,7 @@ bool BranchFolder::TryTailMergeBlocks(MachineBasicBlock *SuccBB,
// Walk through equivalence sets looking for actual exact matches.
while (MergePotentials.size() > 1) {
unsigned CurHash = MergePotentials.back().getHash();
- const DebugLoc &BranchDL = MergePotentials.back().getBranchDebugLoc();
+ const DebugLoc BranchDL = MergePotentials.back().getBranchDebugLoc();
// Build SameTails, identifying the set of blocks with this hash code
// and with the maximum number of instructions in common.
diff --git a/llvm/lib/CodeGen/BranchFolding.h b/llvm/lib/CodeGen/BranchFolding.h
index ff2bbe06c04887..9638cfda1239d1 100644
--- a/llvm/lib/CodeGen/BranchFolding.h
+++ b/llvm/lib/CodeGen/BranchFolding.h
@@ -50,11 +50,15 @@ class TargetRegisterInfo;
class MergePotentialsElt {
unsigned Hash;
MachineBasicBlock *Block;
- DebugLoc BranchDebugLoc;
+ // We use MDNode rather than DebugLoc here because under certain CMake
+ // options*, DebugLoc may contain a SmallVector used for introspection
+ // purposes, which causes errors when stored here.
+ // *LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING=COVERAGE_AND_ORIGIN
+ MDNode *BranchDebugLoc;
public:
- MergePotentialsElt(unsigned h, MachineBasicBlock *b, DebugLoc bdl)
- : Hash(h), Block(b), BranchDebugLoc(std::move(bdl)) {}
+ MergePotentialsElt(unsigned h, MachineBasicBlock *b, MDNode *bdl)
+ : Hash(h), Block(b), BranchDebugLoc(bdl) {}
unsigned getHash() const { return Hash; }
MachineBasicBlock *getBlock() const { return Block; }
@@ -63,7 +67,7 @@ class TargetRegisterInfo;
Block = MBB;
}
- const DebugLoc &getBranchDebugLoc() { return BranchDebugLoc; }
+ const DebugLoc getBranchDebugLoc() { return DebugLoc(BranchDebugLoc); }
bool operator<(const MergePotentialsElt &) const;
};
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 7fa1f9696d43b2..86ac46540c5ef9 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -979,7 +979,7 @@ void Instruction::dropLocation() {
}
if (!MayLowerToCall) {
- setDebugLoc(DebugLoc());
+ setDebugLoc(DebugLoc::getLineZero());
return;
}
@@ -998,7 +998,7 @@ void Instruction::dropLocation() {
//
// One alternative is to set a line 0 location with the existing scope and
// inlinedAt info. The location might be sensitive to when inlining occurs.
- setDebugLoc(DebugLoc());
+ setDebugLoc(DebugLoc::getLineZero());
}
//===----------------------------------------------------------------------===//
diff --git a/llvm/lib/IR/DebugLoc.cpp b/llvm/lib/IR/DebugLoc.cpp
index bdea52180f74ae..9ad1d58dc5dcc0 100644
--- a/llvm/lib/IR/DebugLoc.cpp
+++ b/llvm/lib/IR/DebugLoc.cpp
@@ -9,8 +9,44 @@
#include "llvm/IR/DebugLoc.h"
#include "llvm/Config/llvm-config.h"
#include "llvm/IR/DebugInfo.h"
+
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+#include "llvm/Support/Signals.h"
+
+namespace llvm {
+DbgLocOrigin::DbgLocOrigin(bool ShouldCollectTrace) {
+ if (ShouldCollectTrace) {
+ auto &[Depth, StackTrace] = StackTraces.emplace_back();
+ Depth = sys::getStackTrace(StackTrace);
+ }
+}
+void DbgLocOrigin::addTrace() {
+ if (StackTraces.empty())
+ return;
+ auto &[Depth, StackTrace] = StackTraces.emplace_back();
+ Depth = sys::getStackTrace(StackTrace);
+}
+}
+#endif
+
using namespace llvm;
+#if ENABLE_DEBUGLOC_COVERAGE_TRACKING
+DILocAndCoverageTracking::DILocAndCoverageTracking(const DILocation *L)
+ : TrackingMDNodeRef(const_cast<DILocation *>(L)), DbgLocOrigin(!L),
+ Kind(DebugLocKind::Normal) {}
+
+DebugLoc DebugLoc::getTemporary() { return DebugLoc(DebugLocKind::Temporary); }
+DebugLoc DebugLoc::getUnknown() { return DebugLoc(DebugLocKind::Unknown); }
+DebugLoc DebugLoc::getLineZero() { return DebugLoc(DebugLocKind::LineZero); }
+
+#else
+
+DebugLoc DebugLoc::getTemporary() { return DebugLoc(); }
+DebugLoc DebugLoc::getUnknown() { return DebugLoc(); }
+DebugLoc DebugLoc::getLineZero() { return DebugLoc(); }
+#endif // ENABLE_DEBUGLOC_COVERAGE_TRACKING
+
//===----------------------------------------------------------------------===//
// DebugLoc Implementation
//===----------------------------------------------------------------------===//
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 6f0f3f244c050c..2c0713aa886412 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -1279,6 +1279,9 @@ void Instruction::swapProfMetadata() {
void Instruction::copyMetadata(const Instruction &SrcInst,
ArrayRef<unsigned> WL) {
+ if (WL.empty() || is_contained(WL, LLVMContext::MD_dbg))
+ setDebugLoc(SrcInst.getDebugLoc());
+
if (!SrcInst.hasMetadata())
return;
@@ -1292,8 +1295,6 @@ void Instruction::copyMetadata(const Instruction &SrcInst,
if (WL.empty() || WLS.count(MD.first))
setMetadata(MD.first, MD.second);
}
- if (WL.empty() || WLS.count(LLVMContext::MD_dbg))
- setDebugLoc(SrcInst.getDebugLoc());
}
Instruction *Instruction::clone() const {
@@ -1311,5 +1312,6 @@ Instruction *Instruction::clone() const {
New->SubclassOptionalData = SubclassOptionalData;
New->copyMetadata(*this);
+ New->setDebugLoc(getDebugLoc().getCopied());
return New;
}
diff --git a/llvm/lib/Support/Signals.cpp b/llvm/lib/Support/Signals.cpp
index 9f9030e79d1040..6825720f51e96d 100644
--- a/llvm/lib/Support/Signals.cpp
+++ b/llvm/lib/Support/Signals.cpp
@@ -253,6 +253,122 @@ static bool printSymbolizedStackTrace(StringRef Argv0, void **StackTrace,
return true;
}
+#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
+void sys::symbolizeAddresses(AddressSet &Addresses,
+ SymbolizedAddressMap &SymbolizedAddresses) {
+ assert(!DisableSymbolicationFlag && !getenv(DisableSymbolizationEnv) &&
+ "Debugify origin stacktraces require symbolization to be enabled.");
+
+ // Convert Set of Addresses to ordered list.
+ SmallVector<void *, 0> AddressList(Addresses.begin(), Addresses.end());
+ if (AddressList.empty())
+ retur...
[truncated]
|
You can test this locally with the following command:darker --check --diff -r 5f05d5ec8f9bb15c0ac29fce843a2c73165ac414...dfd8c8b6c892ffa35fe86346fa637226c78ef16e llvm/utils/llvm-original-di-preservation.pyView the diff from darker here.--- llvm-original-di-preservation.py 2024-09-05 10:03:53.000000 +0000
+++ llvm-original-di-preservation.py 2024-09-26 16:10:42.520554 +0000
@@ -112,11 +112,13 @@
row.append(llvm_pass)
row.append(x.instr)
row.append(x.fn_name)
row.append(x.bb_name)
row.append(x.action)
- row.append(f"<details><summary>View Origin StackTrace</summary><pre>{x.origin}</pre></details>")
+ row.append(
+ f"<details><summary>View Origin StackTrace</summary><pre>{x.origin}</pre></details>"
+ )
row.append(" </tr>\n")
# Dump the bugs info into the table.
for column in row:
# The same file-pass pair can have multiple bugs.
if column == " <tr>\n" or column == " </tr>\n":
|
You can test this locally with the following command:git-clang-format --diff 5f05d5ec8f9bb15c0ac29fce843a2c73165ac414 dfd8c8b6c892ffa35fe86346fa637226c78ef16e --extensions cpp,inc,h -- clang/lib/CodeGen/BackendUtil.cpp llvm/include/llvm/IR/DebugLoc.h llvm/include/llvm/Support/Signals.h llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp llvm/lib/CodeGen/BranchFolding.cpp llvm/lib/IR/DebugInfo.cpp llvm/lib/IR/DebugLoc.cpp llvm/lib/IR/Instruction.cpp llvm/lib/Support/Signals.cpp llvm/lib/Support/Unix/Signals.inc llvm/lib/Support/Windows/Signals.inc llvm/lib/Transforms/Utils/Debugify.cppView the diff from clang-format here.diff --git a/llvm/lib/Transforms/Utils/Debugify.cpp b/llvm/lib/Transforms/Utils/Debugify.cpp
index 3467f3482a1..5b358cae1dc 100644
--- a/llvm/lib/Transforms/Utils/Debugify.cpp
+++ b/llvm/lib/Transforms/Utils/Debugify.cpp
@@ -504,10 +504,13 @@ static bool checkInstructions(const DebugInstMap &DILocsBefore,
auto CreateJSONBugEntry = [&](const char *Action) {
Bugs.push_back(llvm::json::Object({
- {"metadata", "DILocation"}, {"fn-name", FnName.str()},
- {"bb-name", BBName.str()}, {"instr", InstName}, {"action", Action},
+ {"metadata", "DILocation"},
+ {"fn-name", FnName.str()},
+ {"bb-name", BBName.str()},
+ {"instr", InstName},
+ {"action", Action},
#if ENABLE_DEBUGLOC_ORIGIN_TRACKING
- {"origin", symbolizeStackTrace(Instr)},
+ {"origin", symbolizeStackTrace(Instr)},
#endif
}));
};
|
SLTozer
force-pushed
the
dl-coverage-3-origin-tracking
branch
from
194069a to
0d750fd
Compare
|
@SLTozer What's in patches 4 and 5...? I keep putting off looking at this stack, as I've been waiting for the whole series to appear (perhaps incorrectly)... |
Patches 4 and 5 are less consequential, but I'll open them up - patch 4 is adding origin-tracking support to the IRBuilder, so that we properly track the origin of DebugLocs that have been assigned by the IRBuilder (rather than exclusively seeing the IRBuilder as the origin), and patch 5 updates |
jryans
requested changes
Sep 23, 2024
Comment on lines
+142
to
+144
| #if !ENABLE_DEBUGLOC_COVERAGE_TRACKING | ||
| #error Cannot enable DebugLoc origin-tracking without coverage-tracking! | ||
| #endif |
There was a problem hiding this comment.
Not sure how critical checking this case is... IIUC, CMake options won't let you set such anything anyway, so perhaps remove this?
If keeping it, perhaps remove the blank line after this #endif here.
There was a problem hiding this comment.
I figured it'd be worth keeping something in the source just in case of some weird scenario with leaky environment variables; it's not likely to happen, but this will give a much cleaner error if it does.
There was a problem hiding this comment.
Hmm, how are environment variables ending up with up confused compiler defines...? Anyway, it's fine to keep, even if highly unlikely to be hit.
Comment on lines
+1282
to
+1283
| if (WL.empty() || is_contained(WL, LLVMContext::MD_dbg)) | ||
| setDebugLoc(SrcInst.getDebugLoc()); |
There was a problem hiding this comment.
Hmm, I'm wondering why this was necessary and if it may have unrelated consequence beyond this patch stack's feature...?
There was a problem hiding this comment.
The short answer is that the patch "works" without this change, but this patch is needed to ensure that the DebugLoc from SrcInst is always copied to the current instruction if MD_dbg is in the set to copy, even if SrcInst has no DebugLoc. The importance of this is straightforward - if the receiving instruction is meant to get its DebugLoc from SrcInst, and SrcInst has no DebugLoc, then we should copy the annotation (or lack of) from SrcInst to determine whether there's a bug and where it comes from.
There was a problem hiding this comment.
Ah okay, thanks, seems reasonable then!
| void sys::symbolizeAddresses(AddressSet &Addresses, | ||
| SymbolizedAddressMap &SymbolizedAddresses) { | ||
| assert(!DisableSymbolicationFlag && !getenv(DisableSymbolizationEnv) && | ||
| "Debugify origin stacktraces require symbolization to be enabled."); |
| int NumAddresses = AddressList.size(); | ||
| llvm::sort(AddressList); | ||
|
|
||
| // Use llvm-symbolizer tool to symbolize the stack traces. First look for it |
There was a problem hiding this comment.
It seems like this copies code from printSymbolizedStackTrace above...? Can we rework this to share common code instead of duplicating it?
There was a problem hiding this comment.
Possibly - my goal to start off here was to keep this as "out of the way" as possible, by purely inserting new code into conditionally compiled blocks whenever possible; I can see if there's a clean way to share the code.
There was a problem hiding this comment.
I don't see a need to be so conservative with this particular bit in the Support library. symbolizeAddresses seems like a useful function that others may want to use as well. So, I'd encourage making that available all the time (removing the #if guards) and refactoring to deduplicate shared code here.
| @@ -28,6 +31,11 @@ | |||
| #include "llvm/Support/FileSystem.h" | |||
| #include "llvm/Support/JSON.h" | |||
| #include <optional> | |||
| #if ENABLE_DEBUGLOC_ORIGIN_TRACKING | |||
| // We need the Signals header to operate on stacktraces if we're using DebugLoc | |||
|
IIRC, there are still some open review threads in this PR, mainly related to reducing duplication in the symbolication bits. |
Yes, this part won't land quite yet; as I'm kicking these reviews back into action, I'll be pushing up some other reviews to come before this one in the patch stack. The origin-tracking feature here is very useful for actually triaging bugs, but the most important features to land first are enabling coverage-tracking in its entirety, including annotating existing sites in the compiler, so I'll be coming back to this patch after landing the remaining coverage-focused patches. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is part of a series of patches that tries to improve DILocation bug detection in Debugify; see below for more details. This patch adds an origin-tracking feature, which collects a stack trace at the point that unintentionally empty DebugLocs are created (e.g. in the Instruction constructor), and storing that trace in the DebugLoc so that Debugify can symbolize it and print it as part of the generated JSON/HTML report for each reported bug. This is obviously very expensive; for the builds I used this with locally, it resulted in compile times around 9x slower. This is not necessarily a problem however, as the goal of this feature is to enable very fast diagnosis of DebugLoc errors by developers on their own machines, not to be part of any automated test environment.
Most DebugLoc errors occur when we create an instruction and forget to assign it a DILocation. These errors are trivial to fix once the responsible code has been identified, but finding them can be tedious and time consuming. Sometimes knowing the pass and instruction type makes it obvious, but in other cases it can take a more thorough reading of the pass to identify the source of the error. Since the root of the error always exists at the point where an empty DebugLoc is constructed however, seeing the call stack at that point can either provide an immediate fix or otherwise cut through the initial stages of investigation.
As far as I'm aware we don't currently use any introspection similar to this in LLVM; I've therefore kept all functionality inside of conditionally-compiled blocks and made it an error to enable it in
NDEBUGbuilds. The intention is that this is kept as a specialized tool for fixing complex or numerous debug line bugs, trading some compute time for developer time. The implementation also just uses the libcbacktracefor now, since it's the simplest way to get the feature implemented.This series of patches adds a "DebugLoc coverage tracking" feature, that inserts conditionally-compiled tracking information into DebugLocs (and by extension, to Instructions), which is used by Debugify to provide more accurate and detailed coverage reports. When enabled, this features tracks whether and why we have intentionally dropped a DebugLoc, allowing Debugify to ignore false positives. An optional additional feature allows also storing a stack trace of the point where a DebugLoc was unintentionally dropped/not generated, which is used to make fixing detected errors significantly easier. The goal of these features is to provide useful tools for developers to fix existing DebugLoc errors and allow reliable detection of regressions by either manual inspection or an automated script.
Previous: #107279
Next: #108214