[lldb] Stop searching for a symbol in a pdb by regex

[asmith]

Summary

It was possible when searching for a symbol by regex in a pdb that an invalid regex would cause an exception on Windows. This updates the code to avoid throwing an exception.

When fixing the exception it was decided there is no reason to search for a symbol in a pdb by regex. To support this, SymbolFilePDB::FindTypes() now only searches for types by name and no longer calls FindTypesByRegEx().

[zturner]

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

392–399

I can see two possible problems here.

1. Now if it is a valid regex, it's going to compile it twice, hurting performance (not sure by how much, it could be negligible)

2. Whether or not it's valid is determined by asking LLDB's regex engine, but it's then compiled with C++ STL's engine. It's possible they won't agree on whether or not a regex is valid.

You mentioned that it throws an exception on an invalid regex. What actually happens though? Because we compile with exception support disabled. Does it terminate the program?

At a minimum, the validity check and the find function should agree on the regex engine. If the only way to make that work is to change from std::regex to lldb_private::RegularExpression as the matching engine, then I guess we have to do that.

[asmith]

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

393

The find_first could be removed depending on the cost of lldb_private::RegularExpression()

[clayborg]

We should have a dedicated API that actually searches for types using a lldb_private::RegularExpression. Why do we even try to create a regular expression in SymbolFilePDB::FindTypes()? Was it used in testing? No API in LLDB currently expects this and all other API in LLDB has a separate function call that uses lldb_private::RegularExpression. We can add one if needed to the lldb_private::SymbolFile API. I don't like API that magically tries to look at something and tries to compile a regular expression on each invocation. Can we change this?

[asmith]

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

392–399

How about this?

  lldb_private::RegularExpression name_regex(name_str);
  if (name_regex.IsValid())
    FindTypesByRegex(name_regex, max_matches, types); // pass regex here
  else
    FindTypesByName(name_str, max_matches, types);
  return types.GetSize();
}

void SymbolFilePDB::FindTypesByRegex(lldb_private::RegularExpression &regex,
                                     uint32_t max_matches,
                                     lldb_private::TypeMap &types) {

[asmith]

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

392–399

Our experience on Windows is that when lldb is built with exceptions disabled, std::regex segfaults on an invalid regex causing lldb to terminate.

The test case will reproduce the failure or an example from the lldb console (if you had the corresponding changes required to do the symbol lookup):

image lookup -n function_name

[asmith]

This changes SymbolFilePDB::FindTypesByRegex () to take an lldb_private::RegularExpression as the argument and removes the use of the STL regex.

[zturner]

lgtm, if you have some time in the future for further improvements Greg's suggestion would be a good way to make this better.

[clayborg]

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

393

I would rather not sniff the string passed in to see if this could be a regex and add a new FindTypesByRegex() call to lldb_private::SymbolFile that anyone can use. Do we have PDB tests that are using this functionality?

[asmith]

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

393

The error can be reproduced with:

image lookup -t char*

[clayborg]

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

394

You should make the RegularExpression object first and then check for errors. If there are errors, the type lookup should happen by normal name. This is again why I don't like us sniffing for a regular expression. If there is an error, like when trying to lookup "char*" as mentioned by Aaron, would you expect to see an error message saying "char*" isn't a valid regex? Then the user thinks that the type lookup always takes a regular expression which is not the case, just something the PDB plugin added for some reason. I do believe part of this patch should be adding the lldb_private::SymbolFile::FindTypesByRegex(...) to the base class and fixing this issue by removing the regex sniffing code since it is fragile at best (how are we doing to lookup a template type without triggering the regex issues?).

[zturner]

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

394

The reason I don't think it's appropriate for this patch is because nothing currently calls that method. So something much higher level would then have to be updated to call this new method, which might even entail adding a new command line option. For now, just fixing a crash is sufficient.

[clayborg]

If you load a exe file that has a PDB file, people can currently run:

(lldb) type lookup "char*"

If no testing is using the regular expression stuff, then just pull it out. Do we have unit tests that depend on this working? If not, lets just pull it out from the SymbolFilePDB::FindTypes() and we can leave the fix in that uses the RegularExpression class so it won't crash in the future?

[clayborg]

Looks good!

[labath]

This is making the windows unit tests fail: http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/7378/steps/run%20unit%20tests/logs/stdio. If I understand correctly, this changes FindTypes to not search by regex.
If that's the case, then the SymbolFilePDBTests::TestRegexNameMatch needs to be updated to not expect that .* will find a match.
Also, while technically not failing, the SymbolFilePDBTests::TestMaxMatches also looks weird in this new world, and probably needs updating.

Could you look into those?

source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

   } else if (auto type_def = llvm::dyn_cast<PDBSymbolTypeTypedef>(&type)) {
     lldb_private::Type *target_type =
         m_ast.GetSymbolFile()->ResolveTypeUID(type_def->getTypeId());
+    if (!target_type)
+      return nullptr;
     std::string name = type_def->getName();
     uint64_t bytes = type_def->getLength();
     if (!target_type)

 
     lldb_private::Type *element_type =
         m_ast.GetSymbolFile()->ResolveTypeUID(element_uid);
+    if (!element_type)
+      return nullptr;
     CompilerType element_ast_type = element_type->GetFullCompilerType();
     CompilerType array_ast_type =
         m_ast.CreateArrayType(element_ast_type, num_elements, false);

source/Plugins/SymbolFile/PDB/SymbolFilePDB.h

       const llvm::pdb::PDBSymbolCompiland &cu,
       llvm::DenseMap<uint32_t, uint32_t> &index_map) const;
 
-  void FindTypesByRegex(const std::string &regex, uint32_t max_matches,
+  void FindTypesByRegex(const lldb_private::RegularExpression &regex,
+                        uint32_t max_matches,
                         lldb_private::TypeMap &types);
 
   void FindTypesByName(const std::string &name, uint32_t max_matches,

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/TypeMap.h"
+#include "lldb/Utility/RegularExpression.h"
 
 #include "llvm/DebugInfo/PDB/GenericError.h"
 #include "llvm/DebugInfo/PDB/IPDBEnumChildren.h"

     return nullptr;
 
   lldb::TypeSP result = pdb->CreateLLDBTypeFromPDBType(*pdb_type);
-  m_types.insert(std::make_pair(type_uid, result));
+  if (result.get())
+    m_types.insert(std::make_pair(type_uid, result));
   return result.get();
 }
 

 
   std::string name_str = name.AsCString();
 
-  // If this might be a regex, we have to return EVERY symbol and process them
-  // one by one, which is going to destroy performance on large PDB files.  So
-  // try really hard not to use a regex match.
-  if (name_str.find_first_of("[]?*.-+\\") != std::string::npos)
-    FindTypesByRegex(name_str, max_matches, types);
-  else
-    FindTypesByName(name_str, max_matches, types);
+  // There is an assumption 'name' is not a regex
+  FindTypesByName(name_str, max_matches, types);
+   
   return types.GetSize();
 }
 
-void SymbolFilePDB::FindTypesByRegex(const std::string &regex,
-                                     uint32_t max_matches,
-                                     lldb_private::TypeMap &types) {
+void
+SymbolFilePDB::FindTypesByRegex(const lldb_private::RegularExpression &regex,
+                                uint32_t max_matches,
+                                lldb_private::TypeMap &types) {
   // When searching by regex, we need to go out of our way to limit the search
   // space as much as possible since this searches EVERYTHING in the PDB,
   // manually doing regex comparisons.  PDB library isn't optimized for regex

   auto global = m_session_up->getGlobalScope();
   std::unique_ptr<IPDBEnumSymbols> results;
 
-  std::regex re(regex);
-
   uint32_t matches = 0;
 
   for (auto tag : tags_to_search) {

         continue;
       }
 
-      if (!std::regex_match(type_name, re))
+      if (!regex.Execute(type_name))
         continue;
 
       // This should cause the type to get cached and stored in the `m_types`

unittests/SymbolFile/PDB/SymbolFilePDBTests.cpp

                                             false, 0, searched_files, results);
   EXPECT_GT(num_results, 1u);
   EXPECT_EQ(num_results, results.GetSize());
+
+  // We expect no exception thrown if the given regex can't be compiled
+  results.Clear();
+  num_results = symfile->FindTypes(sc, ConstString("**"), nullptr,
+                                   false, 0, searched_files, results);
+  EXPECT_EQ(num_results, 0u);
+  EXPECT_EQ(num_results, results.GetSize());
 }
 
 TEST_F(SymbolFilePDBTests, TestMaxMatches) {