This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dzhidzhoev on Feb 14 2023, 6:29 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 moves the emission of global variables 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 static variables (which are globals in terms of LLVM IR)
which comes in the next patch (D144008), making all the globals handled in
a single place, together with other global and local entities.

Diff Detail

Event Timeline

krisb created this revision.Feb 14 2023, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2023, 6:29 AM
krisb edited the summary of this revision. (Show Details)Feb 17 2023, 6:35 AM
krisb added reviewers: dblaikie, jmmartinez.
krisb added projects: Restricted Project, debug-info.
krisb published this revision for review.Feb 17 2023, 6:35 AM
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, 11:01 AM

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1369–1395

If this function remains identical, maybe save moving it around for a separate patch to reduce the size/possible changes in this patch?

(similarly, could /possibly/ refactor the globals loop into a function in one patch, move where the call is in another, then move the function around (if you like/to bring it closer to usage) in another - making it clear that moving the call site isn't changing any other behavior)

krisb updated this revision to Diff 504610.Mar 13 2023, 5:36 AM

Apply comments and rebase.

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

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

The motivation for this patch is mostly the same as for types:

  • unify/simplify emission of local and non-local entities getting everything done in one place (in our case DwarfDebug::endModule()). Keeping 'non-local' globals emission in DwarfDebug::beginModule() (while 'local' globals should be emitted not earlier than DwarfDebug::endModule()) would make the implementation much more complicated as both 'local' and 'global' globals emission depends on GVMap.
  • make sure IR/debug metadata would not be changed after DWARF entities get created.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1369–1395

If this function remains identical, maybe save moving it around for a separate patch to reduce the size/possible changes in this patch?

There are no changes in the function, so I'll move it in a separate patch.

(similarly, could /possibly/ refactor the globals loop into a function in one patch, move where the call is in another, then move the function around (if you like/to bring it closer to usage) in another - making it clear that moving the call site isn't changing any other behavior)

I'd rather extract all the global entities emission (including types and imports) into a separate function, since DwarfDebug::endModule() is quite large. But I'd prefer to do this after all the functional changes get done, otherwise it may cause additional noise and complicate review, testing, committing, and handling possible issues postcommit.

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

The motivation for this patch is mostly the same as for types:

  • unify/simplify emission of local and non-local entities getting everything done in one place (in our case DwarfDebug::endModule()). Keeping 'non-local' globals emission in DwarfDebug::beginModule() (while 'local' globals should be emitted not earlier than DwarfDebug::endModule()) would make the implementation much more complicated as both 'local' and 'global' globals emission depends on GVMap.
  • make sure IR/debug metadata would not be changed after DWARF entities get created.

Fair enough - some tension between independent changes being independent-ish (so they can be reverted/root caused/etc separately if needed) and keeping things consistent. I'm not insisting on any reordering/separation here if you feel this is the best way to go.

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

Hi @dzhidzhoev !
What's missing to land the rest of the patches? I missed the last discussions and I saw you just rebased this one.

dzhidzhoev added a comment.EditedNov 3 2023, 3:26 AM

Hi @dzhidzhoev !
What's missing to land the rest of the patches? I missed the last discussions and I saw you just rebased this one.

Hello! Sorry for the late response; missed your message. I landed the previous patch yesterday, and I'm going to land the rest of them with an interval of 3 days between each commit to ensure that there are no unexpected issues with them.