Page MenuHomePhabricator

Handle lookup of names identifying both a variable and a type
Needs ReviewPublic

Authored by uweigand on Apr 11 2016, 11:26 AM.

Details

Reviewers
spyffe
jingham
Summary

In C++ code, a variable can have the same name as a type, e.g. like

int C;
struct C {

static int a;

};

When evaluating an expression like "C::a" by the LLDB parser, clang
will call back into the external source asking for decls for the
identifier "C". Currently, LLDB will only return the decl for the
variable C. This in turn will cause clang to fail with an error.

(This happens for me with the lang/cpp/scope/TestCppScope.py test
case, due to a static variable C in one of the libm.so objects.)

Instead, it seems clang expects the external data source to return
*both* the variable and the type decl. It will then choose the
appropriate one to use based on its current parsing context.

This patch changes ClangExpressionDeclMap::FindExternalVisibleDecls
to always call ClangASTSource::FindExternalVisibleDecls to possibly
identify types, even if we already found a variable of that name.

Diff Detail

Event Timeline

uweigand updated this revision to Diff 53291.Apr 11 2016, 11:26 AM
uweigand retitled this revision from to Handle lookup of names identifying both a variable and a type.
uweigand updated this object.
uweigand added a reviewer: spyffe.
uweigand added a subscriber: lldb-commits.
labath added a subscriber: labath.Apr 13 2016, 5:19 AM

I think having a self-contained test for this issue (one which does not rely on internals of some system library) would be extremely valuable. Could you add one?

I think having a self-contained test for this issue (one which does not rely on internals of some system library) would be extremely valuable. Could you add one?

Well, a simple way would be to just add something like

int A, B, C;

to the lang/cpp/scope/main.cpp test case. Would that be OK or do you prefer a completely separate test?

I think adding it to the extending the existing test is fine, given that it's a fairly small one. (In case of large tests, it's better to make a new one, so it can be debugged/xfailed independently). Maybe just add int C so that we test both the situation when we have a variable, and when we don't?

uweigand updated this revision to Diff 53553.Apr 13 2016, 6:51 AM

Add test case.

uweigand updated this revision to Diff 57617.May 18 2016, 7:17 AM
uweigand added a reviewer: jingham.

Ping? This fixes the last remaining test suite failure on s390x. I'd like to get a clean test suite ...

Patch updated to current sources.

Added jingham since you touched that code recently.