Do not cache hardcoded formats in FormatManager

[aprantl]

Summary

The cache in FormatCache uses only a type name as key. The hardcoded formats, synthetic children, etc inspect an entire ValueObject to determine their eligibility, which isn't modelled in the cache. This leads to bugs such as the one in this patch (where two similarly named types in different files have different hardcoded summary providers). The problem is exaggerated in the Swift language plugin due to the language's dynamic nature.

rdar://problem/57756763

[aprantl]

Rebase.

[jingham]

This looks fine to me. So long as there wasn't much strict type-based matching done in the Hardcoded Summary providers, then this shouldn't cause the format matching to get slower.

[jingham]

And check the Accept checkbox...

lldb/include/lldb/DataFormatters/FormatManager.h

private:
   NamedSummariesMap m_named_summaries_map;
   TypeCategoryMap m_categories_map;
 
   ConstString m_default_category_name;
   ConstString m_system_category_name;
   ConstString m_vectortypes_category_name;
 
   template <typename ImplSP>
-  ImplSP GetCached(ValueObject &valobj, lldb::DynamicValueType use_dynamic);
+  ImplSP Get(ValueObject &valobj, lldb::DynamicValueType use_dynamic);
+  template <typename ImplSP> ImplSP GetCached(FormattersMatchData &match_data);
   template <typename ImplSP> ImplSP GetHardcoded(FormattersMatchData &);
 
   TypeCategoryMap &GetCategories() { return m_categories_map; }
 
   // These functions are meant to initialize formatters that are very low-
   // level/global in nature and do not naturally belong in any language. The
   // intent is that most formatters go in language-specific categories.
   // Eventually, the runtimes should also be allowed to vend their own

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-caching/Makefile

+C_SOURCES := a.c b.c
+
+include Makefile.rules

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-caching/TestDataFormatterCaching.py

+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class TestDataFormatterCaching(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        TestBase.setUp(self)
+
+    def test_with_run_command(self):
+        """
+        Test that hardcoded summary formatter matches aren't improperly cached.
+        """
+        self.build()
+        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(
+            self, 'break here', lldb.SBFileSpec('a.c'))
+        valobj = self.frame().FindVariable('f')
+        self.assertEqual(valobj.GetValue(), '4')
+        bkpt_b = target.BreakpointCreateBySourceRegex('break here',
+                                                      lldb.SBFileSpec('b.c'))
+        lldbutil.continue_to_breakpoint(process, bkpt_b)
+        valobj = self.frame().FindVariable('f4')
+        self.assertEqual(valobj.GetSummary(), '(1, 2, 3, 4)')

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-caching/a.c

+typedef float float4;
+
+int main() {
+  float4 f = 4.0f;
+  // break here
+  return a(); 
+}

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-caching/b.c

+typedef float float4 __attribute__((ext_vector_type(4)));
+void stop() {}
+int a() {
+  float4 f4 = {1, 2, 3, 4};
+  // break here
+  stop();
+  return 0;
+}

lldb/source/DataFormatters/FormatManager.cpp

for (lldb::LanguageType lang_type : match_data.GetCandidateLanguages()) {
     if (LanguageCategory *lang_category = GetCategoryForLanguage(lang_type)) {
       if (lang_category->GetHardcoded(*this, match_data, retval_sp))
         return retval_sp;
     }
   }
   return retval_sp;
 }
 
-template <typename ImplSP> ImplSP
-FormatManager::GetCached(ValueObject &valobj,
+template <typename ImplSP>
+ImplSP FormatManager::Get(ValueObject &valobj,
                           lldb::DynamicValueType use_dynamic) {
-  ImplSP retval_sp;
   FormattersMatchData match_data(valobj, use_dynamic);
+  if (ImplSP retval_sp = GetCached<ImplSP>(match_data))
+    return retval_sp;
+
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_DATAFORMATTERS));
+  LLDB_LOGF(log, "[%s] Search failed. Giving hardcoded a chance.", __FUNCTION__);
+  return GetHardcoded<ImplSP>(match_data);
+}
+
+template <typename ImplSP>
+ImplSP FormatManager::GetCached(FormattersMatchData &match_data) {
+  ImplSP retval_sp;
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_DATAFORMATTERS));
   if (match_data.GetTypeForCache()) {
     LLDB_LOGF(log, "\n\n[%s] Looking into cache for type %s", __FUNCTION__,
               match_data.GetTypeForCache().AsCString("<invalid>"));
     if (m_format_cache.Get(match_data.GetTypeForCache(), retval_sp)) {
       if (log) {
         LLDB_LOGF(log, "[%s] Cache search success. Returning.", __FUNCTION__);
         LLDB_LOGV(log, "Cache hits: {0} - Cache Misses: {1}",
for (lldb::LanguageType lang_type : match_data.GetCandidateLanguages()) {
           break;
       }
     }
     if (retval_sp) {
       LLDB_LOGF(log, "[%s] Language search success. Returning.", __FUNCTION__);
       return retval_sp;
     }
   }
-  if (!retval_sp) {
-    LLDB_LOGF(log, "[%s] Search failed. Giving hardcoded a chance.", __FUNCTION__);
-    retval_sp = GetHardcoded<ImplSP>(match_data);
-  }
 
   if (match_data.GetTypeForCache() && (!retval_sp || !retval_sp->NonCacheable())) {
     LLDB_LOGF(log, "[%s] Caching %p for type %s", __FUNCTION__,
               static_cast<void *>(retval_sp.get()),
               match_data.GetTypeForCache().AsCString("<invalid>"));
     m_format_cache.Set(match_data.GetTypeForCache(), retval_sp);
   }
   LLDB_LOGV(log, "Cache hits: {0} - Cache Misses: {1}",
             m_format_cache.GetCacheHits(), m_format_cache.GetCacheMisses());
   return retval_sp;
 }
 
 lldb::TypeFormatImplSP
 FormatManager::GetFormat(ValueObject &valobj,
                          lldb::DynamicValueType use_dynamic) {
-  return GetCached<lldb::TypeFormatImplSP>(valobj, use_dynamic);
+  return Get<lldb::TypeFormatImplSP>(valobj, use_dynamic);
 }
 
 lldb::TypeSummaryImplSP
 FormatManager::GetSummaryFormat(ValueObject &valobj,
                                 lldb::DynamicValueType use_dynamic) {
-  return GetCached<lldb::TypeSummaryImplSP>(valobj, use_dynamic);
+  return Get<lldb::TypeSummaryImplSP>(valobj, use_dynamic);
 }
 
 lldb::SyntheticChildrenSP
 FormatManager::GetSyntheticChildren(ValueObject &valobj,
                                     lldb::DynamicValueType use_dynamic) {
-  return GetCached<lldb::SyntheticChildrenSP>(valobj, use_dynamic);
+  return Get<lldb::SyntheticChildrenSP>(valobj, use_dynamic);
 }
 
 lldb::TypeValidatorImplSP
 FormatManager::GetValidator(ValueObject &valobj,
                             lldb::DynamicValueType use_dynamic) {
-  return GetCached<lldb::TypeValidatorImplSP>(valobj, use_dynamic);
+  return Get<lldb::TypeValidatorImplSP>(valobj, use_dynamic);
 }
 
 FormatManager::FormatManager()
     : m_last_revision(0), m_format_cache(), m_language_categories_mutex(),
       m_language_categories_map(), m_named_summaries_map(this),
       m_categories_map(this), m_default_category_name(ConstString("default")),
       m_system_category_name(ConstString("system")),
       m_vectortypes_category_name(ConstString("VectorTypes")) {