Page MenuHomePhabricator

[DebugInfo] Support types, imports and static locals declared in a lexical block (3/5)
Needs ReviewPublic

Authored by krisb on May 16 2022, 8:38 AM.

Details

Summary

This implements support of lexical block scopes for function-local types and static locals. Before this patch and its clang counterparts (see D125694 and D125695), LLVM ignores the fact that such entities are being defined within a lexical block and puts them directly into a parent function.
Consider the following example:

 1  int main() {
 2    int a = 0;
 3    switch(a) {
 4    case 1: {
 5      static const int a = 1;
 6    }	
 7    case 2: {
 8      static const int a = 2;
 9    }
10    }
11  }

All the 'a''s DW_TAG_variable go to subprogram's scope which confuses debuggers when 'a' variable is being inspected.

0x0000002a:   DW_TAG_subprogram
              ...

0x00000043:     DW_TAG_variable
                  DW_AT_name  ("a")
                  DW_AT_type  (0x0000007c "const int")
                  DW_AT_decl_file ("temp.cpp")
                  DW_AT_decl_line (5) 
                  DW_AT_location  (DW_OP_addr 0x402004)

0x00000058:     DW_TAG_variable
                  DW_AT_name  ("a")
                  DW_AT_type  (0x0000007c "const int")
                  DW_AT_decl_file ("temp.cpp")
                  DW_AT_decl_line (8)
                  DW_AT_location  (DW_OP_addr 0x402008)

0x0000006d:     DW_TAG_variable
                  DW_AT_location  (DW_OP_fbreg -8) 
                  DW_AT_name  ("a")
                  DW_AT_decl_file ("temp.cpp")
                  DW_AT_decl_line (2) 
                  DW_AT_type  (0x00000081 "int")

This fixes

In the same time, it also unifies implementation of local imported entities allowing all the function-local declarations to be handled the same way,
thus fixes

This is an alternative implementation for D113741 which relies on localDecls field of DISubprogram or DILexicalBlock for getting local declarations (types, imports or static variable) for a particular local scope.

It has 2 issues from D113741 resolved:

  • Previous patch may misscope type declarations belong to a lexical block. This happened because it wasn't possible to know if there are any types belonging to a particular local scope by the time we started to emit this scope. So, we may skip emission of lexical blocks even if they contain some type declarations, and later couldn't find a proper parent for those types. localDecls tracks all the types declared within a particular local scope, so we no longer skip non-empty lexical blocks.
  • Previous patch didn't have a mapping between a local declaration and its containing subprogram DIE (and CU DIE), which may cause issues like https://reviews.llvm.org/D113741#3206883. Now we are able to create such mapping basing on localDecls of DISubprogram or DILexicalBlock.

Another thing that should be noted that with this patch we will no longer emit local declarations if its parent scope doesn't have a LexicalScope calculated for it (i.e. there are no DILocations that point to this scope or any of its children scopes) unless something refers such a declaration (relevant for types), see changes in llvm/test/Instrumentation/InstrProfiling/debug-info-correlate.ll as an example. I guess this shouldn't affect debugging experience.

Diff Detail

Event Timeline

krisb created this revision.May 16 2022, 8:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
krisb requested review of this revision.May 16 2022, 8:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 16 2022, 8:38 AM

Mark this and D125691 as WIP since I'm still testing the approach on various combinations of debug info and optimization options (O0, O3, thinlto, split-dwarf, split-dwarf-inlining, gline-tables-only, etc.).
Except of the testing purpose itself I'm also trying to catch the issue reported by David here https://reviews.llvm.org/D113741#3437182 for previous implementation (while I hope this approach wouldn't trigger the issue, it'd be good to catch and fix it anyway).

ormris removed a subscriber: ormris.May 16 2022, 11:07 AM

Thanks a lot for the patch! It would be great to get this issue finally fixed. I assume that this is the main patch, other patches in the stack seem like just preparation/adjustments needed for this one to work.

This an alternative implementation for D113741 which relies on localDecls field of DISubprogram or DILexicalBlock for getting local declarations (types, imports or static variable) for a particular local scope.

Can you include a short description of the issue? PR19238 has a good and short example, we can inline it here.

Another thing that should be noted that with this patch we will no longer emit local declarations if its parent scope doesn't have a LexicalScope calculated for it

If a local declaration (a type or a static variable) is optimized away, then with this patch we have no debug information for it, right? For types we have -debug-info-kind=unused-types as a workaround (as shown in lexical-block-retained-types.ll), so the issue is only with static variables that have no uses. Given that they were broken before this patch and there is no way to access them from a debugger (cannot set a breakpoint if a block is optimized away), I think it is fine.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1383

So here we discard LLVM IR metadata that have local declarations tied to a CU, right? This will break compatibility with old LLVM IR. Can we do some upgrade to convert this "old style" metadata the way we expect it now? Does it make sense to support both variants?

krisb edited the summary of this revision. (Show Details)May 23 2022, 4:01 AM
krisb added a comment.May 23 2022, 5:13 AM

Thank you, @asavonic for your comments!

I've added some more details about the issues this is going to fix, hope it'll help to review the patchset.

Thanks a lot for the patch! It would be great to get this issue finally fixed. I assume that this is the main patch, other patches in the stack seem like just preparation/adjustments needed for this one to work.

Correct. The first two patches were extracted to just make this one a bit smaller and easier to review.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1383

Good question. I haven't thought about backward compatibility, not even sure we have some requirements for this. The easiest option seems to increase debug metadata version and drop debug info if it isn't compatible with this one as it done in AutoUpgrade. This prevents producing incorrect DWARF for ill-formed metadata.

Upgrading "old style" metadata would be a bit tricky. Current implementation assumes that all the local declarations (excluding regular local vars and labels) are referenced by parent's localDecls field, including types. Before this patchset there were no ways to find types except by references from their uses. So, to find all the function-local types we may need to iterate over all instructions and parse all debug metadata, which likely increases compile time. Not sure this trade-off is a worth one.

krisb edited the summary of this revision. (Show Details)May 31 2022, 7:36 AM
krisb updated this revision to Diff 434434.Mon, Jun 6, 4:49 AM

Add two tests from D113741.
Fixed crash in DwarfCompileUnit in case of PR55680 (the issue itself wasn't fixed), add a test.

Tested the whole patchset with various combinations of

  • opt levels (O0, O2, O3, ThinLTO, FullLTO),
  • debug info options (-fdebug-types-section, -gline-tables-only, -fno-eliminate-unused-debug-types -gsplit-dwarf, -fsplit-dwarf-inlining),
  • and some additional checks / asserts (like additional verification of abstract / concrete out-of-line tree to be sure local entities are not duplicated, do not go to the wrong tree)

on clang bootstrapping build. No issues so far (except PR55680, which caused not by this and related patches).

This still doesn't guarantee that the patch accounts all the corner cases and no issues would be exposed later, but seems good to start with.
Removing [WIP] status.

krisb retitled this revision from [DebugInfo][WIP] Support types, imports and static locals declared in a lexical block (3/5) to [DebugInfo] Support types, imports and static locals declared in a lexical block (3/5).Mon, Jun 13, 10:26 AM
krisb updated this revision to Diff 438617.Tue, Jun 21, 3:03 AM

Rebase.

krisb updated this revision to Diff 441031.Wed, Jun 29, 8:45 AM

Make DwarfDebug::endModule() to skip function-local decls
CUs 'imports', 'globals' and 'enums' instead of asserting.