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

David Blaikie via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 29 13:21:03 PST 2016

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

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

Comment at: lib/CodeGen/CGDebugInfo.cpp:1479
@@ +1478,3 @@
+void CGDebugInfo::recordDeclarationLexicalScope(const Decl &D) {
+  assert(LexicalBlockMap.find(&D) == LexicalBlockMap.end() &&
+         "D is already mapped to lexical block scope");
aaboud wrote:
> dblaikie wrote:
> > Dose this assert not fire if there are two decls in the same lexical scope?
> 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?
Right, I think I'm following now. I had some other ideas in my head that were wrong.

Comment at: lib/CodeGen/CGDebugInfo.cpp:1481
@@ +1480,3 @@
+         "D is already mapped to lexical block scope");
+  if (!LexicalBlockStack.empty())
+    LexicalBlockMap[&D] = LexicalBlockStack.back();
aaboud wrote:
> dblaikie wrote:
> > Should we assert that LexicalBlockStack isn't empty?
> It might be empty, if the declaration is in the program scope, right?
> So, we do not want to assert, just to check.
Yep, it's starting to make more sense to me now... 

Comment at: lib/CodeGen/CGDebugInfo.cpp:1489
@@ +1488,3 @@
+  if (I != LexicalBlockMap.end()) {
+    RetainedTypes.push_back(Ty.getAsOpaquePtr());
+    return I->second;
aaboud wrote:
> dblaikie wrote:
> > Why does the type need to be added to the retained types list?
> 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)
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.

Comment at: lib/CodeGen/CGDebugInfo.cpp:2066
@@ -2045,3 +2065,3 @@
   if (isImportedFromModule || !ED->getDefinition()) {
-    llvm::DIScope *EDContext = getDeclContextDescriptor(ED);
+    llvm::DIScope *EDContext = getDeclarationLexicalScope(*ED, QualType(Ty, 0));
     llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation());
aaboud wrote:
> dblaikie wrote:
> > 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?
> 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?
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.

Comment at: test/CodeGenCXX/debug-info-lb.cpp:7
@@ +6,3 @@
+  {
+    class X {
+    public:
Could just make this a struct.

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

Comment at: test/CodeGenCXX/debug-info-lb.cpp:14
@@ +13,3 @@
+    static int bar = 0;
+    {
+      X a(x);
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)


More information about the cfe-commits mailing list