From 0267bcce3f043931f1f0018066e580d7402c62a3 Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Thu, 14 Aug 2025 18:14:45 +0000 Subject: [PATCH 1/3] [DebugInfo] When referencing structured bindings use the reference's location, not the binding's declaration's location For structured bindings that use custom `get` specializations, the resulting LLVM IR ascribes the load of the result of `get` to the binding's declaration, rather than the place where the binding is referenced - this caused awkward sequencing in the debug info where, when stepping through the code you'd step back to the binding declaration every time there was a reference to the binding. To fix that - when we cross into IRGening a binding - suppress the debug info location of that subexpression. I don't represent this as a great bit of API design - certainly open to ideas, but putting it out here as a place to start. It's /possible/ this is an incomplete fix, even - if the binding decl had other subexpressions, those would still get their location applied & it'd likely be wrong. So maybe that's a direction to go with to productionize this - add a new location scoped device that suppresses any overriding - this might be more robust. How do people feel about that? --- clang/lib/CodeGen/CGExpr.cpp | 16 ++++++++++------ clang/lib/CodeGen/CodeGenFunction.h | 6 ++++-- clang/test/DebugInfo/CXX/structured-binding.cpp | 7 ++++++- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index 844b445b98c1d..798451b7799ff 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -1629,12 +1629,13 @@ LValue CodeGenFunction::EmitCheckedLValue(const Expr *E, TypeCheckKind TCK) { /// length type, this is not possible. /// LValue CodeGenFunction::EmitLValue(const Expr *E, - KnownNonNull_t IsKnownNonNull) { + KnownNonNull_t IsKnownNonNull, bool SuppressLocation) { // Running with sufficient stack space to avoid deeply nested expressions // cause a stack overflow. LValue LV; - CGM.runWithSufficientStackSpace( - E->getExprLoc(), [&] { LV = EmitLValueHelper(E, IsKnownNonNull); }); + CGM.runWithSufficientStackSpace(E->getExprLoc(), [&] { + LV = EmitLValueHelper(E, IsKnownNonNull, SuppressLocation); + }); if (IsKnownNonNull && !LV.isKnownNonNull()) LV.setKnownNonNull(); @@ -1650,8 +1651,11 @@ static QualType getConstantExprReferredType(const FullExpr *E, } LValue CodeGenFunction::EmitLValueHelper(const Expr *E, - KnownNonNull_t IsKnownNonNull) { - ApplyDebugLocation DL(*this, E); + KnownNonNull_t IsKnownNonNull, + bool SuppressLocation) { + std::optional DL; + if (!SuppressLocation) + DL.emplace(*this, E); switch (E->getStmtClass()) { default: return EmitUnsupportedLValue(E, "l-value expression"); @@ -3387,7 +3391,7 @@ LValue CodeGenFunction::EmitDeclRefLValue(const DeclRefExpr *E) { auto *FD = LambdaCaptureFields.lookup(BD); return EmitCapturedFieldLValue(*this, FD, CXXABIThisValue); } - return EmitLValue(BD->getBinding()); + return EmitLValue(BD->getBinding(), NotKnownNonNull, true); } // We can form DeclRefExprs naming GUID declarations when reconstituting diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h index c02ac18ec0198..b80c6a6a6b4ec 100644 --- a/clang/lib/CodeGen/CodeGenFunction.h +++ b/clang/lib/CodeGen/CodeGenFunction.h @@ -4230,10 +4230,12 @@ class CodeGenFunction : public CodeGenTypeCache { /// variable length type, this is not possible. /// LValue EmitLValue(const Expr *E, - KnownNonNull_t IsKnownNonNull = NotKnownNonNull); + KnownNonNull_t IsKnownNonNull = NotKnownNonNull, + bool SuppressLocation = false); private: - LValue EmitLValueHelper(const Expr *E, KnownNonNull_t IsKnownNonNull); + LValue EmitLValueHelper(const Expr *E, KnownNonNull_t IsKnownNonNull, + bool SuppressLocation); public: /// Same as EmitLValue but additionally we generate checking code to diff --git a/clang/test/DebugInfo/CXX/structured-binding.cpp b/clang/test/DebugInfo/CXX/structured-binding.cpp index 5fbd54c16382c..d1de46807f5f0 100644 --- a/clang/test/DebugInfo/CXX/structured-binding.cpp +++ b/clang/test/DebugInfo/CXX/structured-binding.cpp @@ -7,6 +7,7 @@ // CHECK: #dbg_declare(ptr %{{[0-9]+}}, ![[VAR_4:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 4), // CHECK: #dbg_declare(ptr %z1, ![[VAR_5:[0-9]+]], !DIExpression() // CHECK: #dbg_declare(ptr %z2, ![[VAR_6:[0-9]+]], !DIExpression() +// CHECK: load ptr, ptr %z1, {{.*}}!dbg ![[Z1_DEBUG_LOC:[0-9]+]] // CHECK: ![[VAR_0]] = !DILocalVariable(name: "a" // CHECK: ![[VAR_1]] = !DILocalVariable(name: "x1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}}) // CHECK: ![[VAR_2]] = !DILocalVariable(name: "y1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}}) @@ -46,5 +47,9 @@ int f() { auto [x1, y1] = a; auto &[x2, y2] = a; auto [z1, z2] = B{1, 2}; - return x1 + y1 + x2 + y2 + z1 + z2; + return x1 + y1 + x2 + y2 + +// CHECK: ![[Z1_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]] + z1 // + + // + z2; // } From 4a1d1bbe958a208a5c0d6674157cb59c43486c70 Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Tue, 2 Sep 2025 20:08:16 +0000 Subject: [PATCH 2/3] Generalize/reuse DisableDebugLocationUpdates --- clang/lib/CodeGen/CGCall.cpp | 26 ++++++++++++-------------- clang/lib/CodeGen/CGCall.h | 6 ++++++ clang/lib/CodeGen/CGExpr.cpp | 17 +++++++---------- clang/lib/CodeGen/CodeGenFunction.h | 6 ++---- 4 files changed, 27 insertions(+), 28 deletions(-) diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp index c024f944c9050..a94a7ed51521c 100644 --- a/clang/lib/CodeGen/CGCall.cpp +++ b/clang/lib/CodeGen/CGCall.cpp @@ -4818,19 +4818,6 @@ struct DestroyUnpassedArg final : EHScopeStack::Cleanup { } }; -struct DisableDebugLocationUpdates { - CodeGenFunction &CGF; - bool disabledDebugInfo; - DisableDebugLocationUpdates(CodeGenFunction &CGF, const Expr *E) : CGF(CGF) { - if ((disabledDebugInfo = isa(E) && CGF.getDebugInfo())) - CGF.disableDebugInfo(); - } - ~DisableDebugLocationUpdates() { - if (disabledDebugInfo) - CGF.enableDebugInfo(); - } -}; - } // end anonymous namespace RValue CallArg::getRValue(CodeGenFunction &CGF) const { @@ -4867,7 +4854,9 @@ void CodeGenFunction::EmitWritebacks(const CallArgList &args) { void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E, QualType type) { - DisableDebugLocationUpdates Dis(*this, E); + std::optional Dis; + if (isa(E)) + Dis.emplace(*this); if (const ObjCIndirectCopyRestoreExpr *CRE = dyn_cast(E)) { assert(getLangOpts().ObjCAutoRefCount); @@ -6282,3 +6271,12 @@ RValue CodeGenFunction::EmitVAArg(VAArgExpr *VE, Address &VAListAddr, return CGM.getABIInfo().EmitMSVAArg(*this, VAListAddr, Ty, Slot); return CGM.getABIInfo().EmitVAArg(*this, VAListAddr, Ty, Slot); } + +DisableDebugLocationUpdates::DisableDebugLocationUpdates(CodeGenFunction &CGF) + : CGF(CGF) { + CGF.disableDebugInfo(); +} + +DisableDebugLocationUpdates::~DisableDebugLocationUpdates() { + CGF.enableDebugInfo(); +} diff --git a/clang/lib/CodeGen/CGCall.h b/clang/lib/CodeGen/CGCall.h index 3157b7f16f294..935b5086f5983 100644 --- a/clang/lib/CodeGen/CGCall.h +++ b/clang/lib/CodeGen/CGCall.h @@ -453,6 +453,12 @@ inline FnInfoOpts &operator&=(FnInfoOpts &A, FnInfoOpts B) { return A; } +struct DisableDebugLocationUpdates { + CodeGenFunction &CGF; + DisableDebugLocationUpdates(CodeGenFunction &CGF); + ~DisableDebugLocationUpdates(); +}; + } // end namespace CodeGen } // end namespace clang diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index 798451b7799ff..c5b0bd39082b2 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -1629,13 +1629,12 @@ LValue CodeGenFunction::EmitCheckedLValue(const Expr *E, TypeCheckKind TCK) { /// length type, this is not possible. /// LValue CodeGenFunction::EmitLValue(const Expr *E, - KnownNonNull_t IsKnownNonNull, bool SuppressLocation) { + KnownNonNull_t IsKnownNonNull) { // Running with sufficient stack space to avoid deeply nested expressions // cause a stack overflow. LValue LV; - CGM.runWithSufficientStackSpace(E->getExprLoc(), [&] { - LV = EmitLValueHelper(E, IsKnownNonNull, SuppressLocation); - }); + CGM.runWithSufficientStackSpace( + E->getExprLoc(), [&] { LV = EmitLValueHelper(E, IsKnownNonNull); }); if (IsKnownNonNull && !LV.isKnownNonNull()) LV.setKnownNonNull(); @@ -1651,11 +1650,8 @@ static QualType getConstantExprReferredType(const FullExpr *E, } LValue CodeGenFunction::EmitLValueHelper(const Expr *E, - KnownNonNull_t IsKnownNonNull, - bool SuppressLocation) { - std::optional DL; - if (!SuppressLocation) - DL.emplace(*this, E); + KnownNonNull_t IsKnownNonNull) { + ApplyDebugLocation DL(*this, E); switch (E->getStmtClass()) { default: return EmitUnsupportedLValue(E, "l-value expression"); @@ -3391,7 +3387,8 @@ LValue CodeGenFunction::EmitDeclRefLValue(const DeclRefExpr *E) { auto *FD = LambdaCaptureFields.lookup(BD); return EmitCapturedFieldLValue(*this, FD, CXXABIThisValue); } - return EmitLValue(BD->getBinding(), NotKnownNonNull, true); + DisableDebugLocationUpdates D(*this); + return EmitLValue(BD->getBinding(), NotKnownNonNull); } // We can form DeclRefExprs naming GUID declarations when reconstituting diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h index b80c6a6a6b4ec..c02ac18ec0198 100644 --- a/clang/lib/CodeGen/CodeGenFunction.h +++ b/clang/lib/CodeGen/CodeGenFunction.h @@ -4230,12 +4230,10 @@ class CodeGenFunction : public CodeGenTypeCache { /// variable length type, this is not possible. /// LValue EmitLValue(const Expr *E, - KnownNonNull_t IsKnownNonNull = NotKnownNonNull, - bool SuppressLocation = false); + KnownNonNull_t IsKnownNonNull = NotKnownNonNull); private: - LValue EmitLValueHelper(const Expr *E, KnownNonNull_t IsKnownNonNull, - bool SuppressLocation); + LValue EmitLValueHelper(const Expr *E, KnownNonNull_t IsKnownNonNull); public: /// Same as EmitLValue but additionally we generate checking code to From 45dfb2a279fcef9077785c6aeab941867341de67 Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Tue, 2 Sep 2025 20:41:27 +0000 Subject: [PATCH 3/3] Add more test coverage --- clang/lib/CodeGen/CGExpr.cpp | 6 +++ .../test/DebugInfo/CXX/structured-binding.cpp | 49 ++++++++++++++++--- 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index c5b0bd39082b2..26fba751e6f9d 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -3387,6 +3387,12 @@ LValue CodeGenFunction::EmitDeclRefLValue(const DeclRefExpr *E) { auto *FD = LambdaCaptureFields.lookup(BD); return EmitCapturedFieldLValue(*this, FD, CXXABIThisValue); } + // Suppress debug location updates when visiting the binding, since the + // binding may emit instructions that would otherwise be associated with the + // binding itself, rather than the expression referencing the binding. (this + // leads to jumpy debug stepping behavior where the location/debugger jump + // back to the binding declaration, then back to the expression referencing + // the binding) DisableDebugLocationUpdates D(*this); return EmitLValue(BD->getBinding(), NotKnownNonNull); } diff --git a/clang/test/DebugInfo/CXX/structured-binding.cpp b/clang/test/DebugInfo/CXX/structured-binding.cpp index d1de46807f5f0..4a4a4d8bdfaad 100644 --- a/clang/test/DebugInfo/CXX/structured-binding.cpp +++ b/clang/test/DebugInfo/CXX/structured-binding.cpp @@ -7,7 +7,12 @@ // CHECK: #dbg_declare(ptr %{{[0-9]+}}, ![[VAR_4:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 4), // CHECK: #dbg_declare(ptr %z1, ![[VAR_5:[0-9]+]], !DIExpression() // CHECK: #dbg_declare(ptr %z2, ![[VAR_6:[0-9]+]], !DIExpression() -// CHECK: load ptr, ptr %z1, {{.*}}!dbg ![[Z1_DEBUG_LOC:[0-9]+]] +// CHECK: getelementptr inbounds nuw %struct.A, ptr {{.*}}, i32 0, i32 1, !dbg ![[Y1_DEBUG_LOC:[0-9]+]] +// CHECK: getelementptr inbounds nuw %struct.A, ptr {{.*}}, i32 0, i32 1, !dbg ![[Y2_DEBUG_LOC:[0-9]+]] +// CHECK: load ptr, ptr %z2, {{.*}}!dbg ![[Z2_DEBUG_LOC:[0-9]+]] +// CHECK: getelementptr inbounds [2 x i32], ptr {{.*}}, i64 0, i64 1, !dbg ![[A2_DEBUG_LOC:[0-9]+]] +// CHECK: getelementptr inbounds nuw { i32, i32 }, ptr {{.*}}, i32 0, i32 1, !dbg ![[C2_DEBUG_LOC:[0-9]+]] +// CHECK: extractelement <2 x i32> {{.*}}, i32 1, !dbg ![[V2_DEBUG_LOC:[0-9]+]] // CHECK: ![[VAR_0]] = !DILocalVariable(name: "a" // CHECK: ![[VAR_1]] = !DILocalVariable(name: "x1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}}) // CHECK: ![[VAR_2]] = !DILocalVariable(name: "y1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}}) @@ -47,9 +52,41 @@ int f() { auto [x1, y1] = a; auto &[x2, y2] = a; auto [z1, z2] = B{1, 2}; - return x1 + y1 + x2 + y2 + -// CHECK: ![[Z1_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]] - z1 // - + // - z2; // + int array[2] = {3, 4}; + auto &[a1, a2] = array; + _Complex int cmplx = {1, 2}; + auto &[c1, c2] = cmplx; + int vctr __attribute__ ((vector_size (sizeof(int)*2)))= {1, 2}; + auto &[v1, v2] = vctr; + return // + x1 // + + // +// CHECK: ![[Y1_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]] + y1 // + + // + x2 // + + // +// CHECK: ![[Y2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]] + y2 // + + // + z1 // + + // +// CHECK: ![[Z2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]] + z2 // + + // + a1 // + + // +// CHECK: ![[A2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]] + a2 // + + // + c1 // + + // +// CHECK: ![[C2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]] + c2 // + + // + v1 // + + // +// CHECK: ![[V2_DEBUG_LOC]] = !DILocation(line: [[@LINE+1]] + v2 // + ; }