Fix expression parser to not accept any type whose basename matches for a type that must exist at root level

[clayborg]

Summary

This patch fixes an issue where we weren't looking for exact matches in the expression parser and also fixed the type lookup logic in the Module.cpp. Tests added to make sure we don't regress.

[clayborg]

ping?

[aprantl]

packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py

41 ↗

(On Diff #144133)

accidentally

[labath]

Jim seems to have an idea on how the type lookup should work, so I'll defer to him.

[aprantl]

To aid with reviewing I documented LLDB's current behavior. I don't understand the implementation well enough to comment on it, but I do think the the new behavior is superior over the old one. After this patch LLDB behaves deterministic and like the compiler would behave on that line, before it, LLDB would do a DWIM thing with unpredictable results. So unless Jim has any concerns about the implementation, I would give this a go.

packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py

48 ↗

(On Diff #144133)

This currently returns

(a::namespace_only) $0 = (a = 123)

which — while convenient — behaves differently from code injected at that point in the program.

55 ↗

(On Diff #144133)

both of these work at the moment.

59 ↗

(On Diff #144133)

Currently:

error: reference to 'namespace_and_file' is ambiguous
candidate found by name lookup is 'namespace_and_file'
candidate found by name lookup is 'a::namespace_and_file'
error: reference to 'namespace_and_file' is ambiguous
candidate found by name lookup is 'namespace_and_file'
candidate found by name lookup is 'a::namespace_and_file'

that's a horrible failure mode.

67 ↗

(On Diff #144133)

this works

69 ↗

(On Diff #144133)

accidentally

76 ↗

(On Diff #144133)

(lldb) p *((in_contains_type *)&i)
(a::contains_type::in_contains_type) $5 = (aaa = 123)

80 ↗

(On Diff #144133)

Similar ugly failure:

error: reference to 'contains_type' is ambiguous
candidate found by name lookup is 'contains_type'
candidate found by name lookup is 'a::contains_type'
error: expected expression

88 ↗

(On Diff #144133)

these work as expected.

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/type_lookup/Makefile

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

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py

+"""
+Test that we can lookup types correctly in the expression parser
+"""
+
+from __future__ import print_function
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import decorators
+
+class TestTypeLookup(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def check_value(self, value, ivar_name, ivar_value):
+        self.assertTrue(value.GetError().Success(),
+                        "Invalid valobj: %s" % (
+                                value.GetError().GetCString()))
+        ivar = value.GetChildMemberWithName(ivar_name)
+        self.assertTrue(ivar.GetError().Success(),
+                        "Failed to fetch ivar named '%s'" % (ivar_name))
+        self.assertEqual(ivar_value,
+                         ivar.GetValueAsSigned(),
+                         "Got the right value for ivar")
+
+    def test_namespace_only(self):
+        """
+            Test that we fail to lookup a struct type that exists only in a
+            namespace.
+        """
+        self.build()
+        self.main_source_file = lldb.SBFileSpec("main.cpp")
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+            self, "Set a breakpoint here", self.main_source_file)
+
+        # Get frame for current thread
+        frame = thread.GetSelectedFrame()
+
+        # Make sure we don't accidentally accept structures that exist only
+        # in namespaces when evaluating expressions with top level types.
+        # Prior to the revision that added this test, we would accidentally
+        # accept types from namespaces, so this will ensure we don't regress
+        # to that behavior again
+        expr_result = frame.EvaluateExpression("*((namespace_only *)&i)")
+        self.assertTrue(expr_result.GetError().Fail(),
+                        "'namespace_only' exists in namespace only")
+
+        # Make sure we can find the correct type in a namespace "a"
+        expr_result = frame.EvaluateExpression("*((a::namespace_only *)&i)")
+        self.check_value(expr_result, "a", 123)
+        # Make sure we can find the correct type in a namespace "b"
+        expr_result = frame.EvaluateExpression("*((b::namespace_only *)&i)")
+        self.check_value(expr_result, "b", 123)
+
+        # Make sure we can find the correct type in the root namespace
+        expr_result = frame.EvaluateExpression("*((namespace_and_file *)&i)")
+        self.check_value(expr_result, "ff", 123)
+        # Make sure we can find the correct type in a namespace "a"
+        expr_result = frame.EvaluateExpression(
+                "*((a::namespace_and_file *)&i)")
+        self.check_value(expr_result, "aa", 123)
+        # Make sure we can find the correct type in a namespace "b"
+        expr_result = frame.EvaluateExpression(
+                "*((b::namespace_and_file *)&i)")
+        self.check_value(expr_result, "bb", 123)
+
+        # Make sure we don't accidentally accept structures that exist only
+        # in namespaces when evaluating expressions with top level types.
+        # Prior to the revision that added this test, we would accidentally
+        # accept types from namespaces, so this will ensure we don't regress
+        # to that behavior again
+        expr_result = frame.EvaluateExpression("*((in_contains_type *)&i)")
+        self.assertTrue(expr_result.GetError().Fail(),
+                        "'in_contains_type' exists in struct only")
+
+        # Make sure we can find the correct type in the root namespace
+        expr_result = frame.EvaluateExpression(
+                "*((contains_type::in_contains_type *)&i)")
+        self.check_value(expr_result, "fff", 123)
+        # Make sure we can find the correct type in a namespace "a"
+        expr_result = frame.EvaluateExpression(
+                "*((a::contains_type::in_contains_type *)&i)")
+        self.check_value(expr_result, "aaa", 123)
+        # Make sure we can find the correct type in a namespace "b"
+        expr_result = frame.EvaluateExpression(
+                "*((b::contains_type::in_contains_type *)&i)")
+        self.check_value(expr_result, "bbb", 123)

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/type_lookup/main.cpp

+//===-- main.c --------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// In this test, we define struct that exist might exist at the different
+// levels in the code and test that we can properly locate these types with
+// a varienty of different expressions.
+
+namespace a {
+  struct namespace_only {
+    int a;
+  };
+  struct namespace_and_file {
+    int aa;
+  };
+  struct contains_type {
+    struct in_contains_type {
+      int aaa;
+    };
+  };
+};
+namespace b {
+  struct namespace_only {
+    int b;
+  };
+  struct namespace_and_file {
+    int bb;
+  };
+  struct contains_type {
+    struct in_contains_type {
+      int bbb;
+    };
+  };
+};
+
+struct namespace_and_file {
+  int ff;
+};
+
+struct contains_type {
+  struct in_contains_type {
+    int fff;
+  };
+};
+
+
+int main (int argc, char const *argv[]) {
+  a::namespace_only a_namespace_only = { 1 };
+  a::namespace_and_file a_namespace_and_file = { 2 };
+  a::contains_type::in_contains_type a_in_contains_type = { 3 };
+  b::namespace_only b_namespace_only = { 11 };
+  b::namespace_and_file b_namespace_and_file = { 22 };
+  b::contains_type::in_contains_type b_in_contains_type = { 33 };
+  namespace_and_file file_namespace_and_file = { 44 };
+  contains_type::in_contains_type file_in_contains_type = { 55 };
+  int i = 123; // Provide an integer that can be used for casting
+  // Take address of "i" to ensure it is in memory
+  if (&i == &argc) {
+    i = -1;
+  }
+  return i == -1; // Set a breakpoint here
+}

lldb/trunk/source/Core/Module.cpp

if (FindTypes_Impl(sc, type_basename_const_str, nullptr, append,
                        max_matches, searched_symbol_files, typesmap)) {
       typesmap.RemoveMismatchedTypes(type_scope, type_basename, type_class,
                                      exact_match);
       num_matches = typesmap.GetSize();
     }
   } else {
     // The type is not in a namespace/class scope, just search for it by
     // basename
-    if (type_class != eTypeClassAny) {
+    if (type_class != eTypeClassAny && !type_basename.empty()) {
       // The "type_name_cstr" will have been modified if we have a valid type
       // class prefix (like "struct", "class", "union", "typedef" etc).
       FindTypes_Impl(sc, ConstString(type_basename), nullptr, append,
                      max_matches, searched_symbol_files, typesmap);
-      typesmap.RemoveMismatchedTypes(type_class);
+      typesmap.RemoveMismatchedTypes(type_scope, type_basename, type_class,
+                                     exact_match);
       num_matches = typesmap.GetSize();
     } else {
       num_matches = FindTypes_Impl(sc, name, nullptr, append, max_matches,
                                    searched_symbol_files, typesmap);
+      if (exact_match) {
+        std::string name_str(name.AsCString(""));
+        typesmap.RemoveMismatchedTypes(type_scope, name_str, type_class,
+                                       exact_match);
+      }
     }
   }
   if (num_matches > 0)
     sc.SortTypeList(typesmap, types);
   return num_matches;
 }
 
 SymbolVendor *Module::GetSymbolVendor(bool can_create,

lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

void ClangASTSource::FindExternalVisibleDecls(
   }
 
   do {
     if (context.m_found.type)
       break;
 
     TypeList types;
     SymbolContext null_sc;
-    const bool exact_match = false;
+    const bool exact_match = true;
     llvm::DenseSet<lldb_private::SymbolFile *> searched_symbol_files;
     if (module_sp && namespace_decl)
       module_sp->FindTypesInNamespace(null_sc, name, &namespace_decl, 1, types);
-    else
-      m_target->GetImages().FindTypes(null_sc, name, exact_match, 1,
+    else {
+      SymbolContext sc;
+      sc.module_sp = module_sp;
+      m_target->GetImages().FindTypes(sc, name, exact_match, 1,
                                       searched_symbol_files, types);
+    }
 
     if (size_t num_types = types.GetSize()) {
       for (size_t ti = 0; ti < num_types; ++ti) {
         lldb::TypeSP type_sp = types.GetTypeAtIndex(ti);
 
         if (log) {
           const char *name_string = type_sp->GetName().GetCString();