[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
jmorse wrote: (" it lacks the DW_AT_abstract_origin that gcc produces..." for the DW_TAG_lexical_scopes referring back to the abstract/declaration subprogram). https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
jmorse wrote: Indeed @ the language, it's difficult to know that this is definitely going to work when I don't have a full understanding of the patch series, thus the caution, I've studied a bit further and even with the full patch series we seem to be missing an important link. Here's a gist: https://gist.github.com/jmorse/091ae6ed42e6a36199b8593a7b81bd49 which contains the LLVM DWARF output for the aforementioned lexical-block-static-var.ll . While it passes the FileCheck lines in the test, it lacks the DW_AT_abstract_origin that gcc produces -- observe that nothing references addresses 8a or 5b. The consumer is then presumably left with a problem: it might be able to discover the static variables A and B, and that they're in a lexical block, but not _which_ lexical block. There's also a risk that I misapplied D144008, which I had to copy-and-paste out of Phab, @dzhidzhoev would you have a public-github branch that contains that patch?. In the IR metadata, each DIGlobalVariable refers into a tree of distinct DILexicalScopes and DISubprograms that's part of the definition subprogram for the relevant function. I suspect that in scenarios where multiple instances of an inline function with a static variable are LTO'd, we might see a repeat of this type uniquing problem: LLVM will pick a single DIGlobalVariable as the location for such an inlined-function static variable, which will then refer into the DILexicalScope/DISubprogram tree for one instance of the inlined function, potentially causing confusion or hard-to-interpret DWARF. I think the natural solution to this is putting DIGlobalVariable scopes into a "declaration" tree of DILexicalScopes/DISubprograms too. I'll try to cook up a precise example of this happening so that it's clear, but have to dash for now. https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
dwblaikie wrote: yeah, I worry this feels all a bit vague (that some things "suggest" other things, that root problems "seem to be" (& some fairly broad descriptions of what they seem to be) - not to nitpick your language, and I appreciate the clarity of the uncertainty, to be sure - but the uncertainty is a bit concerning) Might help to have some specific/narrow example(s) to walk through & discuss how we think they should work? https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
jmorse wrote: > Ah, fair enough, because the subprogram will contian a list of these local > types, then the local type's scope will be a (potentially series of) lexical > scope - so the lexical scopes are preserved? That's certainly the outcome that the original patch series is aiming for -- I'm not completely certain that we can still achieve it. Part of the patch series clones types contained within distinct DISubprograms `retainedNodes` when they're inlined, which suggests that there's a connection between the types and the duplicated/distinct scopes that must be maintained, something that this patch would damage. I suppose I should hold off landing this patch until I fully understand that mechanism, and check we can still reach the desired outcome. > > Given that the ODR-uniquing issue was only found experimentally > > in-the-field, I think we have to accept the incremental approach and > > discover problems by experience. > > Not sure I follow this. I mean: at some point we just have to commit code and see what breaks, to discover obscure requirements. > > Stepping back slightly: I think the ODR-uniquing of these types is directly > > opposed to scope-precision: reducing the number of type definitions down to > > one necessarily means discarding the extra locality information about where > > it's scoped in different function instances. > > Not sure I follow this either - I think that if each concrete lexical scope > references the abstract ones, we could emit correct abstract origins to get > the local static variables and types to be scoped appropriately in the > resulting DWARF/for the consumer. Indeed, if that connection is present then we should be fine. I'm worried that some of the extra plumbing (i.e. the cloning-of-types-during-inlining) indicates that this property requires maintenance during optimisation, and that that maintenance could be impossible in the presence of the ODR-uniquing feature. https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
dwblaikie wrote: > > Maybe it's too big/complex a problem to try to think about, and incremental > > development should overrule here - but may be worth thinking about? > > Urgh. I'll admit that I hadn't thought that far ahead, I just wanted to get > the patch-series going again now that the root problem is identified. > > The DWARF in the existing patch series [0] seems to take a similar direction > to this patch, of putting information into declarations where possible. Have > a look at the check-lines for > llvm/test/DebugInfo/Generic/lexical-block-static-var.ll: > the static-local variables are defined and given a location inside > DW_TAG_lexical_blocks within the abstract definitions of functions, Ah, fair enough, because the subprogram will contian a list of these local types, then the local type's scope will be a (potentially series of) lexical scope - so the lexical scopes are preserved? That's OK-ish, at least not as problematic as I was imagining. (rest of the discussion below here is just discussion - this point ^ is enough to satisfy my concerns for this patch direction, at least) > and then there are some other DW_TAG_lexical_blocks inside the inlined > instances. It's not clear to me how the inlined-lexical-blocks refer back to > the abstract ones. Yeah, that's probably missing - the ideal would be the inlined/concrete non-inlined instance's lexical blocks should have abstract_origins that refer to the abstract lexical blocks. > Given that the ODR-uniquing issue was only found experimentally in-the-field, > I think we have to accept the incremental approach and discover problems by > experience. Not sure I follow this. > Stepping back slightly: I think the ODR-uniquing of these types is directly > opposed to scope-precision: reducing the number of type definitions down to > one _necessarily_ means discarding the extra locality information about where > it's scoped in different function instances. Not sure I follow this either - I think that if each concrete lexical scope references the abstract ones, we could emit correct abstract origins to get the local static variables and types to be scoped appropriately in the resulting DWARF/for the consumer. GCC does this: https://godbolt.org/z/Khrzdq1Wx https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
jmorse wrote: > Maybe it's too big/complex a problem to try to think about, and incremental > development should overrule here - but may be worth thinking about? Urgh. I'll admit that I hadn't thought that far ahead, I just wanted to get the patch-series going again now that the root problem is identified. The DWARF in the existing patch series [0] seems to take a similar direction to this patch, of putting information into declarations where possible. Have a look at the check-lines for llvm/test/DebugInfo/Generic/lexical-block-static-var.ll: the static-local variables are defined and given a location inside DW_TAG_lexical_blocks within the abstract definitions of functions, and then there are some other DW_TAG_lexical_blocks inside the inlined instances. It's not clear to me how the inlined-lexical-blocks refer back to the abstract ones. I don't have a complete understanding of how the entire patch series operates, but types-and-variables-in-declarations appears to be where we're heading. It makes sense for our metadata to reflect this. Given that the ODR-uniquing issue was only found experimentally in-the-field, I think we have to accept the incremental approach and discover problems by experience. Stepping back slightly: I think the ODR-uniquing of these types is directly opposed to scope-precision: reducing the number of type definitions down to one _necessarily_ means discarding the extra locality information about where it's scoped in different function instances. Given that the ODR-uniquing facility already exists, I assume it's not something that can be turned off due to efficiency reasons. Thus we're reduced to workarounds while pushing towards the end-solution. [0] final significant patch https://reviews.llvm.org/D144008 , you have to manually delete the phab-error overlay https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
dwblaikie wrote: No idea if this sufficiently overlaps with, or is orthogonal to the following, but: Currently we don't scope function-local types into the appropriate scope inside a function. It seems like associating types with their declaration may make this harder to address - how would we handle that? (like, we'd have to create scopes for function declarations? somehow associate those with the scopes in concrete definitions of a function?) Maybe it's too big/complex a problem to try to think about, and incremental development should overrule here - but may be worth thinking about? https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
@@ -718,6 +738,29 @@ class MetadataLoader::MetadataLoaderImpl { upgradeCULocals(); } + void cloneLocalTypes() { +for (unsigned I = 0; I < MetadataList.size(); ++I) { adrian-prantl wrote: range-based for? https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
@@ -546,55 +546,75 @@ class MetadataLoader::MetadataLoaderImpl { /// Move local imports from DICompileUnit's 'imports' field to /// DISubprogram's retainedNodes. + /// Move fucntion-local enums from DICompileUnit's enums adrian-prantl wrote: ```suggestion /// Move function-local enums from DICompileUnit's enums ``` https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
@@ -486,6 +486,8 @@ class CGDebugInfo { void EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc, QualType FnType, llvm::Function *Fn = nullptr); + llvm::DIScope *PickCompositeTypeScope(llvm::DIScope *S, StringRef Identifier); adrian-prantl wrote: Can you add a doxygen comment that explains the purpose of this function? https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
github-actions[bot] wrote: :warning: C/C++ code formatter, clang-format found issues in your code. :warning: You can test this locally with the following command: ``bash git-clang-format --diff fdb1bf9b5949b2a97041922405a812a060fce5f4 a7a3f59f3caae556c6613056f634b504d5052964 --extensions h,cpp,c -- clang/test/CodeGenCXX/debug-info-local-types.cpp clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/CGDebugInfo.h clang/test/CodeGen/debug-info-codeview-unnamed.c clang/test/CodeGen/debug-info-unused-types.c clang/test/CodeGen/debug-info-unused-types.cpp clang/test/CodeGenCXX/debug-info-access.cpp clang/test/CodeGenCXX/debug-info-anon-union-vars.cpp clang/test/CodeGenCXX/debug-info-codeview-unnamed.cpp clang/test/CodeGenCXX/debug-info-gline-tables-only-codeview.cpp clang/test/CodeGenCXX/debug-lambda-this.cpp llvm/include/llvm/IR/DIBuilder.h llvm/include/llvm/IR/DebugInfo.h llvm/lib/Bitcode/Reader/MetadataLoader.cpp llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp llvm/lib/IR/DIBuilder.cpp llvm/lib/IR/DebugInfo.cpp llvm/lib/IR/Verifier.cpp llvm/lib/Transforms/Utils/CloneFunction.cpp llvm/unittests/Transforms/Utils/CloningTest.cpp `` View the diff from clang-format here. ``diff diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp index e56aef662a..bf67c1d197 100644 --- a/clang/lib/CodeGen/CGDebugInfo.cpp +++ b/clang/lib/CodeGen/CGDebugInfo.cpp @@ -3903,8 +3903,8 @@ CGDebugInfo::getOrCreateLimitedType(const RecordType *Ty) { return Res; } -llvm::DIScope * -CGDebugInfo::PickCompositeTypeScope(llvm::DIScope *S, StringRef Identifier) { +llvm::DIScope *CGDebugInfo::PickCompositeTypeScope(llvm::DIScope *S, + StringRef Identifier) { using llvm::DISubprogram; // Only adjust the scope for composite types placed into functions. @@ -3947,9 +3947,8 @@ CGDebugInfo::PickCompositeTypeScope(llvm::DIScope *S, StringRef Identifier) { DISubprogram *DeclSP = DBuilder.createFunction( SP->getScope(), SP->getName(), SP->getLinkageName(), SP->getFile(), - SP->getLine(), SP->getType(), SP->getScopeLine(), - Flags, SPFlags, SP->getTemplateParams(), nullptr, nullptr, - SP->getAnnotations()); + SP->getLine(), SP->getType(), SP->getScopeLine(), Flags, SPFlags, + SP->getTemplateParams(), nullptr, nullptr, SP->getAnnotations()); SP->replaceDeclaration(DeclSP); return DeclSP; diff --git a/llvm/lib/IR/DIBuilder.cpp b/llvm/lib/IR/DIBuilder.cpp index c6215951e7..705a1312e1 100644 --- a/llvm/lib/IR/DIBuilder.cpp +++ b/llvm/lib/IR/DIBuilder.cpp @@ -361,10 +361,10 @@ DIBuilder::createTemplateAlias(DIType *Ty, StringRef Name, DIFile *File, unsigned LineNo, DIScope *Context, DINodeArray TParams, uint32_t AlignInBits, DINode::DIFlags Flags, DINodeArray Annotations) { - auto *T = DIDerivedType::get(VMContext, dwarf::DW_TAG_template_alias, Name, File, -LineNo, getNonCompileUnitScope(Context), Ty, 0, -AlignInBits, 0, std::nullopt, std::nullopt, Flags, -TParams.get(), Annotations); + auto *T = DIDerivedType::get(VMContext, dwarf::DW_TAG_template_alias, Name, + File, LineNo, getNonCompileUnitScope(Context), + Ty, 0, AlignInBits, 0, std::nullopt, + std::nullopt, Flags, TParams.get(), Annotations); if (isa_and_nonnull(Context)) getSubprogramNodesTrackingVector(Context).emplace_back(T); return T; `` https://github.com/llvm/llvm-project/pull/119001 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [DebugInfo] Place local ODR-uniqued types in decl DISubprograms (PR #119001)
llvmbot wrote: @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Jeremy Morse (jmorse) Changes This is a reapplication and fix for the code in #75385 , support for function-local types in debug-info. The first commit is a reapplication of that code (plus some rebasing-maintenance), the second commit shifts a bit of debug-info metadata in the type hierarchy to make it safer when being unique'd. The root problem in #75385 seems to be that multiple copies of `DICompositeTypes` in LTO contexts, where multiple definitions of functions appear in debug-info, get unique'd and break various links between types and subprograms. Second commit message follows: ~ There are two flavours of DISubprogram: declarations, which are unique'd and abstractly describe the function in question. There are also definition DISubprograms which correspond to real instances, some of which are inlined, duplicated across translation units in LTO, or otherwise can have multiple instances. Given that LLVM sometimes force-uniques types by their ODR-name, see the enableDebugTypeODRUniquing feature, we shouldn't place types that might be unique'd into duplicated contexts like definition DISubprograms. Instead, place them into the declaration. This slightly bends the existing approach where only functions that have a separate declaratrion to their definition get a declaration-DISubprogram. A single function in a translation unit might now get a declaration where it didn't before, if it contains an ODR-unique'd type declaration. This seems reasonable given that the LLVM idea of a declaration doesn't have to exactly match source-language ideas. The added cpp test checks that such ORD-unique'd types are detected and placed in the declaration DISubprogram, creating one if necessary. The IR test ensures that nothing changes after a round-trip with enableDebugTypeODRUniquing turned on. Changes to the codeview test are because the order of metadata changes a little. --- Patch is 111.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/119001.diff 35 Files Affected: - (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+55) - (modified) clang/lib/CodeGen/CGDebugInfo.h (+2) - (modified) clang/test/CodeGen/debug-info-codeview-unnamed.c (+8-8) - (modified) clang/test/CodeGen/debug-info-unused-types.c (+9-7) - (modified) clang/test/CodeGen/debug-info-unused-types.cpp (+8-6) - (modified) clang/test/CodeGenCXX/debug-info-access.cpp (+1-1) - (modified) clang/test/CodeGenCXX/debug-info-anon-union-vars.cpp (+6-6) - (modified) clang/test/CodeGenCXX/debug-info-codeview-unnamed.cpp (+36-27) - (modified) clang/test/CodeGenCXX/debug-info-gline-tables-only-codeview.cpp (+2-2) - (added) clang/test/CodeGenCXX/debug-info-local-types.cpp (+79) - (modified) clang/test/CodeGenCXX/debug-lambda-this.cpp (+2-2) - (modified) llvm/include/llvm/IR/DIBuilder.h (+3-3) - (modified) llvm/include/llvm/IR/DebugInfo.h (+10-1) - (modified) llvm/lib/Bitcode/Reader/MetadataLoader.cpp (+78-33) - (modified) llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp (+40-20) - (modified) llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h (+8-8) - (modified) llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (+10-3) - (modified) llvm/lib/IR/DIBuilder.cpp (+25-8) - (modified) llvm/lib/IR/DebugInfo.cpp (+10-4) - (modified) llvm/lib/IR/Verifier.cpp (+3-3) - (modified) llvm/lib/Transforms/Utils/CloneFunction.cpp (+14-1) - (added) llvm/test/Bitcode/clone-local-types.ll (+46) - (modified) llvm/test/Bitcode/upgrade-cu-locals.ll (+41-27) - (modified) llvm/test/Bitcode/upgrade-cu-locals.ll.bc () - (added) llvm/test/DebugInfo/Generic/inlined-local-type.ll (+128) - (added) llvm/test/DebugInfo/Generic/lexical-block-retained-types.ll (+55) - (added) llvm/test/DebugInfo/Generic/lexical-block-types.ll (+425) - (modified) llvm/test/DebugInfo/Generic/verifier-invalid-disubprogram.ll (+1-1) - (added) llvm/test/DebugInfo/X86/local-type-as-template-parameter.ll (+161) - (modified) llvm/test/DebugInfo/X86/set.ll (+2-2) - (renamed) llvm/test/DebugInfo/X86/split-dwarf-local-import.ll (-1) - (renamed) llvm/test/DebugInfo/X86/split-dwarf-local-import2.ll (-1) - (renamed) llvm/test/DebugInfo/X86/split-dwarf-local-import3.ll () - (added) llvm/test/DebugInfo/local-odr-types-hiearchy.ll (+124) - (modified) llvm/unittests/Transforms/Utils/CloningTest.cpp (+105) ``diff diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp index 60f32f76109e9a..e56aef662a571d 100644 --- a/clang/lib/CodeGen/CGDebugInfo.cpp +++ b/clang/lib/CodeGen/CGDebugInfo.cpp @@ -1235,6 +1235,7 @@ CGDebugInfo::getOrCreateRecordFwdDecl(const RecordType *Ty, // Don't include a linkage name in line tables only. if (CGM.getCodeGenOpts().hasReducedDebugInfo()) Identifier = getTypeIdentifier(Ty, CGM, TheCU); + Ctx = PickCompositeTypeScope(Ctx, Identifier); llvm::DICompositeType *RetTy = DBuilder.createReplacea