Page MenuHomePhabricator

[DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)
AcceptedPublic

Authored by dzhidzhoev on Feb 14 2023, 6:28 AM.

Details

Summary

RFC https://discourse.llvm.org/t/rfc-dwarfdebug-fix-and-improve-handling-imported-entities-types-and-static-local-in-subprogram-and-lexical-block-scopes/68544

Similar to imported declarations, the patch tracks function-local types in
DISubprogram's 'retainedNodes' field. DwarfDebug is adjusted in accordance with
the aforementioned metadata change and provided a support of function-local
types scoped within a lexical block.

The patch assumes that DICompileUnit's 'enums field' no longer tracks local
types and DwarfDebug would assert if any locally-scoped types get placed there.

Diff Detail

Event Timeline

krisb created this revision.Feb 14 2023, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2023, 6:28 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
krisb published this revision for review.Feb 17 2023, 6:31 AM
krisb edited the summary of this revision. (Show Details)
krisb added reviewers: dblaikie, jmmartinez.
krisb added projects: Restricted Project, debug-info.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 6:33 AM
ykhatav added a subscriber: ykhatav.Mar 3 2023, 8:56 AM

Just a few minor comments. Everything else seems good to me.

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
728

I cannot find the link to PR55680. Would you mind sharing it?

You could also reference local-type-as-template-parameter.ll, your test depicts the issue very clearly.

730–731

NIT: You could use find to avoid searching in LexicalBlockDIEs twice.

743–745

NIT: You could use insert/try_emplace instead of using count and operator[]. The assertion would become something like:

auto Inserted = getAbstractScopeDIEs().try_emplace(DS, ScopeDIE);
assert(Inserted.second && "Abstract DIE for this scope exists!");
return ScopeDIE;
krisb marked 2 inline comments as done.Mar 13 2023, 5:34 AM
krisb added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
728

I cannot find the link to PR55680. Would you mind sharing it?

You could also reference local-type-as-template-parameter.ll, your test depicts the issue very clearly.

Here is the link to the issue https://github.com/llvm/llvm-project/issues/55680.
Mentioned the test in the comment. Thank you!

743–745

I'd slightly prefer to keep the original code as it looks a bit more readable to me (in your example, there should be another line to void out unused Inserted variable, otherwise it'd cause a warning).
Additional count() call in the assert() doesn't seem too expensive to me, but if you still think try_emplace() is better, I'll change to it.

krisb updated this revision to Diff 504608.Mar 13 2023, 5:34 AM
krisb marked an inline comment as done.

Apply review comments and rebase.

jmmartinez accepted this revision.Mar 20 2023, 5:22 AM
jmmartinez added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
743–745

Ok for me. If the use of count was outside the assert I would have argued against.

This revision is now accepted and ready to land.Mar 20 2023, 5:22 AM
dblaikie added inline comments.Mar 20 2023, 5:49 PM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
743–745

FWIW I'd lean towards avoiding the extra lookup in the +Asserts build (by using emplace or try_emplace, etc - yeah, with the extra void cast for the non-asserts-unused variable), but wouldn't insist on it.

dzhidzhoev retitled this revision from [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (5/7) to [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7).Mon, May 15, 5:38 AM