[Lldb-commits] [PATCH] D16746: Use an artifical namespace so that member vars do not hide local vars.

[Sean Callanan via lldb-commits]

spyffe requested changes to this revision.
spyffe added a comment.
This revision now requires changes to proceed.

Siva, I've inlined my comments.  I believe there's a misunderstanding about what role //this// has in the patched version of GetUniqueNamespaceDeclaration.  In fact, as I argue in my inline comment, //this// is unused.  That's confusing code.  You can achieve the same effect by leaving the code as is and just passing the right thing in for //this//.


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1381
@@ +1380,3 @@
+                    clang::NamespaceDecl *namespace_decl = ast->GetUniqueNamespaceDeclaration(
+                        name_unique_cstr, nullptr, m_ast_context);
+                    if (namespace_decl)
----------------
Actually from my reading of the code (and from the fact that this is working for you at all) your patch is not using the frame's AST context at all.  There would be a big problem if you did, namely:
  - Your patch sets //frame_decl_context// by getting the CompilerDeclContext from the block.  This comes from the DWARF parser.
  - It uses GetTypeSystem() to extract the ClangASTContext* from that CompilerDeclContext.  This is the DWARF parser's AST context.
  - It then passes that to GetUniqueNamespaceDeclaration as //this//.  **However, GetUniqueNamespaceDeclaration, as your patch modifies it, never actually uses //this//.  It uses //ast// (by which I mean the //ast// argument your patch adds to GetUniqueNamespaceDeclaration and not the //ast// variable in the portion of the patch above this comment), which is just ClangExpressionDeclMap::m_ast_context.**
  - GetUniqueNamespaceDeclaration looks up a Decl matching name_unique_cstr in //ast//'s main translation unit, since the code passed in nullptr for //decl_ctx//.  Let's assume it doesn't find it.  (If it does, then it finds something we created this way.)
  - Then GetUniqueNamespaceDeclaration calls NamespaceDecl::Create, passing in //ast// and //ast//'s main translation unit.  This creates a NamespaceDecl from //ast//.  **If it used //this//, then the NamespaceDecl would be from the DWARF parser's AST context, living in the DWARF parser's main translation unit.**
  - GetUniqueNamespaceDeclaration returns that declaration.
  - Your patch then calls AddNamedDecl() on this namespace declaration.

Suppose the NamespaceDecl actually came from the DWARF parser's AST context, as you suggest.  This would be a bug for two reasons.

First, AddNamedDecl doesn't do any importing.  It feeds the NamespaceDecl straight to the compiler.  The compiler only expects Decls from its own ASTContext, but your patch would have fed it a Decl from the DWARF parser's ASTContext.  Clang would very likely assert and crash left and right if this were actually what your patch was doing.

Second, your patch would have added a namespace declaration to the DWARF parser's AST context.  This is not correct, because the DWARF parser should only contain entities extracted from DWARF.  Namespaces like //$__lldb_local_vars// do not belong there.

Thankfully, as I said, GetUniqueNamespaceDeclaration() as your patch modifies it actually ignores //this// and uses the passed-in //ast// parameter.  If you just leave GetUniqueNamespaceDeclaration() as it is, setting //ast// to this->GetASTContext(), and call m_ast_context.GetUniqueNamespaceDeclaration() in the code above, you achieve the same thing you've done here, without the confusion of the extra argument.

The only remaining issue is the one I mentioned in my previous comment: m_ast_context is a clang::ASTContext and not a ClangASTContext, so you don't get access to GetUniqueNamespaceDeclaration.  That's why I propose making the static method as detailed above, so you can call that.


http://reviews.llvm.org/D16746