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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
