[NFC] Add a test for the inferior memory cache (mainly L1)

[jarin]

Summary

This patch adds a test for L1 of the inferior's memory cache and makes the cache testable. This is mostly in preparation for an L1 flushing bug fix and its regression test (context: https://reviews.llvm.org/D77765).

[labath]

Thanks for splitting this up. This looks fine to me, modulo some nits, but lets wait @clayborg has to say.

lldb/source/Target/Memory.cpp

63

I guess this is a leftover from splitting the patches?

Speaking of post-increment the llvm rule is to use pre-increment whereever possible. I see the test uses post-increment exclusively for no good reason.

lldb/unittests/Target/MemoryCacheTest.cpp

31

You can move setting of m_inferior_read_count into the initializer.

[jarin]

Addressed reviewer comments.

[jarin]

lldb/source/Target/Memory.cpp

63

But, but,... I only increment/decrement in statement position!

Just kidding, I am sorry, fixed now...

[clayborg]

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.

lldb/source/Target/Memory.cpp

31–38

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);

lldb/source/Target/Process.cpp

614

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.

1497

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.

5615

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.

[jarin]

Regarding the callback idea, I have bad experience with callbacks because they break if the code is not crafted for re-entrancy and they are much harder to understand. That change feels out of scope for just adding a test and fixing an unrelated bug.

Adding the SetCacheLineSize method sounds good, but if we want to keep this patch making NFC (which is what I would prefer), it would have to be called at exactly the same places as Clear. How about Pavel's idea to rename Clear -> Reset, and leave the refactoring to SetCacheLineSize for later?

It appears it is really hard to reach agreement about this, so another alternative is I submit a bug report about the L1 invalidation problem and leave it to you to figure this out. In the mean time, we will fix the bug only in our private fork of lldb. Greg, perhaps you would prefer that?

[clayborg]

In D77790#1974047, @jarin wrote:

Regarding the callback idea, I have bad experience with callbacks because they break if the code is not crafted for re-entrancy and they are much harder to understand. That change feels out of scope for just adding a test and fixing an unrelated bug.

The code is already there and working. Not sure what the worry about re-entrancy is from?

Adding the SetCacheLineSize method sounds good, but if we want to keep this patch making NFC (which is what I would prefer), it would have to be called at exactly the same places as Clear. How about Pavel's idea to rename Clear -> Reset, and leave the refactoring to SetCacheLineSize for later?

We are changing the code so that just to make it NFC we have to check it in some half baked state? I don't agree with this. Clear() didn't take a parameter before, nor should it have to now.

It appears it is really hard to reach agreement about this, so another alternative is I submit a bug report about the L1 invalidation problem and leave it to you to figure this out. In the mean time, we will fix the bug only in our private fork of lldb. Greg, perhaps you would prefer that?

I don't see this as really that hard to fix correctly. Feel free to do what you need to if this is too much.

[jarin]

In D77790#1975324, @clayborg wrote:

In D77790#1974047, @jarin wrote:

It appears it is really hard to reach agreement about this, so another alternative is I submit a bug report about the L1 invalidation problem and leave it to you to figure this out. In the mean time, we will fix the bug only in our private fork of lldb. Greg, perhaps you would prefer that?

I don't see this as really that hard to fix correctly. Feel free to do what you need to if this is too much.

Ack, abandoning.

lldb/include/lldb/Target/Memory.h

 
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/lldb-private.h"
 #include <map>
 #include <mutex>
 #include <vector>
 
 namespace lldb_private {
+
+// Abstraction of a reader from inferior's memory.
+class MemoryFromInferiorReader {
+public:
+  /// Read of memory from a process.
+  ///
+  /// This function has the same semantics of ReadMemory except that it
+  /// bypasses caching.
+  ///
+  /// \param[in] vm_addr
+  ///     A virtual load address that indicates where to start reading
+  ///     memory from.
+  ///
+  /// \param[out] buf
+  ///     A byte buffer that is at least \a size bytes long that
+  ///     will receive the memory bytes.
+  ///
+  /// \param[in] size
+  ///     The number of bytes to read.
+  ///
+  /// \param[out] error
+  ///     An error that indicates the success or failure of this
+  ///     operation. If error indicates success (error.Success()),
+  ///     then the value returned can be trusted, otherwise zero
+  ///     will be returned.
+  ///
+  /// \return
+  ///     The number of bytes that were actually read into \a buf. If
+  ///     the returned number is greater than zero, yet less than \a
+  ///     size, then this function will get called again with \a
+  ///     vm_addr, \a buf, and \a size updated appropriately. Zero is
+  ///     returned in the case of an error.
+  virtual size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf,
+                                        size_t size, Status &error) = 0;
+};
+
 // A class to track memory that was read from a live process between
 // runs.
 class MemoryCache {
 public:
   // Constructors and Destructors
-  MemoryCache(Process &process);
+  MemoryCache(MemoryFromInferiorReader &reader, uint64_t cache_line_size);
 
   ~MemoryCache();
 
-  void Clear(bool clear_invalid_ranges = false);
+  void Clear(uint64_t cache_line_size, bool clear_invalid_ranges = false);
 
   void Flush(lldb::addr_t addr, size_t size);
 
   size_t Read(lldb::addr_t addr, void *dst, size_t dst_len, Status &error);
 
   uint32_t GetMemoryCacheLineSize() const { return m_L2_cache_line_byte_size; }
 
   void AddInvalidRange(lldb::addr_t base_addr, lldb::addr_t byte_size);
protected:
   typedef std::map<lldb::addr_t, lldb::DataBufferSP> BlockMap;
   typedef RangeVector<lldb::addr_t, lldb::addr_t, 4> InvalidRanges;
   typedef Range<lldb::addr_t, lldb::addr_t> AddrRange;
   // Classes that inherit from MemoryCache can see and modify these
   std::recursive_mutex m_mutex;
   BlockMap m_L1_cache; // A first level memory cache whose chunk sizes vary that
                        // will be used only if the memory read fits entirely in
                        // a chunk
-  BlockMap m_L2_cache; // A memory cache of fixed size chinks
+  BlockMap m_L2_cache; // A memory cache of fixed size chunks
                        // (m_L2_cache_line_byte_size bytes in size each)
   InvalidRanges m_invalid_ranges;
-  Process &m_process;
+  MemoryFromInferiorReader &m_reader;
   uint32_t m_L2_cache_line_byte_size;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(MemoryCache);
 };
 
     
 

lldb/include/lldb/Target/Process.h

 
 /// \class Process Process.h "lldb/Target/Process.h"
 /// A plug-in interface definition class for debugging a process.
 class Process : public std::enable_shared_from_this<Process>,
                 public ProcessProperties,
                 public UserID,
                 public Broadcaster,
                 public ExecutionContextScope,
-                public PluginInterface {
+                public PluginInterface,
+                public MemoryFromInferiorReader {
   friend class FunctionCaller; // For WaitForStateChangeEventsPrivate
   friend class Debugger; // For PopProcessIOHandler and ProcessIOHandlerIsActive
   friend class DynamicLoader; // For LoadOperatingSystemPlugin
   friend class ProcessEventData;
   friend class StopInfo;
   friend class Target;
   friend class ThreadList;
 
public:
   ///
   /// \return
   ///     The number of bytes that were actually read into \a buf. If
   ///     the returned number is greater than zero, yet less than \a
   ///     size, then this function will get called again with \a
   ///     vm_addr, \a buf, and \a size updated appropriately. Zero is
   ///     returned in the case of an error.
   size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf, size_t size,
-                                Status &error);
+                                Status &error) override;
 
   /// Read a NULL terminated string from memory
   ///
   /// This function will read a cache page at a time until a NULL string
   /// terminator is found. It will stop reading if an aligned sequence of NULL
   /// termination \a type_width bytes is not found before reading \a
   /// cstr_max_len bytes.  The results are always guaranteed to be NULL
   /// terminated, and that no more than (max_bytes - type_width) bytes will be

lldb/source/Target/Memory.cpp

 
 #include <cinttypes>
 #include <memory>
 
 using namespace lldb;
 using namespace lldb_private;
 
 // MemoryCache constructor
-MemoryCache::MemoryCache(Process &process)
+MemoryCache::MemoryCache(MemoryFromInferiorReader &reader,
+                         uint64_t cache_line_size)
     : m_mutex(), m_L1_cache(), m_L2_cache(), m_invalid_ranges(),
-      m_process(process),
-      m_L2_cache_line_byte_size(process.GetMemoryCacheLineSize()) {}
+      m_reader(reader), m_L2_cache_line_byte_size(cache_line_size) {}
 
 // Destructor
 MemoryCache::~MemoryCache() {}
 
-void MemoryCache::Clear(bool clear_invalid_ranges) {
+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 = m_process.GetMemoryCacheLineSize();
+  m_L2_cache_line_byte_size = cache_line_size;
 }
 
 void MemoryCache::AddL1CacheData(lldb::addr_t addr, const void *src,
                                  size_t src_len) {
   AddL1CacheData(
       addr, DataBufferSP(new DataBufferHeap(DataBufferHeap(src, src_len))));
 }
 
 void MemoryCache::AddL1CacheData(lldb::addr_t addr,
                                  const DataBufferSP &data_buffer_sp) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   m_L1_cache[addr] = data_buffer_sp;
 }
 
 void MemoryCache::Flush(addr_t addr, size_t size) {
   if (size == 0)
     return;
 
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
 
   // Erase any blocks from the L1 cache that intersect with the flush range
   if (!m_L1_cache.empty()) {
     AddrRange flush_range(addr, size);
     BlockMap::iterator pos = m_L1_cache.upper_bound(addr);
     if (pos != m_L1_cache.begin()) {
       --pos;
     }
     while (pos != m_L1_cache.end()) {
       AddrRange chunk_range(pos->first, pos->second->GetByteSize());
       if (!chunk_range.DoesIntersect(flush_range))
         break;
       pos = m_L1_cache.erase(pos);
     }
   }
size_t MemoryCache::Read(addr_t addr, void *dst, size_t dst_len,
   // (1) try to read as much of it at once as possible, and (2) don't add the
   // data to the memory cache.  We don't want to split a big read up into more
   // separate reads than necessary, and with a large memory read request, it is
   // unlikely that the caller function will ask for the next
   // 4 bytes after the large memory read - so there's little benefit to saving
   // it in the cache.
   if (dst && dst_len > m_L2_cache_line_byte_size) {
     size_t bytes_read =
-        m_process.ReadMemoryFromInferior(addr, dst, dst_len, error);
+        m_reader.ReadMemoryFromInferior(addr, dst, dst_len, error);
     // Add this non block sized range to the L1 cache if we actually read
     // anything
     if (bytes_read > 0)
       AddL1CacheData(addr, dst, bytes_read);
     return bytes_read;
   }
 
   if (dst && bytes_left > 0) {
while (bytes_left > 0) {
             // an entire page. If this happens, we must cap off how much data
             // we are able to read...
             if (pos->second->GetByteSize() != cache_line_byte_size)
               return dst_len - bytes_left;
           }
         }
       }
 
-      // We need to read from the process
+      // We need to read from the inferior
 
       if (bytes_left > 0) {
         assert((curr_addr % cache_line_byte_size) == 0);
         std::unique_ptr<DataBufferHeap> data_buffer_heap_up(
             new DataBufferHeap(cache_line_byte_size, 0));
-        size_t process_bytes_read = m_process.ReadMemoryFromInferior(
+        size_t inferior_bytes_read = m_reader.ReadMemoryFromInferior(
             curr_addr, data_buffer_heap_up->GetBytes(),
             data_buffer_heap_up->GetByteSize(), error);
-        if (process_bytes_read == 0)
+        if (inferior_bytes_read == 0)
           return dst_len - bytes_left;
 
-        if (process_bytes_read != cache_line_byte_size) {
-          if (process_bytes_read < data_buffer_heap_up->GetByteSize()) {
-            dst_len -= data_buffer_heap_up->GetByteSize() - process_bytes_read;
-            bytes_left = process_bytes_read;
+        if (inferior_bytes_read != cache_line_byte_size) {
+          if (inferior_bytes_read < data_buffer_heap_up->GetByteSize()) {
+            dst_len -= data_buffer_heap_up->GetByteSize() - inferior_bytes_read;
+            bytes_left = inferior_bytes_read;
           }
-          data_buffer_heap_up->SetByteSize(process_bytes_read);
+          data_buffer_heap_up->SetByteSize(inferior_bytes_read);
         }
         m_L2_cache[curr_addr] = DataBufferSP(data_buffer_heap_up.release());
         // We have read data and put it into the cache, continue through the
         // loop again to get the data out of the cache...
       }
     }
   }
 

lldb/source/Target/Process.cpp

: ProcessProperties(this), UserID(LLDB_INVALID_PROCESS_ID),
       m_thread_list(this), m_thread_plans(*this), m_extended_thread_list(this),
       m_extended_thread_stop_id(0), m_queue_list(this), m_queue_list_stop_id(0),
       m_notifications(), m_image_tokens(), m_listener_sp(listener_sp),
       m_breakpoint_site_list(), m_dynamic_checkers_up(),
       m_unix_signals_sp(unix_signals_sp), m_abi_sp(), m_process_input_reader(),
       m_stdio_communication("process.stdio"), m_stdio_communication_mutex(),
       m_stdin_forward(false), m_stdout_data(), m_stderr_data(),
       m_profile_data_comm_mutex(), m_profile_data(), m_iohandler_sync(0),
-      m_memory_cache(*this), m_allocated_memory_cache(*this),
-      m_should_detach(false), m_next_event_action_up(), m_public_run_lock(),
-      m_private_run_lock(), m_finalizing(false), m_finalize_called(false),
+      m_memory_cache(*this, GetMemoryCacheLineSize()),
+      m_allocated_memory_cache(*this), m_should_detach(false),
+      m_next_event_action_up(), m_public_run_lock(), m_private_run_lock(),
+      m_finalizing(false), m_finalize_called(false),
       m_clear_thread_plans_on_stop(false), m_force_next_event_delivery(false),
       m_last_broadcast_state(eStateInvalid), m_destroy_in_process(false),
       m_can_interpret_function_calls(false), m_warnings_issued(),
       m_run_thread_plan_lock(), m_can_jit(eCanJITDontKnow) {
   CheckInWithManager();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
   LLDB_LOGF(log, "%p Process::Process()", static_cast<void *>(this));
void Process::Finalize() {
   m_thread_list_real.Destroy();
   m_thread_list.Destroy();
   m_extended_thread_list.Destroy();
   m_queue_list.Clear();
   m_queue_list_stop_id = 0;
   std::vector<Notifications> empty_notifications;
   m_notifications.swap(empty_notifications);
   m_image_tokens.clear();
-  m_memory_cache.Clear();
+  m_memory_cache.Clear(GetMemoryCacheLineSize());
   m_allocated_memory_cache.Clear();
   {
     std::lock_guard<std::recursive_mutex> guard(m_language_runtimes_mutex);
     m_language_runtimes.clear();
   }
   m_instrumentation_runtimes.clear();
   m_next_event_action_up.reset();
   // Clear the last natural stop ID since it has a strong reference to this
if (StateIsStoppedState(new_state, false)) {
       // The process plugin is responsible for managing the actual behavior of
       // the threads and should have stopped any threads that are going to stop
       // before we get here.
       m_thread_list.DidStop();
 
       m_mod_id.BumpStopID();
       if (!m_mod_id.IsLastResumeForUserExpression())
         m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
-      m_memory_cache.Clear();
+      m_memory_cache.Clear(GetMemoryCacheLineSize());
       LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",
                 StateAsCString(new_state), m_mod_id.GetStopID());
     }
 
     // Use our target to get a shared pointer to ourselves...
     if (m_finalize_called && !PrivateStateThreadIsValid())
       BroadcastEvent(event_sp);
     else
void Process::DidExec() {
   m_image_tokens.clear();
   m_allocated_memory_cache.Clear();
   {
     std::lock_guard<std::recursive_mutex> guard(m_language_runtimes_mutex);
     m_language_runtimes.clear();
   }
   m_instrumentation_runtimes.clear();
   m_thread_list.DiscardThreadPlans();
-  m_memory_cache.Clear(true);
+  m_memory_cache.Clear(GetMemoryCacheLineSize(), true);
   DoDidExec();
   CompleteAttach();
   // Flush the process (threads and all stack frames) after running
   // CompleteAttach() in case the dynamic loader loaded things in new
   // locations.
   Flush();
 
   // After we figure out what was loaded/unloaded in CompleteAttach, we need to

lldb/unittests/Target/CMakeLists.txt

 add_lldb_unittest(TargetTests
   ABITest.cpp
   ExecutionContextTest.cpp
+  MemoryCacheTest.cpp
   MemoryRegionInfoTest.cpp
   ModuleCacheTest.cpp
   PathMappingListTest.cpp
   StackFrameRecognizerTest.cpp
 
   LINK_LIBS
       lldbCore
       lldbHost

lldb/unittests/Target/MemoryCacheTest.cpp

+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Target/Memory.h"
+#include "lldb/Utility/Status.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+namespace {
+
+class MemoryCacheTest : public MemoryFromInferiorReader, public testing::Test {
+  static const size_t k_memory_size = 256;
+  static const uint64_t k_cache_line_size = 16;
+
+  size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf, size_t size,
+                                Status &error) override {
+    ++m_inferior_read_count;
+
+    if (vm_addr >= m_memory.size())
+      return 0;
+
+    size = std::min(size, m_memory.size() - vm_addr);
+    memcpy(buf, m_memory.data() + vm_addr, size);
+    return size;
+  }
+
+public:
+  MemoryCacheTest() : m_cache(*this, k_cache_line_size) {
+    // Fill memory from [0x0 - 0x256) with byte values that match the index. We
+    // will use this memory in each test and each test will start with a fresh
+    // copy.
+    for (size_t i = 0; i < k_memory_size; ++i)
+      m_memory.push_back(static_cast<uint8_t>(i));
+  }
+
+  void AddRangeToL1Cache(lldb::addr_t addr, size_t len) {
+    m_cache.AddL1CacheData(addr, m_memory.data() + addr, len);
+  }
+
+  void IncrementAndFlushRange(lldb::addr_t addr, size_t len) {
+    for (size_t i = 0; i < len; ++i)
+      ++m_memory[addr + i];
+    m_cache.Flush(addr, len);
+  }
+
+  uint8_t ReadByte(lldb::addr_t addr) {
+    uint8_t result;
+    Status error;
+    m_cache.Read(addr, &result, 1, error);
+    EXPECT_TRUE(error.Success());
+    return result;
+  }
+
+protected:
+  MemoryCache m_cache;
+  std::vector<uint8_t> m_memory;
+  size_t m_inferior_read_count = 0;
+};
+
+TEST_F(MemoryCacheTest, L1FlushSimple) {
+  // Add a byte to the cache.
+  AddRangeToL1Cache(10, 1);
+
+  // Sanity check - memory should agree with what ReadByte returns.
+  EXPECT_EQ(10, m_memory[10]);
+  EXPECT_EQ(m_memory[10], ReadByte(10));
+  // Check that we have hit the cache and not the inferior's memory.
+  EXPECT_EQ(0u, m_inferior_read_count);
+
+  IncrementAndFlushRange(10, 1);
+
+  // Check the memory is incremented and the real memory agrees with ReadByte's
+  // result.
+  EXPECT_EQ(11, m_memory[11]);
+  EXPECT_EQ(m_memory[10], ReadByte(10));
+}
+
+// Let us do a parametrized test that caches two ranges. In the test, we then
+// try to modify (and flush) various regions of memory and check that the cache
+// still returns the correct values from reads.
+using AddrRange = Range<lldb::addr_t, lldb::addr_t>;
+class MemoryCacheParametrizedTest
+    : public MemoryCacheTest,
+      public testing::WithParamInterface<AddrRange> {};
+
+TEST_P(MemoryCacheParametrizedTest, L1FlushWithTwoRegions) {
+  // Add two ranges to the cache.
+  std::vector<AddrRange> cached = {{10, 10}, {30, 10}};
+  for (AddrRange cached_range : cached)
+    AddRangeToL1Cache(cached_range.base, cached_range.size);
+
+  // Flush the range given by the parameter.
+  AddrRange flush_range = GetParam();
+  IncrementAndFlushRange(flush_range.base, flush_range.size);
+
+  // Check that reads from the cached ranges read the inferior's memory
+  // if and only if the flush range intersects with one of the cached ranges.
+  bool has_intersection = false;
+  for (AddrRange cached_range : cached) {
+    if (flush_range.DoesIntersect(cached_range))
+      has_intersection = true;
+
+    for (size_t i = 0; i < cached_range.size; ++i)
+      ReadByte(cached_range.base + i);
+  }
+  EXPECT_EQ(has_intersection, m_inferior_read_count > 0);
+
+  // Sanity check: check the memory contents.
+  for (size_t i = 0; i < 50; ++i) {
+    EXPECT_EQ(m_memory[i], ReadByte(i)) << " (element at addr=" << i << ")";
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(
+    AllMemoryCacheParametrizedTest, MemoryCacheParametrizedTest,
+    ::testing::Values(AddrRange(5, 6), AddrRange(5, 10), AddrRange(5, 20),
+                      AddrRange(5, 30), AddrRange(5, 40), AddrRange(10, 1),
+                      AddrRange(10, 21), AddrRange(19, 1), AddrRange(19, 11),
+                      AddrRange(19, 12), AddrRange(30, 1), AddrRange(39, 1),
+                      AddrRange(39, 5), AddrRange(5, 1), AddrRange(5, 5),
+                      AddrRange(9, 1), AddrRange(20, 1), AddrRange(20, 10),
+                      AddrRange(29, 1), AddrRange(40, 1), AddrRange(40, 10)));
+
+} // namespace