This is an archive of the discontinued LLVM Phabricator instance.

[DebugMetadata][DwarfDebug][CodeView] Support function-local static variables in lexical block scopes (6/7)
AcceptedPublic

Authored by dzhidzhoev on Feb 14 2023, 6:30 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 and types, the patch tracks function-local
static variables (globals in terms of LLVM IR) in DISubprogram's 'retainedNodes'.
DwarfDebug is adjusted in accordance with the aforementioned metadata change and
provided a support of static locals scoped by a lexical block.
CodeViewDebug is modified to collect global variables from DISubprogram's
'retainedNodes' too.

The patch assumes that DICompileUnit's 'globals' no longer tracks static locals
and DwarfDebug would assert if any locally-scoped variables get placed there.

Diff Detail

Event Timeline

krisb created this revision.Feb 14 2023, 6:30 AM
Herald added a project: Restricted Project. · View Herald Transcript
krisb published this revision for review.Feb 17 2023, 6:35 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 1 2023, 6:21 AM
ykhatav added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1451–1455

Have you thought about adding this support to CodeView as well? I believe that a similar code could be added to CodeViewDebug::endModule() for the CodeView emission to display static locals in proper lexical blocks.

krisb added inline comments.Mar 2 2023, 7:54 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1451–1455

Unfortunately, I'm not quite familiar with CodeView, but I'm going to take a look at it before committing clang's part (see https://reviews.llvm.org/D125694), since it may cause issues with other debugging formats. But if you want to go ahead I'll be happy if you could reuse this patch or any other from the patchset to improve CodeView.

ykhatav added inline comments.Mar 3 2023, 8:58 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1451–1455

Okay, i'll reuse your patch and take a stab at it! Thanks!

I've just finished reading and playing with the whole set of patches. Beside some minor nitpicks I've already posted everything else looks good to me.

I also tried compiling LLVM with the patches to see if there was any difference in size in clang's binary (w/debug info and static linking): 1454920 Kb before the patch vs 1454684 Kb after. I've got simiar results when trying with split bash.

I tested the code on some simple examples and it worked as expected.

jmmartinez accepted this revision.Mar 10 2023, 6:10 AM
This revision is now accepted and ready to land.Mar 10 2023, 6:10 AM

@aprantl

Since this changes what IR goes where - are there concerns you folks have for auto-upgrade/cross compatibility?

@krisb - how's this handle cases of old IR being used in a newer compiler/linker?

krisb updated this revision to Diff 504611.Mar 13 2023, 5:38 AM

Rebase.

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

@jmmartinez

I've just finished reading and playing with the whole set of patches. Beside some minor nitpicks I've already posted everything else looks good to me.

I also tried compiling LLVM with the patches to see if there was any difference in size in clang's binary (w/debug info and static linking): 1454920 Kb before the patch vs 1454684 Kb after. I've got simiar results when trying with split bash.

I tested the code on some simple examples and it worked as expected.

Thank you so much for your efforts! I really appreciate your help with reviewing this patchset.

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

@dblaikie

@krisb - how's this handle cases of old IR being used in a newer compiler/linker?

There is no backward compatibility, old IR may cause DwarfDebug crash (it asserts that old IR is no longer supported) or miscompilation.
Implementing AutoUpgrade seems to be quite easy, but as far as I can see almost all the changes in debug metadata before were done w/o backward compatibility, so I haven't considered it yet.

@dblaikie

@krisb - how's this handle cases of old IR being used in a newer compiler/linker?

There is no backward compatibility, old IR may cause DwarfDebug crash (it asserts that old IR is no longer supported) or miscompilation.
Implementing AutoUpgrade seems to be quite easy, but as far as I can see almost all the changes in debug metadata before were done w/o backward compatibility, so I haven't considered it yet.

I don't /think/ that's the case - I know Apple used to have a pretty significant dependence on IR upgrading at least invalidating the IR metadata & dropping it (so maybe all you need to do is update the debug info IR verifier so that old IR gets deemed invalid and dropped - not necessarily upgraded).

@aprantl

Since this changes what IR goes where - are there concerns you folks have for auto-upgrade/cross compatibility?

@krisb - how's this handle cases of old IR being used in a newer compiler/linker?

Can you add an auto-upgrade test to llvm/test/BitCode that tests that we can still import the old format?

I don't /think/ that's the case - I know Apple used to have a pretty significant dependence on IR upgrading at least invalidating the IR metadata & dropping it (so maybe all you need to do is update the debug info IR verifier so that old IR gets deemed invalid and dropped - not necessarily upgraded).

We've supported upgrading older debug info metadata BitCode for the last couple of years. See the code in llvm/lib/Bitcode/Reader/MetadataLoader.cpp. We never supported reading older textual IR.

dzhidzhoev retitled this revision from [DebugMetadata][DwarfDebug] Support function-local static variables in lexical block scopes (7/7) to [DebugMetadata][DwarfDebug] Support function-local static variables in lexical block scopes (6/7).May 15 2023, 5:41 AM

Addressing @aprantl comments.

Added an Auto-Updater for the commit, moving static locals from
DICompileUnit's globals to a supposedly correspoding DISubprogram, that
embraces a scope of a DIGlobalVariableExpression being reattached.

aprantl accepted this revision.May 19 2023, 4:38 PM
aprantl added inline comments.
llvm/test/Bitcode/upgrade-cu-static-locals.ll
19

Can you remove all attributes that aren't strictly needed? That will make the test easier to maintain.

Got rid of nondetermistic order of metadata being moved in MetadataLoader.cpp.
Removed unnecessary attributes, addressing @aprantl comment.

dzhidzhoev updated this revision to Diff 529108.EditedJun 6 2023, 5:31 PM

Made AutoUpdater move function-local imports/enums/globals to their DISubprograms. A code for imports/enums/globals is added incrementally in
corresponding patches to maintain backward compatibility during landing.

Made CodeViewDebug collect global variables from DISubprogram’s ‘retainedNodes’ in accordance with metadata change.

Made Verifier assert if any locally-scoped entities are found in enums/globals/imports fields of DICompileUnit.

dzhidzhoev retitled this revision from [DebugMetadata][DwarfDebug] Support function-local static variables in lexical block scopes (6/7) to [DebugMetadata][DwarfDebug][CodeView] Support function-local static variables in lexical block scopes (6/7).Jun 6 2023, 5:33 PM
dzhidzhoev edited the summary of this revision. (Show Details)

@akhuang @rnk @ykhatav could you please take a look at CodeViewReader.cpp change? Frankly, I am new to that format.

@akhuang @rnk @ykhatav could you please take a look at CodeViewReader.cpp change? Frankly, I am new to that format.

Thanks for taking care of this. Could you add a CodeView-specific test to make sure that the changes work as expected?

@akhuang @rnk @ykhatav could you please take a look at CodeViewReader.cpp change? Frankly, I am new to that format.

Thanks for taking care of this. Could you add a CodeView-specific test to make sure that the changes work as expected?

It seems that llvm/test/DebugInfo/COFF/global_visibility.ll test ensures that local/static local/global variables are handled properly. DISubprogram's retainedNodes references a few static local variables, and we check that these variables are present within CodeView dump. Is there a corner case I probably miss?

rnk added a comment.Jun 7 2023, 11:12 AM

I think the CodeView code changes and test updates look correct, based on my understanding of the schema change. Thanks for updating it!

@akhuang @rnk @ykhatav could you please take a look at CodeViewReader.cpp change? Frankly, I am new to that format.

Thanks for taking care of this. Could you add a CodeView-specific test to make sure that the changes work as expected?

It seems that llvm/test/DebugInfo/COFF/global_visibility.ll test ensures that local/static local/global variables are handled properly. DISubprogram's retainedNodes references a few static local variables, and we check that these variables are present within CodeView dump. Is there a corner case I probably miss?

Thanks for pointing that out. The CodeView changes look good to me.