Fix for ambiguous lookup in expressions between local variable and namespace

[shafik]

Summary

In an Objective-C context a local variable and namespace can cause an ambiguous name lookup when used in an expression. The solution involves mimicking the existing C++ solution which is to add local using declarations for local variables. This causes a different type of lookup to be used which eliminates the namespace during acceptable results filtering.

[friss]

source/Expression/ExpressionSourceCode.cpp

171

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.

266–267

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

344

Why only in the static_method case?

[shafik]

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]

@friss I believe I have addressed your comments

[shafik]

source/Expression/ExpressionSourceCode.cpp

172

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

[friss]

source/Expression/ExpressionSourceCode.cpp

181

This should also trigger for eLanguageTypeObjC_plus_plus

359

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

[shafik]

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

[shafik]

@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]

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

[teemperor]

packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_cpp_and_c/TestNamespaceLocalVarSameNameCppAndC.py

11

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

packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_cpp_and_c/main.cpp

7

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

source/Expression/ExpressionSourceCode.cpp

172

langauge -> language

256

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

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

353

Again, end of the string should be aligned.

[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.

source/Expression/ExpressionSourceCode.cpp

267

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.

[shafik]

packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_cpp_and_c/TestNamespaceLocalVarSameNameCppAndC.py

11

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

source/Expression/ExpressionSourceCode.cpp

256

This is a good point. I am going to be working on getting this fix in place which should address this issue https://reviews.llvm.org/D46551

[clayborg]

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?

[clayborg]

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 https://reviews.llvm.org/D46551 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.

Jim

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

[shafik]

@teemperor @jingham @clayborg I believe now that https://reviews.llvm.org/D46551 is landed the performance concerns should be addressed.

[shafik]

Updating after https://reviews.llvm.org/D46551 landed

packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_cpp_and_c/Makefile

+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_cpp_and_c/TestNamespaceLocalVarSameNameCppAndC.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestNamespaceLocalVarSameNameCppAndC(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipUnlessDarwin
+    @add_test_categories(["gmodules"])
+    def test_namespace_local_var_same_name_cpp_and_c(self):
+        self.build()
+
+        (self.target, self.process, _, bkpt) = lldbutil.run_to_source_breakpoint(self, '// break here',
+                lldb.SBFileSpec("main.cpp", False))
+
+        self.expect("expr error",
+                substrs=['(int) $0 = 1'])
+
+        lldbutil.continue_to_breakpoint(self.process, bkpt)
+
+        self.expect("expr error",
+                substrs=['(int) $1 = 1'])

packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_cpp_and_c/main.cpp

+namespace error {
+  int x;
+}
+
+struct A { 
+  void foo() {
+    int error=1; 
+
+    return; // break here
+  }   
+};
+
+int main() {
+ int error=1;
+
+ A a;
+
+ a.foo();
+
+ return 0; // break here
+}

packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_obj_c/Makefile

+LEVEL = ../../make
+OBJCXX_SOURCES := main.mm util.mm
+include $(LEVEL)/Makefile.rules
+
+LDFLAGS += -framework Foundation

packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_obj_c/TestNamespaceLocalVarSameNameObjC.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestNamespaceLocalVarSameNameObjC(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipUnlessDarwin
+    @add_test_categories(["gmodules"])
+    def test_namespace_local_var_same_name_obj_c(self):
+        self.build()
+
+        (self.target, self.process, _, bkpt) = lldbutil.run_to_source_breakpoint(self, '// break here',
+                lldb.SBFileSpec("util.mm", False))
+
+        self.expect("expr error",
+                substrs=['(NSError *) $0 ='])
+
+        lldbutil.continue_to_breakpoint(self.process, bkpt)
+
+        self.expect("expr error",
+                substrs=['(NSError *) $1 ='])

packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name_obj_c/util.mm

+#import <Foundation/Foundation.h>
+
+namespace error {
+int blah;
+}
+
+@interface Util : NSObject
++ (void)debugPrintErrorStatic;
+- (void)debugPrintError;
+@end
+
+@implementation Util
++ (void)debugPrintErrorStatic {
+  NSError* error = [NSError errorWithDomain:NSURLErrorDomain code:-1 userInfo:nil];
+  NSLog(@"xxx, error = %@", error); // break here
+}
+
+- (void)debugPrintError {
+  NSError* error = [NSError errorWithDomain:NSURLErrorDomain code:-1 userInfo:nil];
+  NSLog(@"xxx, error = %@", error); // break here
+}
+@end

source/Expression/ExpressionSourceCode.cpp

for (size_t i = 0; i < dm->GetNumMacroEntries(); i++) {
     default:
       // This is an unknown/invalid entry. Ignore.
       break;
     }
   }
 }
 
 static void AddLocalVariableDecls(const lldb::VariableListSP &var_list_sp,
-                                  StreamString &stream) {
+                                  StreamString &stream,
+                                  lldb::LanguageType wrapping_language) {
   for (size_t i = 0; i < var_list_sp->GetSize(); i++) {
     lldb::VariableSP var_sp = var_list_sp->GetVariableAtIndex(i);
 
     ConstString var_name = var_sp->GetName();
-    if (!var_name || var_name == ConstString("this") ||
-        var_name == ConstString(".block_descriptor"))
+
+    // We can check for .block_descriptor w/o checking for langauge since this
+    // is not a valid identifier in either C or C++.
+    if (!var_name || var_name == ConstString(".block_descriptor"))
+      continue;
+
+    if ((var_name == ConstString("self") || var_name == ConstString("_cmd")) &&
+        (wrapping_language == lldb::eLanguageTypeObjC ||
+         wrapping_language == lldb::eLanguageTypeObjC_plus_plus))
+      continue;
+
+    if (var_name == ConstString("this") &&
+        wrapping_language == lldb::eLanguageTypeC_plus_plus)
       continue;
 
     stream.Printf("using $__lldb_local_vars::%s;\n", var_name.AsCString());
   }
 }
 
 bool ExpressionSourceCode::GetText(std::string &text,
                                    lldb::LanguageType wrapping_language,
if (StackFrame *frame = exe_ctx.GetFramePtr()) {
 
     if (sc.comp_unit && sc.line_entry.IsValid()) {
       DebugMacros *dm = sc.comp_unit->GetDebugMacros();
       if (dm) {
         AddMacroState state(sc.line_entry.file, sc.line_entry.line);
         AddMacros(dm, sc.comp_unit, state, debug_macros_stream);
       }
     }
 
-    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)
+        AddLocalVariableDecls(var_list_sp, lldb_local_var_decls,
+                              wrapping_language);
       }
   }
 
   if (m_wrap) {
     switch (wrapping_language) {
     default:
       return false;
     case lldb::eLanguageTypeC:
     case lldb::eLanguageTypeC_plus_plus:
case lldb::eLanguageTypeObjC:
       if (static_method) {
         wrap_stream.Printf(
             "@interface $__lldb_objc_class ($__lldb_category)        \n"
             "+(void)%s:(void *)$__lldb_arg;                          \n"
             "@end                                                    \n"
             "@implementation $__lldb_objc_class ($__lldb_category)   \n"
             "+(void)%s:(void *)$__lldb_arg                           \n"
             "{                                                       \n"
+            "    %s;                                \n"
             "%s"
             "}                                                       \n"
             "@end                                                    \n",
-            m_name.c_str(), m_name.c_str(), tagged_body.c_str());
+            m_name.c_str(), m_name.c_str(), lldb_local_var_decls.GetData(),
+            tagged_body.c_str());
       } else {
         wrap_stream.Printf(
             "@interface $__lldb_objc_class ($__lldb_category)       \n"
             "-(void)%s:(void *)$__lldb_arg;                         \n"
             "@end                                                   \n"
             "@implementation $__lldb_objc_class ($__lldb_category)  \n"
             "-(void)%s:(void *)$__lldb_arg                          \n"
             "{                                                      \n"
+            "    %s;                                \n"
             "%s"
             "}                                                      \n"
             "@end                                                   \n",
-            m_name.c_str(), m_name.c_str(), tagged_body.c_str());
+            m_name.c_str(), m_name.c_str(), lldb_local_var_decls.GetData(),
+            tagged_body.c_str());
       }
       break;
     }
 
     text = wrap_stream.GetString();
   } else {
     text.append(m_body);
   }