[Lldb-commits] [PATCH] D59960: Fix for ambiguous lookup in expressions between local variable and namespace

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.

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.



================
Comment at: source/Expression/ExpressionSourceCode.cpp:267
 
-    if (add_locals) {
-      if (Language::LanguageIsCPlusPlus(frame->GetLanguage())) {
-        if (target->GetInjectLocalVariables(&exe_ctx)) {
-          lldb::VariableListSP var_list_sp =
-              frame->GetInScopeVariableList(false, true);
-          AddLocalVariableDecls(var_list_sp, lldb_local_var_decls);
-        }
+    if (add_locals)
+      if (target->GetInjectLocalVariables(&exe_ctx)) {
----------------
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.
This
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.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59960/new/

https://reviews.llvm.org/D59960