[Expression parser] Return both types and variables

[spyffe]

Summary

Many times a user wants to access a type when there's a variable of the same name, or a variable when there's a type of the same name. Depending on the precise context, currently the expression parser can fail to resolve one or the other.

This is because ClangExpressionDeclMap has logic to limit the amount of information it searches, and that logic sometimes cuts down the search prematurely. This patch removes some of those early exits.

In that sense, this patch trades performance (early exit is faster) for correctness.

I've also included two new test cases showing examples of this behavior – as well as modifying an existing test case that gets it wrong.

[jingham]

This looks okay on the lldb side. Thanks for adding the test cases. I do worry that we're going to end up looking too far afield for types when the local variable. But we can make it correct first and see what we can do about performance if that becomes an issue.

[spyffe]

Closed by r301273

packages/Python/lldbsuite/test/lang/cpp/llvm-style/Makefile

+LEVEL = ../../../make
+C_SOURCES := main.c
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/lang/cpp/llvm-style/TestLLVMStyle.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(
+    __file__, globals(), [
+        decorators.expectedFailureAll(
+            oslist=["windows"], bugnumber="llvm.org/pr24764")])

packages/Python/lldbsuite/test/lang/cpp/llvm-style/main.cc

+//===-- main.cc -------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LIDENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+namespace n {
+    struct D {
+        int i;
+        static int anInt() { return 2; }
+        int dump() { return i; }
+    };
+
+    class C {
+    public:
+        int foo(D *D);
+    };
+}
+
+using namespace n;
+
+int C::foo(D* D) {
+    return D->dump(); //% self.expect("expression -- D->dump()", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["int", "2"])
+                      //% self.expect("expression -- D::anInt()", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["int", "2"])
+
+}
+
+int main (int argc, char const *argv[])
+{
+    D myD { D::anInt() };
+    C().foo(&myD);
+    return 0; 
+}

packages/Python/lldbsuite/test/lang/cpp/nsimport/TestCppNsImport.py

def test_with_run_command(self):
 
         test_result = frame.EvaluateExpression("Imported::imported")
         self.assertTrue(
             test_result.IsValid() and test_result.GetValueAsSigned() == 99,
             "Imported::imported = 99")
 
         test_result = frame.EvaluateExpression("imported")
         self.assertTrue(
-            test_result.IsValid() and test_result.GetError().Fail(),
-            "imported is ambiguous")
+            test_result.IsValid() and test_result.GetValueAsSigned() == 99,
+            "imported = 99")
 
         test_result = frame.EvaluateExpression("single")
         self.assertTrue(
             test_result.IsValid() and test_result.GetValueAsSigned() == 3,
             "single = 3")
 
         # Continue to second breakpoint
         process.Continue()

packages/Python/lldbsuite/test/lang/cpp/symbols/Makefile

+LEVEL = ../../../make
+C_SOURCES := main.c
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/lang/cpp/symbols/TestSymbpls.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(
+    __file__, globals(), [
+        decorators.expectedFailureAll(
+            oslist=["windows"], bugnumber="llvm.org/pr24764")])

packages/Python/lldbsuite/test/lang/cpp/symbols/main.cc

+//===-- main.cc -------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LIDENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+void *D = 0;
+
+class D {
+    static int i;
+};
+
+int D::i = 3;
+
+namespace errno {
+    int j = 4;
+};
+
+int twice(int n)
+{
+    return n * 2; //% self.expect("expression -- D::i", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["int", "3"])
+                  //% self.expect("expression -- D", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["void"])
+                  //% self.expect("expression -- errno::j", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["int", "4"])
+}
+
+const char getAChar()
+{
+    const char D[] = "Hello world";
+    return D[0];  //% self.expect("expression -- D::i", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["int", "3"])
+                  //% self.expect("expression -- D", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["char", "Hello"])
+}
+
+int main (int argc, char const *argv[])
+{
+    int six = twice(3);
+    return 0; 
+}

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

if (const NamespaceDecl *namespace_context =
     CompilerDeclContext namespace_decl;
 
     if (log)
       log->Printf("  CEDM::FEVD[%u] Searching the root namespace", current_id);
 
     FindExternalVisibleDecls(context, lldb::ModuleSP(), namespace_decl,
                              current_id);
   }
   
-  if (!context.m_found.variable && !context.m_found.local_vars_nsp)
-    ClangASTSource::FindExternalVisibleDecls(context);
 }
 
 void ClangExpressionDeclMap::FindExternalVisibleDecls(
     NameSearchContext &context, lldb::ModuleSP module_sp,
     CompilerDeclContext &namespace_decl, unsigned int current_id) {
   assert(m_ast_context);
 
   std::function<void(clang::FunctionDecl *)> MaybeRegisterFunctionBody =
if (frame && local_var_lookup) {
           for (size_t vi = 0, ve = vars->GetSize(); vi != ve; ++vi) {
             VariableSP candidate_var = vars->GetVariableAtIndex(vi);
             if (candidate_var->GetDecl() == decl) {
               var = candidate_var;
               break;
             }
           }
 
-          if (var) {
+          if (var && !variable_found) {
             variable_found = true;
             valobj = ValueObjectVariable::Create(frame, var);
             AddOneVariable(context, var, valobj, current_id);
             context.m_found.variable = true;
           }
         }
         if (variable_found)
           return;
if (name_unique_cstr[0] == '$' && !namespace_decl) {
 
     if (target) {
       if (ClangModulesDeclVendor *decl_vendor =
               target->GetClangModulesDeclVendor()) {
         decl_vendor->FindDecls(name, false, UINT32_MAX, decls_from_modules);
       }
     }
 
-    if (!context.m_found.variable) {
-      const bool include_inlines = false;
-      const bool append = false;
 
-      if (namespace_decl && module_sp) {
-        const bool include_symbols = false;
 
-        module_sp->FindFunctions(name, &namespace_decl, eFunctionNameTypeBase,
-                                 include_symbols, include_inlines, append,
-                                 sc_list);
-      } else if (target && !namespace_decl) {
-        const bool include_symbols = true;
 
-        // TODO Fix FindFunctions so that it doesn't return
-        //   instance methods for eFunctionNameTypeBase.
 
-        target->GetImages().FindFunctions(name, eFunctionNameTypeFull,
-                                          include_symbols, include_inlines,
-                                          append, sc_list);
-      }
 
-      // If we found more than one function, see if we can use the
-      // frame's decl context to remove functions that are shadowed
-      // by other functions which match in type but are nearer in scope.
-      //
-      // AddOneFunction will not add a function whose type has already been
-      // added, so if there's another function in the list with a matching
-      // type, check to see if their decl context is a parent of the current
-      // frame's or was imported via a and using statement, and pick the
-      // best match according to lookup rules.
-      if (sc_list.GetSize() > 1) {
-        // Collect some info about our frame's context.
-        StackFrame *frame = m_parser_vars->m_exe_ctx.GetFramePtr();
-        SymbolContext frame_sym_ctx;
-        if (frame != nullptr)
-          frame_sym_ctx = frame->GetSymbolContext(lldb::eSymbolContextFunction |
-                                                  lldb::eSymbolContextBlock);
-        CompilerDeclContext frame_decl_context =
-            frame_sym_ctx.block != nullptr
-                ? frame_sym_ctx.block->GetDeclContext()
+          frame_sym_ctx.block != nullptr ? frame_sym_ctx.block->GetDeclContext()
                                          : CompilerDeclContext();
 
-        // We can't do this without a compiler decl context for our frame.
-        if (frame_decl_context) {
-          clang::DeclContext *frame_decl_ctx =
-              (clang::DeclContext *)frame_decl_context.GetOpaqueDeclContext();
-          ClangASTContext *ast = llvm::dyn_cast_or_null<ClangASTContext>(
-              frame_decl_context.GetTypeSystem());
 
-          // Structure to hold the info needed when comparing function
-          // declarations.
-          struct FuncDeclInfo {
-            ConstString m_name;
-            CompilerType m_copied_type;
-            uint32_t m_decl_lvl;
-            SymbolContext m_sym_ctx;
-          };
 
-          // First, symplify things by looping through the symbol contexts
-          // to remove unwanted functions and separate out the functions we
-          // want to compare and prune into a separate list.
-          // Cache the info needed about the function declarations in a
-          // vector for efficiency.
-          SymbolContextList sc_sym_list;
-          uint32_t num_indices = sc_list.GetSize();
-          std::vector<FuncDeclInfo> fdi_cache;
-          fdi_cache.reserve(num_indices);
-          for (uint32_t index = 0; index < num_indices; ++index) {
-            FuncDeclInfo fdi;
-            SymbolContext sym_ctx;
-            sc_list.GetContextAtIndex(index, sym_ctx);
 
-            // We don't know enough about symbols to compare them,
-            // but we should keep them in the list.
-            Function *function = sym_ctx.function;
-            if (!function) {
-              sc_sym_list.Append(sym_ctx);
-              continue;
-            }
-            // Filter out functions without declaration contexts, as well as
-            // class/instance methods, since they'll be skipped in the
-            // code that follows anyway.
-            CompilerDeclContext func_decl_context = function->GetDeclContext();
-            if (!func_decl_context ||
-                func_decl_context.IsClassMethod(nullptr, nullptr, nullptr))
-              continue;
-            // We can only prune functions for which we can copy the type.
-            CompilerType func_clang_type =
-                function->GetType()->GetFullCompilerType();
-            CompilerType copied_func_type = GuardedCopyType(func_clang_type);
-            if (!copied_func_type) {
-              sc_sym_list.Append(sym_ctx);
-              continue;
-            }
 
-            fdi.m_sym_ctx = sym_ctx;
-            fdi.m_name = function->GetName();
-            fdi.m_copied_type = copied_func_type;
-            fdi.m_decl_lvl = LLDB_INVALID_DECL_LEVEL;
-            if (fdi.m_copied_type && func_decl_context) {
-              // Call CountDeclLevels to get the number of parent scopes we
-              // have to look through before we find the function declaration.
-              // When comparing functions of the same type, the one with a
-              // lower count will be closer to us in the lookup scope and
-              // shadows the other.
-              clang::DeclContext *func_decl_ctx =
-                  (clang::DeclContext *)
-                      func_decl_context.GetOpaqueDeclContext();
-              fdi.m_decl_lvl =
-                  ast->CountDeclLevels(frame_decl_ctx, func_decl_ctx,
-                                       &fdi.m_name, &fdi.m_copied_type);
-            }
-            fdi_cache.emplace_back(fdi);
-          }
+                (clang::DeclContext *)func_decl_context.GetOpaqueDeclContext();
+            fdi.m_decl_lvl = ast->CountDeclLevels(
+                frame_decl_ctx, func_decl_ctx, &fdi.m_name, &fdi.m_copied_type);
 
-          // Loop through the functions in our cache looking for matching types,
-          // then compare their scope levels to see which is closer.
-          std::multimap<CompilerType, const FuncDeclInfo *> matches;
-          for (const FuncDeclInfo &fdi : fdi_cache) {
-            const CompilerType t = fdi.m_copied_type;
-            auto q = matches.find(t);
-            if (q != matches.end()) {
-              if (q->second->m_decl_lvl > fdi.m_decl_lvl)
-                // This function is closer; remove the old set.
-                matches.erase(t);
-              else if (q->second->m_decl_lvl < fdi.m_decl_lvl)
-                // The functions in our set are closer - skip this one.
-                continue;
-            }
-            matches.insert(std::make_pair(t, &fdi));
-          }
 
-          // Loop through our matches and add their symbol contexts to our list.
-          SymbolContextList sc_func_list;
-          for (const auto &q : matches)
-            sc_func_list.Append(q.second->m_sym_ctx);
 
-          // Rejoin the lists with the functions in front.
-          sc_list = sc_func_list;
-          sc_list.Append(sc_sym_list);
-        }
-      }
 
-      if (sc_list.GetSize()) {
-        Symbol *extern_symbol = NULL;
-        Symbol *non_extern_symbol = NULL;
 
-        for (uint32_t index = 0, num_indices = sc_list.GetSize();
-             index < num_indices; ++index) {
-          SymbolContext sym_ctx;
-          sc_list.GetContextAtIndex(index, sym_ctx);
 
-          if (sym_ctx.function) {
-            CompilerDeclContext decl_ctx = sym_ctx.function->GetDeclContext();
 
-            if (!decl_ctx)
-              continue;
 
-            // Filter out class/instance methods.
-            if (decl_ctx.IsClassMethod(nullptr, nullptr, nullptr))
-              continue;
 
-            AddOneFunction(context, sym_ctx.function, NULL, current_id);
-            context.m_found.function_with_type_info = true;
-            context.m_found.function = true;
-          } else if (sym_ctx.symbol) {
-            if (sym_ctx.symbol->GetType() == eSymbolTypeReExported && target) {
-              sym_ctx.symbol = sym_ctx.symbol->ResolveReExportedSymbol(*target);
-              if (sym_ctx.symbol == NULL)
-                continue;
-            }
 
-            if (sym_ctx.symbol->IsExternal())
-              extern_symbol = sym_ctx.symbol;
-            else
-              non_extern_symbol = sym_ctx.symbol;
-          }
-        }
 
-        if (!context.m_found.function_with_type_info) {
-          for (clang::NamedDecl *decl : decls_from_modules) {
-            if (llvm::isa<clang::FunctionDecl>(decl)) {
-              clang::NamedDecl *copied_decl =
-                  llvm::cast_or_null<FunctionDecl>(m_ast_importer_sp->CopyDecl(
-                      m_ast_context, &decl->getASTContext(), decl));
-              if (copied_decl) {
-                context.AddNamedDecl(copied_decl);
-                context.m_found.function_with_type_info = true;
-              }
-            }
-          }
-        }
 
-        if (!context.m_found.function_with_type_info) {
-          if (extern_symbol) {
-            AddOneFunction(context, NULL, extern_symbol, current_id);
-            context.m_found.function = true;
-          } else if (non_extern_symbol) {
-            AddOneFunction(context, NULL, non_extern_symbol, current_id);
-            context.m_found.function = true;
-          }
-        }
-      }
 
-      if (!context.m_found.function_with_type_info) {
-        // Try the modules next.
 
-        do {
-          if (ClangModulesDeclVendor *modules_decl_vendor =
-                  m_target->GetClangModulesDeclVendor()) {
-            bool append = false;
-            uint32_t max_matches = 1;
-            std::vector<clang::NamedDecl *> decls;
 
-            if (!modules_decl_vendor->FindDecls(name, append, max_matches,
-                                                decls))
-              break;
+          if (!modules_decl_vendor->FindDecls(name, append, max_matches, decls))
 
-            clang::NamedDecl *const decl_from_modules = decls[0];
 
-            if (llvm::isa<clang::FunctionDecl>(decl_from_modules)) {
-              if (log) {
-                log->Printf("  CAS::FEVD[%u] Matching function found for "
-                            "\"%s\" in the modules",
-                            current_id, name.GetCString());
-              }
 
-              clang::Decl *copied_decl = m_ast_importer_sp->CopyDecl(
-                  m_ast_context, &decl_from_modules->getASTContext(),
-                  decl_from_modules);
-              clang::FunctionDecl *copied_function_decl =
-                  copied_decl ? dyn_cast<clang::FunctionDecl>(copied_decl)
-                              : nullptr;
 
-              if (!copied_function_decl) {
-                if (log)
-                  log->Printf("  CAS::FEVD[%u] - Couldn't export a function "
-                              "declaration from the modules",
-                              current_id);
 
-                break;
-              }
 
-              MaybeRegisterFunctionBody(copied_function_decl);
 
-              context.AddNamedDecl(copied_function_decl);
 
-              context.m_found.function_with_type_info = true;
-              context.m_found.function = true;
-            } else if (llvm::isa<clang::VarDecl>(decl_from_modules)) {
-              if (log) {
-                log->Printf("  CAS::FEVD[%u] Matching variable found for "
-                            "\"%s\" in the modules",
-                            current_id, name.GetCString());
-              }
 
-              clang::Decl *copied_decl = m_ast_importer_sp->CopyDecl(
-                  m_ast_context, &decl_from_modules->getASTContext(),
-                  decl_from_modules);
-              clang::VarDecl *copied_var_decl =
-                  copied_decl ? dyn_cast_or_null<clang::VarDecl>(copied_decl)
-                              : nullptr;
 
-              if (!copied_var_decl) {
-                if (log)
-                  log->Printf("  CAS::FEVD[%u] - Couldn't export a variable "
-                              "declaration from the modules",
-                              current_id);
 
-                break;
-              }
 
-              context.AddNamedDecl(copied_var_decl);
 
-              context.m_found.variable = true;
-            }
-          }
-        } while (0);
-      }
 
-      if (target && !context.m_found.variable && !namespace_decl) {
-        // We couldn't find a non-symbol variable for this.  Now we'll hunt for
-        // a generic
-        // data symbol, and -- if it is found -- treat it as a variable.
 
-        const Symbol *data_symbol = FindGlobalDataSymbol(*target, name);
 
-        if (data_symbol) {
-          std::string warning("got name from symbols: ");
-          warning.append(name.AsCString());
-          const unsigned diag_id =
-              m_ast_context->getDiagnostics().getCustomDiagID(
-                  clang::DiagnosticsEngine::Level::Warning, "%0");
-          m_ast_context->getDiagnostics().Report(diag_id) << warning.c_str();
-          AddOneGenericVariable(context, *data_symbol, current_id);
-          context.m_found.variable = true;
-        }
-      }
-    }
-  }
-}
 
 // static opaque_compiler_type_t
 // MaybePromoteToBlockPointerType
 //(
 //    ASTContext *ast_context,
 //    opaque_compiler_type_t candidate_type
 //)
 //{