[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
https://github.com/dzhidzhoev closed https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
dzhidzhoev wrote: > > Performance cost in terms of compilation time? > > Compilation time indeed -- there's a small chance that we unexpectedly start > producing masses of useless type information, which would mean we'd have to > rethink this. It'll show up on https://llvm-compile-time-tracker.com/ pretty > rapidly if there's a problem. Thank you! I'll keep an eye on it. https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
jmorse wrote: > Performance cost in terms of compilation time? Compilation time indeed -- there's a small chance that we unexpectedly start producing masses of useless type information, which would mean we'd have to rethink this. It'll show up on https://llvm-compile-time-tracker.com/ pretty rapidly if there's a problem. https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
https://github.com/jmorse approved this pull request. > It seems to me that manual finalizeSubprogram() calls were added > sporadically, not by following declaration-vs-definition logic. > With the call added [and with #119001], the test output changes: declaration > DISubprograms have their DICompositeTypes in the retainedNodes field. I think that's a good enough reason for this patch -- I get the feeling that the DIBuilder API has grown organically rather than being designed to fit a model, and this is just a further evolution of it. I can't think of any problems that this change would introduce. If there's some risk / problem that the patch introduces then we can always refine DIBuilder further. https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
dzhidzhoev wrote: Gentle ping https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
dzhidzhoev wrote: > I suppose what this is encoding is a (subtle?) assumption about how > definition-subprograms are modelled: they're created once and are immutable. > Wheras declaration-functions can be forwarded declared and then have extra > information added to them when a definition is emitted. It is possible. By the way, I've noticed that (declaration) SPs created with the `DIBuilder::CreateMethod` call in `CGDebugInfo::CreateCXXMemberFunction` are not finalized anywhere in clang (at least it's true for `get_a`/`get_b` in `clang/test/CodeGenCXX/debug-info-local-types.cpp` from https://github.com/llvm/llvm-project/pull/119001). I think clang expected DIBuilder to finalize everything before https://reviews.llvm.org/D33704, where it turned out that clang needed some subprograms to be finalized before the finalization of the whole CGDebugInfo instance. It seems to me that manual finalizeSubprogram() calls were added sporadically, not followed by declaration-vs-definition logic. > If there's no performance cost as a result of this patch, then it seems fine > to go in to me. Performance cost in terms of compilation time? > but is there a functional reason in a later patch that makes it necessary? I've touched it briefly here https://github.com/llvm/llvm-project/pull/119001#discussion_r2089196943. I've noticed that the mentioned PR doesn't call `finalizeSubprogram()` for the created declaration SP. With the call added, the test output changes: declaration DISubprograms have their DICompositeTypes in the `retainedNodes` field. We can't just add `finalizeSubprogram` right after `CreateSubprogram()`, as we return from the function before the local types are created. And we can't attach a declaration subprogram to the corresponding Clang AST declaration, as it may not exist. We could start tracking these declarations in CGDebugInfo class, but I'm curious why not to do that on DIBuilder's level globally :) https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
dwblaikie wrote: yeah, can't say I have any recollection of why function declaration would be in a "retained types" list - probably incidental/not especially intentional (we didn't make declarations for functions, except member functions, until "recently" when call site debug info was added). I guess it's worth a go to add them to all the subprograms. https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
https://github.com/jmorse commented: I suppose what this is encoding is a (subtle?) assumption about how definition-subprograms are modelled: they're created once and are immutable. Wheras declaration-functions can be forwarded declared and then have extra information added to them when a definition is emitted. DIBuilder enforces this by refusing to let you re-set the retainedNodes of definition subprograms. If there's no performance cost as a result of this patch, then it seems fine to go in to me., but is there a functional reason in a later patch that makes it necessary? Other folks might believe it's valuable to preserve the assumption about how debug-info is constructed within DIBuilder (CC @adrian-prantl @dwblaikie ). https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
dzhidzhoev wrote: Gentle ping https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
https://github.com/dzhidzhoev edited https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
https://github.com/dzhidzhoev edited https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
https://github.com/dzhidzhoev edited https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
https://github.com/dzhidzhoev edited https://github.com/llvm/llvm-project/pull/139914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
llvmbot wrote:
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-debuginfo
Author: Vladislav Dzhidzhoev (dzhidzhoev)
Changes
DIBuilder began tracking definition subprograms and finalizing them in
`DIBuilder::finalize()` in eb1bb4e419.
Currently, `finalizeSubprogram()` attaches local variables and labels to the
`retainedNodes:` field of a corresponding subprogram.
After 75819aedf, the definition and some declaration subprograms are finalized
in `DIBuilder::finalize()`:
`AllSubprograms` holds references to definition subprograms.
`RetainValues` holds references to declaration subprograms.
For DISubprogram elements of both variables, `finalizeSubprogram()` was called
there.
However, `retainTypes()` is not necessarily called for every declaration
subprogram (as in 40a3fcb0).
DIBuilder clients may also want to attach DILocalVariables to declaration
subprograms, for example, in 58bdf8f9a8.
Thus, the `finalizeSubprogram()` function is called for all definition
subprograms in `DIBuilder::finalize()` because they are stored in the
`AllSubprograms` during the `CreateFunction(isDefinition: true)` call. But for
the declaration subprograms, it should be called manually.
I think this feature of the DIBuilder interface is somewhat unclear, and
`AllSubprograms` could just be used for holding and finalizing all
DISubprograms.
---
Full diff: https://github.com/llvm/llvm-project/pull/139914.diff
2 Files Affected:
- (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+1-3)
- (modified) llvm/lib/IR/DIBuilder.cpp (+2-4)
``diff
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp
b/clang/lib/CodeGen/CGDebugInfo.cpp
index 2a11eebf1b682..bac113289f507 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4611,7 +4611,7 @@ void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD,
SourceLocation Loc,
// Preserve btf_decl_tag attributes for parameters of extern functions
// for BPF target. The parameters created in this loop are attached as
- // DISubprogram's retainedNodes in the subsequent finalizeSubprogram call.
+ // DISubprogram's retainedNodes in the DIBuilder::finalize() call.
if (IsDeclForCallSite && CGM.getTarget().getTriple().isBPF()) {
if (auto *FD = dyn_cast(D)) {
llvm::DITypeRefArray ParamTypes = STy->getTypeArray();
@@ -4628,8 +4628,6 @@ void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD,
SourceLocation Loc,
if (IsDeclForCallSite)
Fn->setSubprogram(SP);
-
- DBuilder.finalizeSubprogram(SP);
}
void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
diff --git a/llvm/lib/IR/DIBuilder.cpp b/llvm/lib/IR/DIBuilder.cpp
index 90da9f3acfe57..35e61caf9a241 100644
--- a/llvm/lib/IR/DIBuilder.cpp
+++ b/llvm/lib/IR/DIBuilder.cpp
@@ -940,8 +940,7 @@ DISubprogram *DIBuilder::createFunction(
SPFlags, IsDefinition ? CUNode : nullptr, TParams, Decl, nullptr,
ThrownTypes, Annotations, TargetFuncName);
- if (IsDefinition)
-AllSubprograms.push_back(Node);
+ AllSubprograms.push_back(Node);
trackIfUnresolved(Node);
return Node;
}
@@ -978,8 +977,7 @@ DISubprogram *DIBuilder::createMethod(
Flags, SPFlags, IsDefinition ? CUNode : nullptr, TParams, nullptr,
nullptr, ThrownTypes);
- if (IsDefinition)
-AllSubprograms.push_back(SP);
+ AllSubprograms.push_back(SP);
trackIfUnresolved(SP);
return SP;
}
``
https://github.com/llvm/llvm-project/pull/139914
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [llvm][DebugInfo][clang] Finalize all declaration subprograms in DIBuilder::finalize() (PR #139914)
https://github.com/dzhidzhoev created
https://github.com/llvm/llvm-project/pull/139914
DIBuilder began tracking definition subprograms and finalizing them in
`DIBuilder::finalize()` in eb1bb4e419.
Currently, `finalizeSubprogram()` attaches local variables and labels to the
`retainedNodes:` field of a corresponding subprogram.
After 75819aedf, the definition and some declaration subprograms are finalized
in `DIBuilder::finalize()`:
`AllSubprograms` holds references to definition subprograms.
`RetainValues` holds references to declaration subprograms.
For DISubprogram elements of both variables, `finalizeSubprogram()` was called
there.
However, `retainTypes()` is not necessarily called for every declaration
subprogram (as in 40a3fcb0).
DIBuilder clients may also want to attach DILocalVariables to declaration
subprograms, for example, in 58bdf8f9a8.
Thus, the `finalizeSubprogram()` function is called for all definition
subprograms in `DIBuilder::finalize()` because they are stored in the
`AllSubprograms` during the `CreateFunction(isDefinition: true)` call. But for
the declaration subprograms, it should be called manually.
I think this feature of the DIBuilder interface is somewhat unclear, and
`AllSubprograms` could just be used for holding and finalizing all
DISubprograms.
>From 261890db08127247b6ea0e436b630d59d016443d Mon Sep 17 00:00:00 2001
From: Vladislav Dzhidzhoev
Date: Tue, 13 May 2025 12:40:42 +0200
Subject: [PATCH] [llvm][DebugInfo][clang] Finalize all declaration subprograms
in DIBuilder::finalize()
DIBuilder began tracking definition subprograms and finalizing them
in DIBuilder::finalize() in eb1bb4e419.
Currently, finalizeSubprogram() attaches local variables and labels
to the `retainedNodes:` field of a corresponding subprogram.
After 75819aedf, the definition and some declaration subprograms were
finalized in DIBuilder::finalize():
`AllSubprograms` held references to definition subprograms.
`RetainValues` held references to declaration subprograms.
For DISubprogram elements of both members, finalizeSubprogram()
was called there.
However, retainTypes() is not necessarily called for every declaration
subprogram (as in 40a3fcb0).
DIBuilder clients may also want to attach DILocalVariables to declaration
subprograms, for example, in 58bdf8f9a8.
Thus, the finalizeSubprogram() function is called for all definition
subprograms in DIBuilder::finalize() because they are stored
in the `AllSubprograms` during the `CreateFunction(isDefinition: true)` call.
But for the definition subprograms, it should be called manually.
I think this feature of the DIBuilder interface is somewhat unclear,
and `AllSubprograms` could just be used for holding and finalizing
all DISubprograms.
---
clang/lib/CodeGen/CGDebugInfo.cpp | 4 +---
llvm/lib/IR/DIBuilder.cpp | 6 ++
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp
b/clang/lib/CodeGen/CGDebugInfo.cpp
index 2a11eebf1b682..bac113289f507 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4611,7 +4611,7 @@ void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD,
SourceLocation Loc,
// Preserve btf_decl_tag attributes for parameters of extern functions
// for BPF target. The parameters created in this loop are attached as
- // DISubprogram's retainedNodes in the subsequent finalizeSubprogram call.
+ // DISubprogram's retainedNodes in the DIBuilder::finalize() call.
if (IsDeclForCallSite && CGM.getTarget().getTriple().isBPF()) {
if (auto *FD = dyn_cast(D)) {
llvm::DITypeRefArray ParamTypes = STy->getTypeArray();
@@ -4628,8 +4628,6 @@ void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD,
SourceLocation Loc,
if (IsDeclForCallSite)
Fn->setSubprogram(SP);
-
- DBuilder.finalizeSubprogram(SP);
}
void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
diff --git a/llvm/lib/IR/DIBuilder.cpp b/llvm/lib/IR/DIBuilder.cpp
index 90da9f3acfe57..35e61caf9a241 100644
--- a/llvm/lib/IR/DIBuilder.cpp
+++ b/llvm/lib/IR/DIBuilder.cpp
@@ -940,8 +940,7 @@ DISubprogram *DIBuilder::createFunction(
SPFlags, IsDefinition ? CUNode : nullptr, TParams, Decl, nullptr,
ThrownTypes, Annotations, TargetFuncName);
- if (IsDefinition)
-AllSubprograms.push_back(Node);
+ AllSubprograms.push_back(Node);
trackIfUnresolved(Node);
return Node;
}
@@ -978,8 +977,7 @@ DISubprogram *DIBuilder::createMethod(
Flags, SPFlags, IsDefinition ? CUNode : nullptr, TParams, nullptr,
nullptr, ThrownTypes);
- if (IsDefinition)
-AllSubprograms.push_back(SP);
+ AllSubprograms.push_back(SP);
trackIfUnresolved(SP);
return SP;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
