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