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

[Amjad Aboud via cfe-commits]

aaboud marked an inline comment as done.
aaboud added a comment.

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.


================
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");
----------------
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?

================
Comment at: lib/CodeGen/CGDebugInfo.cpp:1481
@@ +1480,3 @@
+         "D is already mapped to lexical block scope");
+  if (!LexicalBlockStack.empty())
+    LexicalBlockMap[&D] = LexicalBlockStack.back();
----------------
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.

================
Comment at: lib/CodeGen/CGDebugInfo.cpp:1488
@@ +1487,3 @@
+  auto I = LexicalBlockMap.find(&D);
+  if (I != LexicalBlockMap.end()) {
+    RetainedTypes.push_back(Ty.getAsOpaquePtr());
----------------
dblaikie wrote:
> Perhaps we should assert here that if D's scope is a lexical block, that it should have a corresponding entry in the LexicalBlockMap?
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.

================
Comment at: lib/CodeGen/CGDebugInfo.cpp:1489
@@ +1488,3 @@
+  if (I != LexicalBlockMap.end()) {
+    RetainedTypes.push_back(Ty.getAsOpaquePtr());
+    return I->second;
----------------
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)

================
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());
----------------
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?


http://reviews.llvm.org/D15977