Page MenuHomePhabricator

Use an artifical namespace so that member vars do not hide local vars.
ClosedPublic

Authored by sivachandra on Jan 29 2016, 6:53 PM.

Details

Summary

While evaluating expressions when stopped in a class method, there was a
problem of member variables hiding local variables. This was happening
because, in the context of a method, clang already knew about member
variables with their name and assumed that they were the only variables
with those names in scope. Consequently, clang never checks with LLDB
about the possibility of local variables with the same name and goes
wrong. This change addresses the problem by using an artificial
namespace "$lldb_local_vars". All local variables in scope are
declared in the "$
lldb_expr" method as follows:

using $__lldb_local_vars::<local var 1>;
using $__lldb_local_vars::<local var 2>;
...

This hides the member variables with the same name and forces clang to
enquire about the variables which it thinks are declared in
$lldb_local_vars. When LLDB notices that clang is enquiring about
variables in $
lldb_local_vars, it looks up local vars and conveys
their information if found. This way, member variables do not hide local
variables, leading to correct evaluation of expressions.

A point to keep in mind is that the above solution does not solve the
problem for one specific case:

namespace N
{
    int a;
}

class A
{
public:
    void Method();
    int a;
};

void
A::Method()
{
    using N::a;
    ...

    // Since the above solution only touches locals, it does not
    // force clang to enquire about "a" coming from namespace N.
}

Diff Detail

Event Timeline

sivachandra retitled this revision from to Use an artifical namespace so that member vars do not hide local vars..
sivachandra updated this object.
sivachandra added reviewers: clayborg, spyffe.
sivachandra added a subscriber: lldb-commits.

I have tested this with examples and the test run is clean. I will add tests if you think the overall approach is acceptable.

sivachandra added inline comments.Jan 29 2016, 6:55 PM
source/Target/StackFrame.cpp
600 ↗(On Diff #46460)

This should not be part of this change. I have another patch for that: http://reviews.llvm.org/D16745

clayborg accepted this revision.Feb 1 2016, 10:08 AM
clayborg edited edge metadata.

Looks good to me, but Sean should be the one to OK this for real.

This revision is now accepted and ready to land.Feb 1 2016, 10:08 AM
spyffe requested changes to this revision.Feb 1 2016, 10:24 AM
spyffe edited edge metadata.

Siva, this is a clever and self-contained solution to a problem that's annoyed us a great deal for a while now. I have a few minor quibbles about implementation but overall this is a great fix! Thank you!

source/Expression/ExpressionSourceCode.cpp
276

I think we should do this in all C++ cases. The reason is because it'll make sure the method gets more testing and if anything fails we'll catch it in the common case instead of seeing it only in specific "edge" cases. Could we just check if the frame is C++?

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
1381

This bit where we're passing the AST context as an argument as well as the this pointer is a bit awkward. The common pattern when we have a class (like lldb::ClangASTContext) that provides useful functionality on top of another class (clang::ASTContext, in this case) is to add a method:

static void GetUniqueNamespaceDeclaration(clang::ASTContext *, …)

and then have the instance method call through to it transparently. Unless there's some reason why the frame's AST context is important, let's just do that here too.

This revision now requires changes to proceed.Feb 1 2016, 10:24 AM
sivachandra updated this revision to Diff 46717.Feb 2 2016, 4:04 PM
sivachandra edited edge metadata.
  1. Added tests.
  2. Extended the scope of this feature in presense of "using namespace <name>;"

decls.

  1. Addressed one of the two comments by Sean. Will respond to the other on the

review page.

sivachandra updated this object.Feb 2 2016, 4:08 PM
sivachandra edited edge metadata.
sivachandra marked an inline comment as done.Feb 2 2016, 4:16 PM

Thanks, Greg and Sean for the review. The new version of the patch has tests also added. I also have an inline response to a comment by Sean.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
1381

I did not understand this comment fully. However, the frame's AST is important here because we want the new namespace decl to have the same clang::ExternalASTSource as that of the current frame. The ClangASTContext object |ast| above does not come with any AST set, and hence GetUniqueNamespaceDeclaration makes one up with external AST source set to ClangExternalASTSourceCallbacks instead of clang_internal::ClangASTSource. To avoid this, I added the new argument which specifies the AST to be used.

A note to Greg: Though you have stamped green on the first version, I think you should take a look at the new version as I have changes to the API in TypeSystem (and consequently, in ClangASTContext).

spyffe requested changes to this revision.Feb 2 2016, 6:44 PM
spyffe edited edge metadata.

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.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
1381

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.

This revision now requires changes to proceed.Feb 2 2016, 6:44 PM

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.

This comment clears it up for me. I now follow and agree with the rest of your inline comments. Will send the updated patch as soon as I can get to this.

Thanks,
Siva Chandra

sivachandra updated this revision to Diff 46852.Feb 3 2016, 4:58 PM
sivachandra edited edge metadata.

Address Sean's comment.

sivachandra added inline comments.Feb 3 2016, 4:59 PM
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
1381

Done. I hope I understood you right.

spyffe accepted this revision.Feb 3 2016, 6:26 PM
spyffe edited edge metadata.

I suggested a name change; just fix that and I think we're good to go.

source/Symbol/ClangASTContext.cpp
1893

We might decide later that it's not worth the work of creating an entire ClangASTContext (which has a bunch of auto_ptrs and stuff that would get initialized to no purpose) to do this work, but on the other hand we do all kinds of work doing type importing so this is likely to get lost in the noise.

If we did decide to do something about this, we'd flip it around so that the static function is the one that does the work and the non-static version calls through to the static one.

9797

The ignore_imported_decls parameter would probably be better off being called ignore_using_decls. That makes it more clear what's happening.

This revision is now accepted and ready to land.Feb 3 2016, 6:26 PM
sivachandra updated this revision to Diff 46861.Feb 3 2016, 6:47 PM
sivachandra edited edge metadata.

Address the arg name change comment.

Thanks a lot for the review. I have changed the arg name as suggested. I will put this patch in tomorrow morning so that I can monitor the bots afterwards.

sivachandra closed this revision.Feb 4 2016, 10:42 AM