Page MenuHomePhabricator

Fix for ambiguous lookup in expressions between local variable and namespace

Authored by shafik on Mar 28 2019, 2:05 PM.

Diff Detail


Event Timeline

shafik created this revision.Mar 28 2019, 2:05 PM
friss added a subscriber: friss.Mar 28 2019, 2:41 PM
friss added inline comments.
171 ↗(On Diff #192712)

This seems dangerous. Now people having a variable called 'self' or '_cmd' in their C++ code will not be able to evaluate them anymore. This filtering needs to be per-language.

257 ↗(On Diff #192712)

How does this work? The locals seem to be added only in C++, I'm not sure how this patch makes any difference?

334 ↗(On Diff #192712)

Why only in the static_method case?

shafik updated this revision to Diff 194377.Apr 9 2019, 12:12 PM

Addressing comments:

  • Now applies to all languages not just C++
  • When adding locals be more selective on filtering i.e. only filter self and _cmd for Objective C etc...
shafik marked 3 inline comments as done.Apr 9 2019, 12:12 PM

@friss I believe I have addressed your comments

shafik marked an inline comment as done.Apr 9 2019, 12:13 PM
shafik added inline comments.
172 ↗(On Diff #194377)

@jingham I would appreciate if you could confirm this looks like the right approach

friss added inline comments.Apr 9 2019, 12:36 PM
181 ↗(On Diff #194377)

This should also trigger for eLanguageTypeObjC_plus_plus

356 ↗(On Diff #194377)

Remove that comment, also I don't think the test case covers this codepath?

shafik updated this revision to Diff 194600.Apr 10 2019, 3:37 PM

-Adjusting tests to ensure coverage of Objecive-C static and non-static methods and C and C++

@friss I had to rework the tests a little but they now cover Objective-C static and non-static methods as well as C and C++.

friss added a comment.Apr 10 2019, 5:04 PM

You didn't address my comment that "this" needs to treated specially in Obj-C++ too. Other than that this LGTM

teemperor requested changes to this revision.Apr 11 2019, 7:12 AM
teemperor added inline comments.
11 ↗(On Diff #194600)

I believe gmodules is unnecessary (or I'm missing something) :)

7 ↗(On Diff #194600)

Can you clang-format this file (and all the other cpp files too just to be safe)?

172 ↗(On Diff #194600)

langauge -> language

256 ↗(On Diff #194600)

I believe this check was originally there because injecting local variables into the expression is quite costly (as we will have to preload all the associated AST nodes even if they are unused).

It seems this patch now completely removes this check, even though we don't always need to inject variables for this fix (e.g. for the C wrapping language, which is for functions in C and I believe also non-member functions in C++). I think the AddLocalVariableDecls should do the checking and do nothing unless we are in C++, Obj-C or Obj-C++. Otherwise this patch could degrade performance in these other cases as an unintended side effect.

339 ↗(On Diff #194600)

I think the end of this string should be aligned with the others.

353 ↗(On Diff #194600)

Again, end of the string should be aligned.

This revision now requires changes to proceed.Apr 11 2019, 7:12 AM
clayborg requested changes to this revision.Apr 11 2019, 11:21 AM
clayborg added a subscriber: clayborg.

I would really like to see a better solution than just adding all var decls to an expression as this is already costly for C++ and we shouldn't propagate this. I know it used to be really really expensive. Not sure if it still is. But it would be worth measuring by running expressions in C++ and adding and removing this feature to see how much it costs us when we have many variables all with unique and complex types.

267 ↗(On Diff #194600)

Adding all locals can be very expensive. Image if you have a function with hundreds of variables, this will cause any expression to parse all local variables and complete their types. We used to do this only for C++ cause of how expressions work: we add ourselves as a precompiled header and clang only asks us about things it doesn't know about. So if you have a class:

class foo {
  int x;
  void callme();

Then you are stopped inside callme:

void foo::callme() {
  int x = 12; // local copy of x that will supersede the this->x

If you are stopped inside callme and we evaluate the expression, clang will never ask us about "x" because we have sandboxed our expression as a function that is in a method of the class foo called "void foo::$__lldb_expr()" and clang will find this->x first because it knows the definition of the class.

For all other languages languages we should be able to just be asked about "x" in the precompiled header code and we should find it.

So the right fix might be something a bit more tame and would work for all languages. Two solutions we have talked about before:
1- grab all identifiers from the expression source code, and only add the ones that are in var_list_sp. This would limit the amount of variables we add, and limit the number of types we need to copy into the expression AST. Remember that each expression has its own AST context. We copy types into the expression AST context as needed, one for each local variable we have, unless its type has already been copied. So just adding all variables is quite expensive already for C++ and I would like to see that reduced with a better solution.
2 - add a special lookup mechanism into clang for debugger mode only, where it might ask for any extra var decls through the precompiled header mechanism even when/if it has a class member variable.

shafik marked 2 inline comments as done.Apr 11 2019, 2:54 PM
shafik added inline comments.
11 ↗(On Diff #194600)

This only reproduces in modules build. The inconsistency is a separate issue.

256 ↗(On Diff #194600)

This is a good point. I am going to be working on getting this fix in place which should address this issue

Just thought of 1 additional way to allow us to pull in fewer var declarations: get a list of all of the member variable names in the current class when stopped in a class method and only add ones that match local variables. If we are in a static member variable then skip of course. Comments? Thoughts?

Jim Ingham said:

This won't be complete, since we also get collisions between local variables and namespaces, and that wouldn't be detected by your heuristic.

I think the trick that Fred used in is actually pretty complete. An expression is never going to need to look up a variable if

strstr(expressionText, varname) == NULL

And OTOH if this is not NULL, there's a very good chance we will need to look it up. So this is a pretty optimal filter. We should clean this up (I think it caused some test failures with the new variable completion in expr, IIRC). If we need to do more after that is in place we can, but I bet this will end up with us for the most part only injecting local variables where they are needed.


I am fine if the strstr method is already in place, then my objection is gone.

shafik added a comment.May 2 2019, 3:09 PM

@teemperor @jingham @clayborg I believe now that is landed the performance concerns should be addressed.

This revision was not accepted when it landed; it landed in state Needs Review.May 3 2019, 12:58 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 12:58 PM