[lldb-dev] Issue with expression parser finding the correct types
Jim Ingham via lldb-dev
lldb-dev at lists.llvm.org
Wed Apr 25 19:49:16 PDT 2018
> On Apr 25, 2018, at 5:52 PM, Greg Clayton via lldb-dev <lldb-dev at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
More information about the lldb-dev
mailing list