[Clang] Supporting all entities declared in lexical scope in LLVM debug info

[aaboud]

Summary

This is the full implementation in Clang of the proposal:
http://lists.llvm.org/pipermail/llvm-dev/2015-December/093313.html

[probinson]

I don't feel comfortable enough with Clang to review the code part.
The tests are generally good (one inline comment) but it seems like they could be done all in one test file, which is more efficient in terms of test run time.

test/CodeGenCXX/debug-info-lb-static2.cpp

13 ↗

(On Diff #44273)

I think the second check wants to end with scope: [[FuncScope]], (as written I think it's just redefining FuncScope).

[aaboud]

Merged all 4 Lit tests into one.

[dblaikie]

lib/CodeGen/CGDebugInfo.cpp

1479 ↗

(On Diff #45400)

Dose this assert not fire if there are two decls in the same lexical scope?

1481 ↗

(On Diff #45400)

Should we assert that LexicalBlockStack isn't empty?

1488 ↗

(On Diff #45400)

Perhaps we should assert here that if D's scope is a lexical block, that it should have a corresponding entry in the LexicalBlockMap?

1489 ↗

(On Diff #45400)

Why does the type need to be added to the retained types list?

1491 ↗

(On Diff #45400)

else-after-return (just drop the else and let the following code remain unconditionally)

2066 ↗

(On Diff #45400)

Would it be reasonable/possible to do these changes one at a time & demonstrate the different situations in which types were not correctly nested in lexical scopes? Or are they all necessary for some correctness/invariant that must hold?

[aaboud]

Thanks David for the feedback.
I have some answers below, please let me know if they are not sufficient or you are still have concerns.

lib/CodeGen/CGDebugInfo.cpp

1479 ↗

(On Diff #45400)

Why two decls would have the same object instance "Decl"?
I am not sure even that you can declare two decls with same name at same lexical-scope, but even if they are defined in the same lexical scope, they will have different line numbers, right?

1481 ↗

(On Diff #45400)

It might be empty, if the declaration is in the program scope, right?
So, we do not want to assert, just to check.

1488 ↗

(On Diff #45400)

We will get to this function also for D's that are not in lexical scope.
We should not assert, that is also why we have the else handling (which is how we used to get context before).

I can agree with you that the function name is misleading, and I am open to other suggestions.

1489 ↗

(On Diff #45400)

Excellent question :)
This was your suggestion few months ago.
The reason is that backend will need to know what types are declared in lexical blocks.
Imported entities are collected in the import entity list, while types are collected in the retained types.

Check line 518 at DwarfDebug.cpp (http://reviews.llvm.org/D15976#51cfb106)

2066 ↗

(On Diff #45400)

Clang changes cannot be committed before the LLVM changes, otherwise we would crash in backend.
Regarding splitting Clang changes into several comments it is possible with the following splits:

1. Static variable (lines 2508-2517)
2. Types (all the others)

Now types can be split into several comments as well: Records and Typedef
But the infrastructure of recording lexical-scope declarations and getting the lexical-scope context should be committed with the first patch (nevertheless what one it would be, Records or Typedef).

Regarding the test, I used to have 3 (even 4) separate tests, but I was asked to merged them.
Now, if we do commit patches in steps, I suggest that I still end up with one final test, but I will need to modify it with each patch (I will be building it step by step).

Now, what is your call? do you want me to commit the this patch in 3 steps? Or just go with it as one piece?

[dblaikie]

Looks good to me. Test case could be simplified a bit further, but feel free to commit after that.

lib/CodeGen/CGDebugInfo.cpp

1479 ↗

(On Diff #45400)

Right, I think I'm following now. I had some other ideas in my head that were wrong.

1481 ↗

(On Diff #45400)

Yep, it's starting to make more sense to me now...

1489 ↗

(On Diff #45400)

Not quite following - could we discover the types lazily as we're generating DWARF in the backend - if we happen to emit a variable of that type & generate the DIE* for the type, etc? (so that if a type becomes unused it's able to be dropped - rather than being preserved only when the type is local)

I suppose that would make it difficult to choose which scopes we had to preserve when emitting debug info in the backend (if we didn't see a type until after the scope was created - we wouldn't've known whether to emit/create that scope or collapse it into its parent, etc) - so this probably isn't possible/practical.

2066 ↗

(On Diff #45400)

Right, incremental patches that update the test case (test case could be committed as-is with CHECK-NOTs in places where the expected output didn't match the desired result, or with them removed & the types that are incorrect omitted until the functionality was added)

But I think I'm understanding this mostly well enough for it go in as-is/in one go.

test/CodeGenCXX/debug-info-lb.cpp

7 ↗

(On Diff #45400)

Could just make this a struct.

It probably doesn't need any members either. (simply "struct X { }")

14 ↗

(On Diff #45400)

This block seems unnecessary, and its contents are probably more complicated than needed. This seems to suffice:

X a;
Y b;

& the global variable is emitted without referencing it anyway.

(you could drop the int return and x parameter, too - no need to return anything from the function, and the if (x) could be removed - the scope can remain & still produce the same debug info I think)

[aaboud]

Thanks David for the comments.
See answer below.

lib/CodeGen/CGDebugInfo.cpp

1489 ↗

(On Diff #45400)

Your analysis is correct.
I just tried to re-check how we can discover types lazily, but I could not figure out how to make sure the lexical scope would be created for these types.

I guess we can revisit this issue later and re-think a better implementation that make sure optimized types will not be emitted in the debug info. However, I think we can do that in a separate (following) patch.

test/CodeGenCXX/debug-info-lb.cpp

14 ↗

(On Diff #45400)

OK, I simplify the test, and it is still does what I needed.

The only thing that I need to clarify is the extra block scope that I added to wrap the "a" and "b" variable definition.
The reason I did that is to test the case where the types "X" and "Y" are defined in a different scope than they are used at.
Without handling this case correctly, the LLVM IR could mistakenly associate "X" and "Y" to the scope of the usage and not the definition, i.e. the scope of "a" and "b", to test that I had to have different scope as in the example.
I added a comment in the test explaining that.

[aaboud]

Reduced the LIT tests and added comment explaining part of it.

[dblaikie]

Looks good - though you could do that one further simplification to the test case, I think. (removing x/if x)

test/CodeGenCXX/debug-info-lb.cpp

5 ↗

(On Diff #46708)

Looks like you could remove this "if (x)" and the "int x" parameter & still keep the test otherwise in tact.

[aaboud]

Simplified LIT tests according to David comment.

[dblaikie]

Thanks - please commit when ready

cfe/trunk/lib/CodeGen/CGDebugInfo.h

class CGDebugInfo {
 
   /// 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, QualType Ty);
+
   /// Emit standalone debug info for a type.
   llvm::DIType *getOrCreateStandaloneType(QualType Ty, SourceLocation Loc);
 
   void completeType(const EnumDecl *ED);
   void completeType(const RecordDecl *RD);
   void completeRequiredType(const RecordDecl *RD);
   void completeClassData(const RecordDecl *RD);
 

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

llvm::DIType *CGDebugInfo::CreateType(const TemplateSpecializationType *Ty,
   SourceLocation Loc = AliasDecl->getLocation();
   return DBuilder.createTypedef(Src, OS.str(), getOrCreateFile(Loc),
                                 getLineNumber(Loc),
                                 getDeclContextDescriptor(AliasDecl));
 }
 
 llvm::DIType *CGDebugInfo::CreateType(const TypedefType *Ty,
                                       llvm::DIFile *Unit) {
+  TypedefNameDecl *TD = Ty->getDecl();
   // We don't set size information, but do specify where the typedef was
   // declared.
-  SourceLocation Loc = Ty->getDecl()->getLocation();
+  SourceLocation Loc = TD->getLocation();
+
+  llvm::DIScope *TDContext = getDeclarationLexicalScope(*TD, QualType(Ty, 0));
 
   // Typedefs are derived from some other type.
   return DBuilder.createTypedef(
       getOrCreateType(Ty->getDecl()->getUnderlyingType(), Unit),
       Ty->getDecl()->getName(), getOrCreateFile(Loc), getLineNumber(Loc),
-      getDeclContextDescriptor(Ty->getDecl()));
+      TDContext);
 }
 
 llvm::DIType *CGDebugInfo::CreateType(const FunctionType *Ty,
                                       llvm::DIFile *Unit) {
   SmallVector<llvm::Metadata *, 16> EltTys;
 
   // Add the result type at least.
   EltTys.push_back(getOrCreateType(Ty->getReturnType(), Unit));
llvm::DIType *CGDebugInfo::getOrCreateStandaloneType(QualType D,
   // because they are only referenced by name in the IR.
   if (auto *CTy = dyn_cast<llvm::DICompositeType>(T))
     if (!CTy->getIdentifier().empty())
       return T;
   RetainedTypes.push_back(D.getAsOpaquePtr());
   return T;
 }
 
+void CGDebugInfo::recordDeclarationLexicalScope(const Decl &D) {
+  assert(LexicalBlockMap.find(&D) == LexicalBlockMap.end() &&
+         "D is already mapped to lexical block scope");
+  if (!LexicalBlockStack.empty())
+    LexicalBlockMap[&D] = LexicalBlockStack.back();
+}
+
+llvm::DIScope *CGDebugInfo::getDeclarationLexicalScope(const Decl &D,
+                                                       QualType Ty) {
+  auto I = LexicalBlockMap.find(&D);
+  if (I != LexicalBlockMap.end()) {
+    RetainedTypes.push_back(Ty.getAsOpaquePtr());
+    return I->second;
+  }
+  return getDeclContextDescriptor(cast<Decl>(&D));
+}
+
 void CGDebugInfo::completeType(const EnumDecl *ED) {
   if (DebugKind <= codegenoptions::DebugLineTablesOnly)
     return;
   QualType Ty = CGM.getContext().getEnumType(ED);
   void *TyPtr = Ty.getAsOpaquePtr();
   auto I = TypeCache.find(TyPtr);
   if (I == TypeCache.end() || !cast<llvm::DIType>(I->second)->isForwardDecl())
     return;
if (isImportedFromModule || !ED->getDefinition()) {
     StringRef EDName = ED->getName();
     llvm::DIType *RetTy = DBuilder.createReplaceableCompositeType(
         llvm::dwarf::DW_TAG_enumeration_type, EDName, TmpContext.get(), DefUnit,
         Line, 0, Size, Align, llvm::DINode::FlagFwdDecl, FullName);
 
     // Cache the enum type so it is available when building the declcontext
     // and replace the declcontect with the real thing.
     TypeCache[Ty].reset(RetTy);
-    TmpContext->replaceAllUsesWith(getDeclContextDescriptor(ED));
+    TmpContext->replaceAllUsesWith(
+        getDeclarationLexicalScope(*ED, QualType(Ty, 0)));
 
     ReplaceMap.emplace_back(
         std::piecewise_construct, std::make_tuple(Ty),
         std::make_tuple(static_cast<llvm::Metadata *>(RetTy)));
     return RetTy;
   }
 
   return CreateTypeDefinition(Ty);
Enumerators.push_back(DBuilder.createEnumerator(
         Enum->getName(), Enum->getInitVal().getSExtValue()));
   }
 
   // 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, QualType(Ty, 0));
   llvm::DIType *ClassTy =
       ED->isFixed() ? getOrCreateType(ED->getIntegerType(), DefUnit) : nullptr;
   return DBuilder.createEnumerationType(EnumContext, ED->getName(), DefUnit,
                                         Line, Size, Align, EltArray, ClassTy,
                                         FullName);
 }
 
 static QualType UnwrapTypeForDebugInfo(QualType T, const ASTContext &C) {

 llvm::DICompositeType *CGDebugInfo::CreateLimitedType(const RecordType *Ty) {
   RecordDecl *RD = Ty->getDecl();
 
   // Get overall information about the record type for the debug info.
   llvm::DIFile *DefUnit = getOrCreateFile(RD->getLocation());
   unsigned Line = getLineNumber(RD->getLocation());
   StringRef RDName = getClassName(RD);
 
-  llvm::DIScope *RDContext = getDeclContextDescriptor(RD);
+  llvm::DIScope *RDContext = getDeclarationLexicalScope(*RD, QualType(Ty, 0));
 
   // 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,
   // member, and the record type is marked as __declspec(dllexport), an implicit
   // definition of the member will be created in the record context.  DWARF
   // doesn't seem to have a nice way to describe this in a form that consumers
   // are likely to understand, so fake the "normal" situation of a definition
   // outside the class by putting it in the global scope.
   if (DC->isRecord())
     DC = CGM.getContext().getTranslationUnitDecl();
 
+  if (VD->isStaticLocal()) {
+    // Get context for static locals (that are technically globals) the same way
+    // we do for "local" locals -- by using current lexical block.
+    assert(!LexicalBlockStack.empty() && "Region stack mismatch, stack empty!");
+    VDContext = LexicalBlockStack.back();
+  } else {
     llvm::DIScope *Mod = getParentModuleOrNull(VD);
     VDContext = getContextDescriptor(cast<Decl>(DC), Mod ? Mod : TheCU);
   }
+}
 
 llvm::DISubprogram *
 CGDebugInfo::getFunctionForwardDeclaration(const FunctionDecl *FD) {
   llvm::DINodeArray TParamsArray;
   StringRef Name, LinkageName;
   unsigned Flags = 0;
   SourceLocation Loc = FD->getLocation();
   llvm::DIFile *Unit = getOrCreateFile(Loc);

cfe/trunk/lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitDecl(const Decl &D) {
   case Decl::Friend:
   case Decl::FriendTemplate:
   case Decl::Block:
   case Decl::Captured:
   case Decl::ClassScopeFunctionSpecialization:
   case Decl::UsingShadow:
   case Decl::ObjCTypeParam:
     llvm_unreachable("Declaration should not be in declstmts!");
-  case Decl::Function:  // void X();
-  case Decl::Record:    // struct/union/class X;
-  case Decl::Enum:      // enum X;
-  case Decl::EnumConstant: // enum ? { X = ? }
-  case Decl::CXXRecord: // struct/union/class X; [C++]
+  case Decl::Function:     // void X();
   case Decl::StaticAssert: // static_assert(X, ""); [C++0x]
   case Decl::Label:        // __label__ x;
   case Decl::Import:
   case Decl::OMPThreadPrivate:
   case Decl::OMPCapturedExpr:
   case Decl::Empty:
     // None of these decls require codegen support.
     return;
 
+  case Decl::Record:       // struct/union/class X;
+  case Decl::Enum:         // enum X;
+  case Decl::EnumConstant: // enum ? { X = ? }
+  case Decl::CXXRecord:    // struct/union/class X; [C++]
+    if (CGDebugInfo *DI = getDebugInfo())
+      DI->recordDeclarationLexicalScope(D);
+    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::UsingDirective: // using namespace X; [C++]
     if (CGDebugInfo *DI = getDebugInfo())
       DI->EmitUsingDirective(cast<UsingDirectiveDecl>(D));
     return;
   case Decl::Var: {
     const VarDecl &VD = cast<VarDecl>(D);
     assert(VD.isLocalVarDecl() &&
            "Should not see file-scope variables inside a function!");
     return EmitVarDecl(VD);
   }
 
   case Decl::Typedef:      // typedef int X;
   case Decl::TypeAlias: {  // using X = int; [C++0x]
     const TypedefNameDecl &TD = cast<TypedefNameDecl>(D);
     QualType Ty = TD.getUnderlyingType();
 
+    if (CGDebugInfo *DI = getDebugInfo())
+      DI->recordDeclarationLexicalScope(D);
+
     if (Ty->isVariablyModifiedType())
       EmitVariablyModifiedType(Ty);
   }
   }
 }
 
 /// EmitVarDecl - This method handles emission of any variable declaration
 /// inside a function, including static vars etc.

cfe/trunk/test/CodeGenCXX/debug-info-anon-union-vars.cpp

unsigned char *buildBytes(const T v) {
   value = v;
   return result;
 }
 
 void instantiate(int x) {
   buildBytes(x);
 }
 
+// CHECK: ![[UNION:[0-9]+]] = !DICompositeType(tag: DW_TAG_union_type,
+// CHECK-NOT: name:
+// CHECK: elements
 // CHECK: [[FILE:.*]] = !DIFile(filename: "{{.*}}debug-info-anon-union-vars.cpp",
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i", scope: ![[UNION]],
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "c", scope: ![[UNION]],
 // CHECK: !DIGlobalVariable(name: "c",{{.*}} file: [[FILE]], line: 6,{{.*}} isLocal: true, isDefinition: true
 // CHECK: !DIGlobalVariable(name: "d",{{.*}} file: [[FILE]], line: 6,{{.*}} isLocal: true, isDefinition: true
 // CHECK: !DIGlobalVariable(name: "a",{{.*}} file: [[FILE]], line: 6,{{.*}} isLocal: true, isDefinition: true
 // CHECK: !DIGlobalVariable(name: "b",{{.*}} file: [[FILE]], line: 6,{{.*}} isLocal: true, isDefinition: true
 // CHECK: !DIGlobalVariable(name: "result", {{.*}} isLocal: false, isDefinition: true
 // CHECK: !DIGlobalVariable(name: "value", {{.*}} isLocal: false, isDefinition: true
 // CHECK: !DILocalVariable(name: "i", {{.*}}, flags: DIFlagArtificial
 // CHECK: !DILocalVariable(name: "c", {{.*}}, flags: DIFlagArtificial
 // CHECK: !DILocalVariable(
 // CHECK-NOT: name:
-// CHECK: type: ![[UNION:[0-9]+]]
-// CHECK: ![[UNION]] = !DICompositeType(tag: DW_TAG_union_type,
-// CHECK-NOT: name:
-// CHECK: elements
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i", scope: ![[UNION]],
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "c", scope: ![[UNION]],
+// CHECK: type: ![[UNION]]

cfe/trunk/test/CodeGenCXX/debug-info-lb.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]+}} = !DICompositeType(tag: DW_TAG_structure_type, name: "X", scope: [[LBScope:![0-9]+]],
+// CHECK: [[LBScope]] = distinct !DILexicalBlock(scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: 5)
+
+// CHECK: [[FuncScope:![0-9]+]] = distinct !DISubprogram(name: "foo",
+
+// CHECK: !{{[0-9]+}} = !DIDerivedType(tag: DW_TAG_typedef, name: "Y", scope: [[LBScope]],
+// CHECK: !{{[0-9]+}} = !DIGlobalVariable(name: "bar", scope: [[FuncScope]], file: !{{[0-9]+}}, line: 4
+// CHECK: !{{[0-9]+}} = !DIGlobalVariable(name: "bar", scope: [[LBScope]], file: !{{[0-9]+}}, line: 8
+