> On Apr 25, 2018, at 5:52 PM, Greg Clayton via lldb-dev
> <lldb-dev@lists.llvm.org> wrote:
>
> I have identified an issue in the expression parse. If you compile this code:
>
> struct Foo { typedef unsigned node; };
> struct Bar { typedef int node; };
> int main(int argc, const char **argv) {
> Foo::node foo_carp = 22;
> Bar::node bar_node = 33;
> return 0;
> }
>
>
> Then then do:
>
> (lldb) file a.out
> (lldb) b main
> (lldb) run
> (lldb) p (node *)&bar_node
>
> This should fail because there is no "node" type in the global namespace. But
> we don't fail, we pick the first type whose base name matches "node" and
> return it and start using it regardless of where the type is located decl
> context wise...
>
> The issue happens when we call:
>
> void ClangASTSource::FindExternalVisibleDecls(NameSearchContext &context,
> lldb::ModuleSP module_sp, CompilerDeclContext &namespace_decl, unsigned int
> current_id);
>
> In this code we end up doing:
>
> TypeList types;
> SymbolContext null_sc;
> const bool exact_match = false;
> 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,
> searched_symbol_files, types);
>
> if (size_t num_types = types.GetSize()) {
> for (size_t ti = 0; ti < num_types; ++ti) {
>
> So if we don't have a valid namespace_decl, which will be the case when we
> are searching at the translation unit level, we end up searching the target
> for the first match for the type whose _basename_ matches "name" regardless
> if of the module we are currently running the expression in, and regardless
> of the translation unit decl context being a requirement. So we find any type
> whose basename matches "name" from _any_ decl context. That is really bad!
> A few things wrong here:
> 1 - we seem to assume if we have a namespace (which is a parameter whose type
> is CompilerDeclContext &), that it must go with the module_sp. Why? Because a
> CompilerDeclContext has a "TypeSystem *" inside of it, and each module has a
> their own "TypeSystem *" for a give language. We then call
> module_sp->FindTypesInNamespace(...) with the CompilerDeclContext * to
> namespace_decl. This eventually makes its way down to
> SymbolFileDWARF::FindTypes(...) which will do the check:
>
> if (!DeclContextMatchesThisSymbolFile(parent_decl_ctx))
> return 0;
>
> So if the CompilerDeclContext's type system doesn't match the type system of
> the DWARF file that is makes it into, it will ignore the search entirely.
> This means if we have a namespace from an expression like "p std::foo", and
> we find the namespace "foo" in one module, we will only search a specific
> module for "foo" within "std", even though another module could contain
> "std::foo". So if the first module that has debug info for namespace "std"
> doesn't contain "foo", we don't find the type?
The code above what you cited looks to see if the CompilerDeclContext is in the
module, and if it isn't it searches all the modules for that
CompilerDeclContext and then adds the appropriate module_sp and
CompilerDeclContext to the NameSpaceSearch context object. As an aside, that
search should probably break when it finds the namespace, since it's not going
to find that CompilerDeclContext in another module...
It seems like we should then use the NamespaceSearchContext that we went to the
trouble of making up, or at least look in the module we actually already found
containing that Namespace.
This search is gated on !HasMerger, and HasMerger says:
//------------------------------------------------------------------
/// Returns true if there is a merger. This only occurs if the target is
/// using modern lookup.
//------------------------------------------------------------------
This "modern lookup" is a target property that is off by default. So we are
doing this search.
I wonder if there's just something that didn't get wired up correctly when Sean
was adding the modern lookup.
>
> 2 - If we don't have a namespace, we want things only at the translation unit
> level, yet we search all modules in the target in the target image list
> order. It seems we should be checking by starting with the current module,
> then expanding to any module except the current module if the type lookup
> fails in the current module. Then we need to search for more than 1 type so
> we can weed any results out that don't have a decl context of a translation
> unit level or anonymous namespace from the current translation unit.
Yes, we certainly should start with the current module. This does seem wrong
to me. Finding only one match also seems wrong. This is providing a
convenience - not having to type in the full name of a scoped object. But we
certainly should not do that if the name lookup was ambiguous, and you can't
tell that if you only get one match. I wonder what the logic was here?
>
> So to fix this I might propose the following:
>
> Modify all "FindTypes()" functions to take a new version of something like
> the DWARFDeclContext from DWARFDeclContext.h. This is an object that contains
> a enumeration for the decl context type plus an optional name . This think
> things like "translation unit with name 'main.c'", class with name "Foo",
> namespace with name "std", function with name "erase". This new item could
> be something like:
>
> class DeclContextMatch {
> enum class Kind {
> TranslationUnit,
> Namespace,
> Class,
> Struct,
> Union,
> Function
> };
> struct Entry {
> Kind kind;
> ConstString name;
> };
> std::vector<Entry> m_entries;
> void Append(DeclContextMatch::Kind k, const char *name = nullptr);
> };
>
> We would stop passing the "const CompilerDeclContext *parent_decl_ctx" to the
> ::FindTypes(...) calls and instead pass a "DeclContextMatch *" so that
> clients can easily weed out things that don't match in an abstract way that
> doesn't tie the containing decl context to the current module. So if I was
> doing to search for "node" at the translation unit level, I would fill in:
>
> DeclContextMatch dcm;
> dcm.Append(DeclContextMatch::Kind::TranslationUnit);
> target->GetImages.FindTypes(null_sc, name, &dcm, ...)
>
>
> Since the DeclContextMatch isn't specific to any module (unlike
> CompilerDeclContext instances which are), the search can happen and each
> SymbolFile subclass can search how ever they need to by only returning
> matches that match the DeclContextMatch if one is given. If no
> DeclContextMatch is supplied, then we search for any type whose basename
> matches the name requested. If DeclContextMatch is given, only matching types
> are returned.
Before I went inventing something new, I'd like to understand what this
NameSearchContext is, and why we bother to find the right module to go with the
namespace context, but then seem to ignore it a few lines later.
Jim
>
> This has to be causing all sorts of problems in our expressions, so we really
> need to fix this ASAP. Comments and suggestions are welcome.
>
> Greg
>
> _______________________________________________
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
