This is an archive of the discontinued LLVM Phabricator instance.

[DwarfDebug] Move emission of global vars, types and imports to endModule() (2/5)
AbandonedPublic

Authored by krisb on Nov 29 2021, 6:13 AM.

Details

Summary

This patch proposes to move the emission of global variables, types, imported entities, etc from DwarfDebug::beginModule() to DwarfDebug::endModule().
Effectively, this changes nothing but the order of debug entities.

Note that the order of emitted compile units may also be changed as now we emit units that contain subprograms first and then all other non-empty units.

This is a part extracted from D113741.

The motivation behind this change is the following:
(1) DwarfDebug::beginModule() is run at the very beginning of backend's pipeline, from this time IR can be significantly changed by target-specific passes, and debug info never accounts for these changes (for example, address space lowering done by generic-to-nvvm for NVPTX backend).
(2) imported subprogram names should refer to an abstract subprogram if it exists, but it isn't known in DwarfDebug::beginModule() (it's possible to make some guesses based on location info, but it's not quite reliable);
(3) aforementioned entities if they are scoped within a bracketed block (subject of D113741) couldn't be emitted in DwarfDebug::beginModule() (they need parent emitted first). Another problem is if we try to gather some information about local entities and defer their emission (till subprogram's processing or DwarfDebug::endModule()) all the gathered details might be irrelevant by the time the entities are being emitted (because of (1)).

This patch isn't intended to fix something but the issue (1), but I believe it should also simplify reviewing and testing of D113741.

It also removes some comments that I believe are no longer relevant.

Diff Detail

Event Timeline

krisb created this revision.Nov 29 2021, 6:13 AM
krisb requested review of this revision.Nov 29 2021, 6:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2021, 6:13 AM
krisb edited the summary of this revision. (Show Details)Nov 29 2021, 6:16 AM
krisb added a project: debug-info.

(1) DwarfDebug::beginModule() is run at the very beginning of backend's pipeline, from this time IR can be significantly changed by target-specific passes, and debug info never accounts for these changes (for example, address space lowering done by generic-to-nvvm for NVPTX backend).

Could you point to/provide inline comments on any tests that demonstrate this difference?

Are all the other tests just shifting things around/not changing the content?

@dblaikie

Could you point to/provide inline comments on any tests that demonstrate this difference?
Are all the other tests just shifting things around/not changing the content?

Actually, all the changes in tests in this patch just reflect the changed order of debug entities, non of them reproduced the issue (1).

Perhaps I stated the motivation in the wrong order. The main goal of the patch is to make reviewing D113741 easier, and all changes in the tests just reflect the new order of debug info; thus the tests from this PR could be removed from D113741.

Concerning (1) from the description is not about an actual observable bug, but rather about a design flaw that could cause a bug in the future. To implement D113741 we could in theory collect debug info data in DwarfDebug::beginModule, but in this case, no one can guarantee that the collected info will be valid till endFunctionImpl() or endModule(). NVPTX indeed changes globals in a module, but it's hard to pinpoint a concrete test case that would show the problem, and it is not covered by the current test suite.
The patch eliminates (module) passes between stages of debug info emission, thus in my opinion, the design becomes more reliable. It's much better to guarantee no changes in IR rather than argue that "reasonable" changes in IR could not affect DI.

dblaikie accepted this revision.Nov 30 2021, 11:53 AM

Sounds good - thanks for the context!

This revision is now accepted and ready to land.Nov 30 2021, 11:53 AM
krisb reopened this revision.Feb 23 2022, 11:17 PM
This revision is now accepted and ready to land.Feb 23 2022, 11:17 PM
krisb updated this revision to Diff 411130.Feb 24 2022, 7:38 AM

Rebase & align checks with new order in the following tests:

  • llvm/test/DebugInfo/X86/debug-info-access.ll
  • llvm/test/DebugInfo/X86/tu-to-non-tu.ll

No functional changes.

krisb updated this revision to Diff 429718.May 16 2022, 8:00 AM

Rebase on ToT.

Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 8:00 AM
krisb retitled this revision from [DwarfDebug] Move emission of global vars, types and imports to endModule() to [DwarfDebug] Move emission of global vars, types and imports to endModule() (2/5).
krisb edited the summary of this revision. (Show Details)
krisb updated this revision to Diff 436958.Jun 14 2022, 2:55 PM

Rebased