[RFC][clang][DebugInfo] Allow function-local statics and types to be scoped within a lexical block

[krisb]

Summary

This is almost a reincarnation of https://reviews.llvm.org/D15977 originally
implemented by Amjad Aboud. It was discussed on llvm-dev [0], committed
with its backend counterpart [1], but finally reverted [2].

This patch makes clang to emit debug info for function-local static variables,
records (classes, structs and unions) and typdefs correctly scoped if
those function-local entites defined within a lexical (bracketed) block.

Before this patch, clangs emits all those entities directly scoped in
DISubprogram no matter where they were really defined, causing
debug info loss (reported several times in [3], [4], [5]).

Backend support is at D113741.

[0] https://lists.llvm.org/pipermail/llvm-dev/2015-November/092551.html
[1] https://reviews.llvm.org/rG30e7a8f694a19553f64b3a3a5de81ce317b9ec2f
[2] https://reviews.llvm.org/rGdc4531e552af6c880a69d226d3666756198fbdc8
[3] https://bugs.llvm.org/show_bug.cgi?id=19238
[4] https://bugs.llvm.org/show_bug.cgi?id=23164
[5] https://bugs.llvm.org/show_bug.cgi?id=44695

[aprantl]

This looks reasonable to me. Thanks!

clang/lib/CodeGen/CGDebugInfo.cpp

234

maybe use LexicalBlockMap.insert({&D, LexicalBlockStack.back()}); to make it clear that we are not going to overwrite an entry?

[krisb]

Applied the comment & silence clang-format.

[krisb]

@aprantl thank you for taking a look at this!

[krisb]

Ping

[dblaikie]

Guessing Adrian probably meant to sign off on this, judging by his phrasing but maybe forgot to hit the "accept" button.

[JDevlieghere]

Hey Kristina, this broke TestSetData.py on GreenDragon: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/39089/

Since the bot has been red for several hours I went ahead and reverted your change in 4cb79294e8df8c91ae15264d1014361815d34a53.

[krisb]

In D113743#3173981, @JDevlieghere wrote:

Hey Kristina, this broke TestSetData.py on GreenDragon: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/39089/

Since the bot has been red for several hours I went ahead and reverted your change in 4cb79294e8df8c91ae15264d1014361815d34a53.

Thank you for taking care of this!
I'm looking at the issue, but it's been taking more time than I expected.

This doesn't seem like a flaw of the patch, but likely is a lack of support of records/typedefs scoped within a bracketed block from lldb side.
I see lldb couldn't handle cases like

int foo(int a) {
  {
    typedef int Int;
    Int local = a;
    return local;
  }
}

which produces the same error as for TestSetData.py:

Process 2487354 stopped
* thread #1, name = 'a.out', stop reason = step over
    frame #0: 0x000000000040111d a.out`foo(a=1) at test_lldb.cpp:5:12
   2   	  {
   3   	    typedef int Int;
   4   	    Int local = a;
-> 5   	    return local;
   6   	  }
   7   	}
   8   	
(lldb) p local
error: expression failed to parse:
error: <lldb wrapper prefix>:45:31: no member named 'local' in namespace '$__lldb_local_vars'
    using $__lldb_local_vars::local;
          ~~~~~~~~~~~~~~~~~~~~^
error: <user expression 0>:1:1: use of undeclared identifier 'local'
local
^

[dblaikie]

In D113743#3174073, @krisb wrote:

In D113743#3173981, @JDevlieghere wrote:

Hey Kristina, this broke TestSetData.py on GreenDragon: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/39089/

Since the bot has been red for several hours I went ahead and reverted your change in 4cb79294e8df8c91ae15264d1014361815d34a53.

Thank you for taking care of this!
I'm looking at the issue, but it's been taking more time than I expected.

This doesn't seem like a flaw of the patch, but likely is a lack of support of records/typedefs scoped within a bracketed block from lldb side.
I see lldb couldn't handle cases like

int foo(int a) {
  {
    typedef int Int;
    Int local = a;
    return local;
  }
}

which produces the same error as for TestSetData.py:

Process 2487354 stopped
* thread #1, name = 'a.out', stop reason = step over
    frame #0: 0x000000000040111d a.out`foo(a=1) at test_lldb.cpp:5:12
   2   	  {
   3   	    typedef int Int;
   4   	    Int local = a;
-> 5   	    return local;
   6   	  }
   7   	}
   8   	
(lldb) p local
error: expression failed to parse:
error: <lldb wrapper prefix>:45:31: no member named 'local' in namespace '$__lldb_local_vars'
    using $__lldb_local_vars::local;
          ~~~~~~~~~~~~~~~~~~~~^
error: <user expression 0>:1:1: use of undeclared identifier 'local'
local
^

Not super surprising that lldb might not be able to deal with DWARF in this shape - is LLDB support important to you/something you plan to work on for this DWARF? Otherwise we might need to opt-out of this functionality when tuning for LLDB, for instance. Unless LLDB-invested folks are interested in doing the integration work (& even then, maybe disabling it for LLDB tuning until that's fixed). @aprantl @JDevlieghere

[krisb]

In D113743#3175301, @dblaikie wrote:

Not super surprising that lldb might not be able to deal with DWARF in this shape - is LLDB support important to you/something you plan to work on for this DWARF? Otherwise we might need to opt-out of this functionality when tuning for LLDB, for instance. Unless LLDB-invested folks are interested in doing the integration work (& even then, maybe disabling it for LLDB tuning until that's fixed). @aprantl @JDevlieghere

Thank you for the suggestion!
I implemented a small workaround that makes lldb behaves like if the types have subprogram scope (see https://reviews.llvm.org/D115277), not sure I'll be able to do something better any time soon.
So, if D115277 isn't acceptable, and nobody volunteers to implement this properly, the only way to proceed is to turn this functionality off for lldb.

[krisb]

Rebase on top of 'main'.

[dblaikie]

In D113743#3177437, @krisb wrote:

In D113743#3175301, @dblaikie wrote:

Not super surprising that lldb might not be able to deal with DWARF in this shape - is LLDB support important to you/something you plan to work on for this DWARF? Otherwise we might need to opt-out of this functionality when tuning for LLDB, for instance. Unless LLDB-invested folks are interested in doing the integration work (& even then, maybe disabling it for LLDB tuning until that's fixed). @aprantl @JDevlieghere

Thank you for the suggestion!
I implemented a small workaround that makes lldb behaves like if the types have subprogram scope (see https://reviews.llvm.org/D115277), not sure I'll be able to do something better any time soon.
So, if D115277 isn't acceptable, and nobody volunteers to implement this properly, the only way to proceed is to turn this functionality off for lldb.

Thanks for having a go at a fix/workaround here.

I think we'll still need to hear from Apple folks (@aprantl @JDevlieghere @shafik ) - I'm not sure what their backwards compatibility (new clang-produced binaries being debugged with an old lldb) requirements are. If they need some backcompat there, then I guess they/we/you would still need to disable this DWARF fidelity improvement when tuning for lldb. Hopefully that can be avoided, but I don't know.

[krisb]

In D113743#3343481, @dblaikie wrote:

In D113743#3177437, @krisb wrote:

In D113743#3175301, @dblaikie wrote:

Not super surprising that lldb might not be able to deal with DWARF in this shape - is LLDB support important to you/something you plan to work on for this DWARF? Otherwise we might need to opt-out of this functionality when tuning for LLDB, for instance. Unless LLDB-invested folks are interested in doing the integration work (& even then, maybe disabling it for LLDB tuning until that's fixed). @aprantl @JDevlieghere

Thank you for the suggestion!
I implemented a small workaround that makes lldb behaves like if the types have subprogram scope (see https://reviews.llvm.org/D115277), not sure I'll be able to do something better any time soon.
So, if D115277 isn't acceptable, and nobody volunteers to implement this properly, the only way to proceed is to turn this functionality off for lldb.

Thanks for having a go at a fix/workaround here.

I think we'll still need to hear from Apple folks (@aprantl @JDevlieghere @shafik ) - I'm not sure what their backwards compatibility (new clang-produced binaries being debugged with an old lldb) requirements are. If they need some backcompat there, then I guess they/we/you would still need to disable this DWARF fidelity improvement when tuning for lldb. Hopefully that can be avoided, but I don't know.

Thank you!

FYI. I'm going to split this patch into two: one for local statics and one for types and other. The part for static variables seems clean and safe so we can let it go once backend will be ready for it.

[krisb]

Split on two: D125694 and D125695.

clang/lib/CodeGen/CGDebugInfo.h

#include "clang/Basic/OpenCLExtensionTypes.def"
 
   /// Cache of replaceable forward declarations (functions and
   /// variables) to RAUW at the end of compilation.
   std::vector<std::pair<const DeclaratorDecl *, llvm::TrackingMDRef>>
       FwdDeclReplaceMap;
 
   /// Keep track of our current nested lexical block.
   std::vector<llvm::TypedTrackingMDRef<llvm::DIScope>> LexicalBlockStack;
+
+  /// Map of AST declaration to its lexical block scope.
+  llvm::DenseMap<const Decl *, llvm::TypedTrackingMDRef<llvm::DIScope>>
+      LexicalBlockMap;
+
   llvm::DenseMap<const Decl *, llvm::TrackingMDRef> RegionMap;
   /// Keep track of LexicalBlockStack counter at the beginning of a
   /// function. This is used to pop unbalanced regions at the end of a
   /// function.
   std::vector<unsigned> FnBeginRegionCount;
 
   /// This is a storage for names that are constructed on demand. For
   /// example, C++ destructors, C++ operators etc..
public:
   llvm::DIImportedEntity *EmitNamespaceAlias(const NamespaceAliasDecl &NA);
 
   /// Emit record type's standalone debug info.
   llvm::DIType *getOrCreateRecordType(QualType Ty, SourceLocation L);
 
   /// Emit an Objective-C interface type standalone debug info.
   llvm::DIType *getOrCreateInterfaceType(QualType Ty, SourceLocation Loc);
 
+  /// Map AST declaration to its lexical block scope if available.
+  void recordDeclarationLexicalScope(const Decl &D);
+
+  /// Get lexical scope of AST declaration.
+  llvm::DIScope *getDeclarationLexicalScope(const Decl *D);
+
   /// Emit standalone debug info for a type.
   llvm::DIType *getOrCreateStandaloneType(QualType Ty, SourceLocation Loc);
 
   /// Add heapallocsite metadata for MSAllocator calls.
   void addHeapAllocSiteMetadata(llvm::CallBase *CallSite, QualType AllocatedTy,
                                 SourceLocation Loc);
 
   void completeType(const EnumDecl *ED);

clang/lib/CodeGen/CGDebugInfo.cpp

llvm::DIScope *CGDebugInfo::getContextDescriptor(const Decl *Context,
 
   if (const auto *RDecl = dyn_cast<RecordDecl>(Context))
     if (!RDecl->isDependentType())
       return getOrCreateType(CGM.getContext().getTypeDeclType(RDecl),
                              TheCU->getFile());
   return Default;
 }
 
+void CGDebugInfo::recordDeclarationLexicalScope(const Decl &D) {
+  assert(LexicalBlockMap.find(&D) == LexicalBlockMap.end() &&
+         "D is already mapped to a lexical block scope");
+  if (!LexicalBlockStack.empty())
+    LexicalBlockMap.insert({&D, LexicalBlockStack.back()});
+}
+
+llvm::DIScope *CGDebugInfo::getDeclarationLexicalScope(const Decl *D) {
+  auto I = LexicalBlockMap.find(D);
+  if (I != LexicalBlockMap.end())
+    return I->second;
+  return getDeclContextDescriptor(cast<Decl>(D));
+}
+
 PrintingPolicy CGDebugInfo::getPrintingPolicy() const {
   PrintingPolicy PP = CGM.getContext().getPrintingPolicy();
 
   // If we're emitting codeview, it's important to try to match MSVC's naming so
   // that visualizers written for MSVC will trigger for our class names. In
   // particular, we can't have spaces between arguments of standard templates
   // like basic_string and vector, but we must have spaces between consecutive
   // angle brackets that close nested template argument lists.
llvm::DIType *CGDebugInfo::CreateType(const TypedefType *Ty,
 
   if (Ty->getDecl()->hasAttr<NoDebugAttr>())
     return Underlying;
 
   // We don't set size information, but do specify where the typedef was
   // declared.
   SourceLocation Loc = Ty->getDecl()->getLocation();
 
+  llvm::DIScope *TDContext = getDeclarationLexicalScope(Ty->getDecl());
   uint32_t Align = getDeclAlignIfRequired(Ty->getDecl(), CGM.getContext());
   // Typedefs are derived from some other type.
   llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(Ty->getDecl());
   return DBuilder.createTypedef(Underlying, Ty->getDecl()->getName(),
                                 getOrCreateFile(Loc), getLineNumber(Loc),
-                                getDeclContextDescriptor(Ty->getDecl()), Align,
-                                Annotations);
+                                TDContext, Align, Annotations);
 }
 
 static unsigned getDwarfCC(CallingConv CC) {
   switch (CC) {
   case CC_C:
     // Avoid emitting DW_AT_calling_convention if the C convention was used.
     return 0;
 
llvm::DIType *CGDebugInfo::CreateEnumType(const EnumType *Ty) {
   // marked node and just return it.
   if (isImportedFromModule || !ED->getDefinition()) {
     // Note that it is possible for enums to be created as part of
     // their own declcontext. In this case a FwdDecl will be created
     // twice. This doesn't cause a problem because both FwdDecls are
     // entered into the ReplaceMap: finalize() will replace the first
     // FwdDecl with the second and then replace the second with
     // complete type.
-    llvm::DIScope *EDContext = getDeclContextDescriptor(ED);
+    llvm::DIScope *EDContext = getDeclarationLexicalScope(ED);
     llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation());
     llvm::TempDIScope TmpContext(DBuilder.createReplaceableCompositeType(
         llvm::dwarf::DW_TAG_enumeration_type, "", TheCU, DefUnit, 0));
 
     unsigned Line = getLineNumber(ED->getLocation());
     StringRef EDName = ED->getName();
     llvm::DIType *RetTy = DBuilder.createReplaceableCompositeType(
         llvm::dwarf::DW_TAG_enumeration_type, EDName, EDContext, DefUnit, Line,
Enumerators.push_back(
         DBuilder.createEnumerator(Enum->getName(), Enum->getInitVal()));
   }
 
   // Return a CompositeType for the enum itself.
   llvm::DINodeArray EltArray = DBuilder.getOrCreateArray(Enumerators);
 
   llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation());
   unsigned Line = getLineNumber(ED->getLocation());
-  llvm::DIScope *EnumContext = getDeclContextDescriptor(ED);
+  llvm::DIScope *EnumContext = getDeclarationLexicalScope(ED);
   llvm::DIType *ClassTy = getOrCreateType(ED->getIntegerType(), DefUnit);
   return DBuilder.createEnumerationType(EnumContext, ED->getName(), DefUnit,
                                         Line, Size, Align, EltArray, ClassTy,
                                         Identifier, ED->isScoped());
 }
 
 llvm::DIMacro *CGDebugInfo::CreateMacro(llvm::DIMacroFile *Parent,
                                         unsigned MType, SourceLocation LineLoc,
llvm::DICompositeType *CGDebugInfo::CreateLimitedType(const RecordType *Ty) {
   const SourceLocation Loc = RD->getLocation();
   llvm::DIFile *DefUnit = nullptr;
   unsigned Line = 0;
   if (Loc.isValid()) {
     DefUnit = getOrCreateFile(Loc);
     Line = getLineNumber(Loc);
   }
 
-  llvm::DIScope *RDContext = getDeclContextDescriptor(RD);
+  llvm::DIScope *RDContext = getDeclarationLexicalScope(RD);
 
   // If we ended up creating the type during the context chain construction,
   // just return that.
   auto *T = cast_or_null<llvm::DICompositeType>(
       getTypeOrNull(CGM.getContext().getRecordType(RD)));
   if (T && (!T->isForwardDecl() || !RD->getDefinition()))
     return T;
 
void CGDebugInfo::collectVarDeclProps(const VarDecl *VD, llvm::DIFile *&Unit,
 
   if (isa<VarTemplateSpecializationDecl>(VD)) {
     llvm::DINodeArray parameterNodes = CollectVarTemplateParams(VD, &*Unit);
     TemplateParameters = parameterNodes.get();
   } else {
     TemplateParameters = nullptr;
   }
 
+  // Get context for static locals (that are technically globals) the same way
+  // we do for "local" locals -- by using current lexical block.
+  if (VD->isStaticLocal()) {
+    assert(!LexicalBlockStack.empty() && "Region stack mismatch, stack empty!");
+    VDContext = LexicalBlockStack.back();
+    return;
+  }
+
   // Since we emit declarations (DW_AT_members) for static members, place the
   // definition of those static members in the namespace they were declared in
   // in the source code (the lexical decl context).
   // FIXME: Generalize this for even non-member global variables where the
   // declaration and definition may have different lexical decl contexts, once
   // we have support for emitting declarations of (non-member) global variables.
   const DeclContext *DC = VD->isStaticDataMember() ? VD->getLexicalDeclContext()
                                                    : VD->getDeclContext();

clang/lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitDecl(const Decl &D) {
   case Decl::UsingShadow:
   case Decl::ConstructorUsingShadow:
   case Decl::ObjCTypeParam:
   case Decl::Binding:
   case Decl::UnresolvedUsingIfExists:
     llvm_unreachable("Declaration should not be in declstmts!");
   case Decl::Record:    // struct/union/class X;
   case Decl::CXXRecord: // struct/union/class X; [C++]
-    if (CGDebugInfo *DI = getDebugInfo())
+    if (CGDebugInfo *DI = getDebugInfo()) {
+      DI->recordDeclarationLexicalScope(D);
       if (cast<RecordDecl>(D).getDefinition())
         DI->EmitAndRetainType(getContext().getRecordType(cast<RecordDecl>(&D)));
+    }
     return;
   case Decl::Enum:      // enum X;
-    if (CGDebugInfo *DI = getDebugInfo())
+    if (CGDebugInfo *DI = getDebugInfo()) {
+      DI->recordDeclarationLexicalScope(D);
       if (cast<EnumDecl>(D).getDefinition())
         DI->EmitAndRetainType(getContext().getEnumType(cast<EnumDecl>(&D)));
+    }
     return;
-  case Decl::Function:     // void X();
   case Decl::EnumConstant: // enum ? { X = ? }
+  case Decl::Function:     // void X();
   case Decl::StaticAssert: // static_assert(X, ""); [C++0x]
   case Decl::Label:        // __label__ x;
   case Decl::Import:
   case Decl::MSGuid:    // __declspec(uuid("..."))
   case Decl::TemplateParamObject:
   case Decl::OMPThreadPrivate:
   case Decl::OMPAllocate:
   case Decl::OMPCapturedExpr:
   case Decl::OMPRequires:
   case Decl::Empty:
   case Decl::Concept:
   case Decl::LifetimeExtendedTemporary:
   case Decl::RequiresExprBody:
     // None of these decls require codegen support.
     return;
 
   case Decl::NamespaceAlias:
     if (CGDebugInfo *DI = getDebugInfo())
-        DI->EmitNamespaceAlias(cast<NamespaceAliasDecl>(D));
     return;
   case Decl::Using:          // using X; [C++]
     if (CGDebugInfo *DI = getDebugInfo())
-        DI->EmitUsingDecl(cast<UsingDecl>(D));
     return;
   case Decl::UsingEnum: // using enum X; [C++]
     if (CGDebugInfo *DI = getDebugInfo())
       DI->EmitUsingEnumDecl(cast<UsingEnumDecl>(D));
     return;
   case Decl::UsingPack:
     for (auto *Using : cast<UsingPackDecl>(D).expansions())
       EmitDecl(*Using);
case Decl::OMPDeclareReduction:
     return CGM.EmitOMPDeclareReduction(cast<OMPDeclareReductionDecl>(&D), this);
 
   case Decl::OMPDeclareMapper:
     return CGM.EmitOMPDeclareMapper(cast<OMPDeclareMapperDecl>(&D), this);
 
   case Decl::Typedef:      // typedef int X;
   case Decl::TypeAlias: {  // using X = int; [C++0x]
     QualType Ty = cast<TypedefNameDecl>(D).getUnderlyingType();
-    if (CGDebugInfo *DI = getDebugInfo())
+    if (CGDebugInfo *DI = getDebugInfo()) {
+      DI->recordDeclarationLexicalScope(D);
       DI->EmitAndRetainType(Ty);
+    }
     if (Ty->isVariablyModifiedType())
       EmitVariablyModifiedType(Ty);
     return;
   }
   }
 }
 
 /// EmitVarDecl - This method handles emission of any variable declaration

clang/test/CodeGenCXX/debug-info-lexcial-block.cpp

+// RUN: %clang_cc1 -triple x86_64-none-linux-gnu -emit-llvm -debug-info-kind=limited %s -o - | FileCheck %s
+
+void foo() {
+  static int bar = 1;
+  {
+    struct X {};
+    typedef char Y;
+    static int bar = 0;
+    // The following basic block is intended, in order to check the case where
+    // types "X", "Y" are defined in a different scope than where they are used.
+    // They should have the scope they are defined at as their parent scope.
+    {
+      X a;
+      Y b;
+    }
+  }
+}
+
+// CHECK: !{{[0-9]+}} = distinct !DIGlobalVariable(name: "bar", scope: [[FSCOPE:![0-9]+]]
+// CHECK: [[FSCOPE]] = distinct !DISubprogram(name: "foo"
+// CHECK: !{{[0-9]+}} = distinct !DIGlobalVariable(name: "bar", scope: [[LBSCOPE:![0-9]+]]
+// CHECK: [[LBSCOPE]] = distinct !DILexicalBlock(scope: [[FSCOPE]]
+// CHECK: !{{[0-9]+}} = !DILocalVariable(name: "a", scope: [[LBSCOPE2:![0-9]+]], {{.*}} type: [[STRUCT:![0-9]+]]
+// CHECK: [[LBSCOPE2]] = distinct !DILexicalBlock(scope: [[LBSCOPE]]
+// CHECK: [[STRUCT]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "X", scope: [[LBSCOPE]]
+// CHECK: !{{[0-9]+}} = !DILocalVariable(name: "b", scope: [[LBSCOPE2]], {{.*}} type: [[TYPEDEF:![0-9]+]]
+// CHECK: [[TYPEDEF]] = !DIDerivedType(tag: DW_TAG_typedef, name: "Y", scope: [[LBSCOPE]]