This is an archive of the discontinued LLVM Phabricator instance.

[DwarfDebug] Move emission of types from beginModule() to endModule() (7/7)
AcceptedPublic

Authored by dzhidzhoev on Feb 14 2023, 6:25 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

This patch proposes to move the emission of types and type-like entities from
DwarfDebug::beginModule() to DwarfDebug::endModule(). Effectively, this changes
nothing but the order of debug entities in the resulting DWARF.

This is needed to simplify DWARF emission in a context of proper support of
function-local types which comes in the next patch (D144006), making all
the types handled in a single place, together with other global and
local entities.

No functional changes intended.

Diff Detail

Event Timeline

krisb created this revision.Feb 14 2023, 6:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2023, 6:25 AM
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.
ykhatav added a subscriber: ykhatav.Mar 3 2023, 8:56 AM
jmmartinez accepted this revision.Mar 10 2023, 6:09 AM
This revision is now accepted and ready to land.Mar 10 2023, 6:09 AM
dblaikie accepted this revision.Mar 10 2023, 10:57 AM

Change seems OK - but might be nice to know a bit more about why this is helpful for correcting function-local entities?

krisb updated this revision to Diff 504606.Mar 13 2023, 5:33 AM

Rebase.

krisb added a comment.Mar 13 2023, 5:33 AM

Change seems OK - but might be nice to know a bit more about why this is helpful for correcting function-local entities?

Strictly speaking, moving emission of non-local types isn't required to get local types worked. This is mostly done for the sake of unification/simplification since we are stopping emitting anything in DwarfDebug::beginModule(). There is another motivation point, but it is a bit speculative: we may get IR (as well as debug metadata) changed after DwarfDebug::beginModule(), and if this happened, emitted debug info would be out-of-date. I've found such a case only once, and considered it rather a bug (see for details https://reviews.llvm.org/D113653). I'm not aware of any other cases, but it's still possible to change debug metadata after DwarfDebug::beginModule(), so it seems much safer to emit DWARF at a time when we do not expect any changes in IR (i.e. DwarfDebug::endModule()).

Change seems OK - but might be nice to know a bit more about why this is helpful for correcting function-local entities?

Strictly speaking, moving emission of non-local types isn't required to get local types worked. This is mostly done for the sake of unification/simplification since we are stopping emitting anything in DwarfDebug::beginModule(). There is another motivation point, but it is a bit speculative: we may get IR (as well as debug metadata) changed after DwarfDebug::beginModule(), and if this happened, emitted debug info would be out-of-date. I've found such a case only once, and considered it rather a bug (see for details https://reviews.llvm.org/D113653). I'm not aware of any other cases, but it's still possible to change debug metadata after DwarfDebug::beginModule(), so it seems much safer to emit DWARF at a time when we do not expect any changes in IR (i.e. DwarfDebug::endModule()).

Perhaps this could be done as a separate change, probably after this sequence is in and baked a while, so that it can be backed out/reverted/understood separately from the rest of this sequence? It's already a fairly involved sequence of patches, would be good to ensure it's not labored with other complexity that could reasonably be considered independent.

Moving this commit to the end of a sequence, so it can be handled after key changes are landed. Addressing @dblaikie’s comment.

dzhidzhoev retitled this revision from [DwarfDebug] Move emission of types from beginModule() to endModule() (4/7) to [DwarfDebug] Move emission of types from beginModule() to endModule() (7/7).May 15 2023, 5:42 AM