[Lldb-commits] [PATCH] D77790: [NFC] Add a test for the inferior memory cache (mainly L1)

Greg Clayton via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Apr 9 17:39:29 PDT 2020


clayborg added a comment.

Mostly good, but it seems weird to supply the cache line size when calling the MemoryCache::Clear() function. We also don't seem to be catching updates to the cache line size property and invalidating the cache when and only when the property is modified, but we seem to be relying on calls to Clear() to do this when we stop? It would be nice to clean this up before submitting just because this change forces this now out of place looking setting to be passed along. ProcessProperties::ProcessProperties() has a place where you can hook in and call a function when the property changes:

  ProcessProperties::ProcessProperties(lldb_private::Process *process)
      : Properties(),
        m_process(process) // Can be nullptr for global ProcessProperties
  {
    if (process == nullptr) {
      // Global process properties, set them up one time
      m_collection_sp =
          std::make_shared<ProcessOptionValueProperties>(ConstString("process"));
      m_collection_sp->Initialize(g_process_properties);
      m_collection_sp->AppendProperty(
          ConstString("thread"), ConstString("Settings specific to threads."),
          true, Thread::GetGlobalProperties()->GetValueProperties());
    } else {
      m_collection_sp = std::make_shared<ProcessOptionValueProperties>(
          Process::GetGlobalProperties().get());
      m_collection_sp->SetValueChangedCallback(
          ePropertyPythonOSPluginPath,
          [this] { m_process->LoadOperatingSystemPlugin(true); });
    }
  }

So we could add:

  m_collection_sp->SetValueChangedCallback(
      ePropertyMemCacheLineSize,
      [this] { m_process->UpdateCacheLineSize(); });

This function would then update the cache line size in the m_memory_cache variable.



================
Comment at: lldb/source/Target/Memory.cpp:31-38
+void MemoryCache::Clear(uint64_t cache_line_size, bool clear_invalid_ranges) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   m_L1_cache.clear();
   m_L2_cache.clear();
   if (clear_invalid_ranges)
     m_invalid_ranges.Clear();
+  m_L2_cache_line_byte_size = cache_line_size;
----------------
Do we call clear when the cache line size changes? Maybe we should just have a function that does this and only this?:

```
void MemoryCache::SetCacheLineSize(uint64_t size);
```



================
Comment at: lldb/source/Target/Process.cpp:614
   m_image_tokens.clear();
-  m_memory_cache.Clear();
+  m_memory_cache.Clear(GetMemoryCacheLineSize());
   m_allocated_memory_cache.Clear();
----------------
Seems weird to be passing the cache line size to the clear method. Here we clearly just want to clear out the cache and don't care about the cache line size.


================
Comment at: lldb/source/Target/Process.cpp:1497
         m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
-      m_memory_cache.Clear();
+      m_memory_cache.Clear(GetMemoryCacheLineSize());
       LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",
----------------
Seems weird to be passing the cache line size to the clear method. Here we clearly just want to clear out the cache and don't care about the cache line size.


================
Comment at: lldb/source/Target/Process.cpp:5615
   m_thread_list.DiscardThreadPlans();
-  m_memory_cache.Clear(true);
+  m_memory_cache.Clear(GetMemoryCacheLineSize(), true);
   DoDidExec();
----------------
Seems weird to be passing the cache line size to the clear method. Here we clearly just want to clear out the cache and don't care about the cache line size.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77790/new/

https://reviews.llvm.org/D77790





More information about the lldb-commits mailing list