addressing comments

src/core/arm/cache/cache.cpp

@@ -2,7 +2,6 @@
 // Licensed under GPLv2 or any later version
 // Refer to the license.txt file included.
 
-#include "common/assert.h"
 #include "core/arm/cache/cache.h"
 
 namespace Cache {
@@ -10,20 +9,20 @@ namespace Cache {
 static std::list<CacheBase*> caches;
 
 void RegisterCode(u32 address, u32 size) {
-    for (auto const& cache : caches) {
+    for (const auto& cache : caches) {
-        cache->OnCodeLoad(address, size);
+        cache->cache_fns.code_load(address, size);
     }
 }
 
 void UnregisterCode(u32 address, u32 size) {
-    for (auto const& cache : caches) {
+    for (const auto& cache : caches) {
-        cache->OnCodeUnload(address, size);
+        cache->cache_fns.code_unload(address, size);
     }
 }
 
 void ClearCache() {
-    for (auto const& cache : caches) {
+    for (const auto& cache : caches) {
-        cache->Clear();
+        cache->cache_fns.clear();
     }
 }
 
@@ -35,10 +34,7 @@ static void UnregisterCache(CacheBase* cache) {
     caches.erase(std::remove(caches.begin(), caches.end(), cache), caches.end());
 }
 
-CacheBase::CacheBase(bool index_mode, OnClearCb clearcb) : index_mode(index_mode) {
+CacheBase::CacheBase(functions fns) : cache_fns(fns) {
-    page_pointers.fill(nullptr);
-    Clear();
-    SetClearCallback(clearcb);
     RegisterCache(this);
 }
 
@@ -46,144 +42,4 @@ CacheBase::~CacheBase() {
     UnregisterCache(this);
 }
 
-void CacheBase::Clear() {
-    if (OnClearCallback != nullptr) {
-        OnClearCallback();
-    }
-
-    for (auto& cache : ptr_caches) {
-        cache.data.assign(cache.data.size(), nullptr);
-    }
-
-    if (index_mode) {
-        blocks_pc.assign(MAX_BLOCKS, INVALID_BLOCK);
-        next_block = num_blocks = 0;
-    }
-}
-
-bool CacheBase::RemoveBlock(u32 pc) {
-    void** ptr = page_pointers[pc >> Memory::PAGE_BITS];
-    if (ptr != nullptr) {
-        ptr = &ptr[pc & Memory::PAGE_MASK];
-
-        if (*ptr == nullptr) {
-            return false;
-        }
-
-        if (index_mode) {
-            const u32 id = pointer_to_id(*ptr);
-            ASSERT(blocks_pc[id] == pc);
-
-            blocks_pc[id] = INVALID_BLOCK;
-            if (id < next_block) {
-                next_block = id;
-            }
-            while (num_blocks > 0 && blocks_pc[num_blocks - 1] == INVALID_BLOCK) {
-                --num_blocks;
-            }
-        }
-        *ptr = nullptr;
-        return true;
-    }
-    return false;
-}
-
-bool CacheBase::RemoveRange(u32 start, u32 end) {
-    bool result = false;
-    for (auto& cache : ptr_caches) {
-        for (u32 i = std::max(start, cache.addr); i < std::min(end, cache.addr_end); ++i) {
-            void** ptr = &cache.data[i - cache.addr];
-
-            if (*ptr == nullptr) {
-                continue;
-            }
-
-            if (index_mode) {
-                const u32 id = pointer_to_id(*ptr);
-                ASSERT(blocks_pc[id] == i);
-
-                blocks_pc[id] = INVALID_BLOCK;
-                if (id < next_block) {
-                    next_block = id;
-                }
-                while (num_blocks > 0 && blocks_pc[num_blocks - 1] == INVALID_BLOCK) {
-                    --num_blocks;
-                }
-            }
-            *ptr = nullptr;
-            result = true;
-        }
-    }
-    return result;
-}
-
-void CacheBase::OnCodeLoad(u32 address, u32 size) {
-    const u32 end = address + size;
-
-    // Check there is no overlapping
-    for (auto const& cache : ptr_caches) {
-        ASSERT((address >= cache.addr_end) || (end <= cache.addr));
-    }
-
-    ASSERT((address & Memory::PAGE_MASK) == 0 && (size & Memory::PAGE_MASK) == 0);
-
-    BlockPtrCache cache{ address, address + size };
-    cache.data.assign(size, nullptr);
-
-    for (u32 i = address; i < end; i += Memory::PAGE_SIZE) {
-        page_pointers[i >> Memory::PAGE_BITS] = &cache.data[i - address];
-    }
-    ptr_caches.emplace_back(std::move(cache));
-}
-
-void CacheBase::OnCodeUnload(u32 address, u32 size) {
-    ptr_caches.erase(std::remove_if(ptr_caches.begin(), ptr_caches.end(),
-        [&, this](auto const& cache) {
-            if ((address < cache.addr_end) && (address + size > cache.addr)) {
-                this->RemoveRange(cache.addr, cache.addr_end);
-                for (u32 i = cache.addr; i < cache.addr_end; i += Memory::PAGE_SIZE) {
-                    page_pointers[i >> Memory::PAGE_BITS] = nullptr;
-                }
-                return true;
-            }
-            return false;
-        }),
-        ptr_caches.cend());
-}
-
-void*& CacheBase::GetNewPtr(u32 pc) {
-    DEBUG_ASSERT(!index_mode || next_block == MAX_BLOCKS || ((next_block < MAX_BLOCKS) && blocks_pc[next_block] == INVALID_BLOCK));
-    DEBUG_ASSERT(GetPtr(pc) == nullptr);
-
-    void** page_ptr = page_pointers[pc >> Memory::PAGE_BITS];
-    if (page_ptr == nullptr) {
-        // pc isnt within mapped code
-        OnCodeLoad(pc & ~Memory::PAGE_MASK, Memory::PAGE_SIZE);
-        page_ptr = page_pointers[pc >> Memory::PAGE_BITS];
-    }
-
-    void** block_ptr = &page_ptr[pc & Memory::PAGE_MASK];
-
-    DEBUG_ASSERT(*block_ptr == nullptr);
-
-    if (index_mode) {
-        if (next_block == MAX_BLOCKS) {
-            Clear();
-        }
-
-        blocks_pc[next_block] = pc;
-        *block_ptr = id_to_pointer(next_block);
-
-        do {
-            ++next_block;
-        } while (next_block <= num_blocks && blocks_pc[next_block] != INVALID_BLOCK);
-
-        if (next_block > num_blocks) {
-            num_blocks++;
-        }
-    }
-
-    return *block_ptr;
-}
-
 }

src/core/arm/cache/cache.h

@@ -27,19 +27,41 @@ void ClearCache();
 
 using OnClearCb = std::function<void()>;
 
-const u32 MAX_BLOCKS = 0x40000;
+constexpr u32 MAX_BLOCKS = 0x40000;
-const u32 INVALID_BLOCK = 0xFFFFFFFF;
+constexpr u32 INVALID_BLOCK = 0xFFFFFFFF;
 
+template <typename T>
 struct BlockPtrCache {
     u32 addr;
     u32 addr_end;
-    std::vector<void*> data;
+    std::vector<T> data;
 };
 
 class CacheBase {
+public:
+    struct functions {
+        std::function<void(u32, u32)> code_load;
+        std::function<void(u32, u32)> code_unload;
+        std::function<void()> clear;
+    } const cache_fns;
+
 protected:
-    explicit CacheBase(bool index_mode, OnClearCb clearcb);
+    explicit CacheBase(functions fns);
     ~CacheBase();
+};
+
+template <typename T>
+class CacheCommon : CacheBase {
+protected:
+    explicit CacheCommon(bool index_mode, OnClearCb clearcb) : index_mode(index_mode), CacheBase({
+            [this](u32 start, u32 size) { this->OnCodeLoad(start, size); },
+            [this](u32 start, u32 size) { this->OnCodeUnload(start, size); },
+            [this]() { this->Clear(); }}) {
+        static_assert(std::is_pointer<T>::value, "T must be a pointer");
+        Clear();
+        SetClearCallback(clearcb);
+    }
+    ~CacheCommon() {}
 
 public:
     /// Called when the cache needs to reset or Clear() is called
@@ -48,34 +70,165 @@ public:
     }
 
     /// Clear and call clear callback
-    void Clear();
+    void Clear() {
-    // returns true if block was found, false otherwise
+        if (OnClearCallback != nullptr) {
-    bool RemoveBlock(u32 pc);
+            OnClearCallback();
-    bool RemoveRange(u32 start, u32 end);
+        }
 
-    void OnCodeLoad(u32 address, u32 size);
+        for (auto& cache : ptr_caches) {
-    void OnCodeUnload(u32 address, u32 size);
+            cache.data.assign(cache.data.size(), nullptr);
+        }
+
+        if (index_mode) {
+            blocks_pc.assign(MAX_BLOCKS, INVALID_BLOCK);
+            next_block = num_blocks = 0;
+        }
+    }
+
+    /// Returns true if block was found, false otherwise
+    bool RemoveBlock(u32 pc) {
+        T* ptr = page_pointers[pc >> Memory::PAGE_BITS];
+        if (ptr != nullptr) {
+            ptr = &ptr[pc & Memory::PAGE_MASK];
+
+            if (*ptr == nullptr) {
+                return false;
+            }
+
+            if (index_mode) {
+                const u32 id = pointer_to_index(*ptr);
+                ASSERT(blocks_pc[id] == pc);
+
+                blocks_pc[id] = INVALID_BLOCK;
+                next_block = std::min(id, next_block);
+
+                while (num_blocks > 0 && blocks_pc[num_blocks - 1] == INVALID_BLOCK) {
+                    --num_blocks;
+                }
+            }
+            *ptr = nullptr;
+            return true;
+        }
+        return false;
+    }
+
+    /// Returns true if at least one block was found, false otherwise
+    bool RemoveRange(u32 start, u32 end) {
+        bool result = false;
+        for (auto& cache : ptr_caches) {
+            for (u32 i = std::max(start, cache.addr); i < std::min(end, cache.addr_end); ++i) {
+                T* ptr = &cache.data[i - cache.addr];
+
+                if (*ptr == nullptr) {
+                    continue;
+                }
+
+                if (index_mode) {
+                    const u32 id = pointer_to_index(*ptr);
+                    ASSERT(blocks_pc[id] == i);
+
+                    blocks_pc[id] = INVALID_BLOCK;
+                    next_block = std::min(id, next_block);
+
+                    while (num_blocks > 0 && blocks_pc[num_blocks - 1] == INVALID_BLOCK) {
+                        --num_blocks;
+                    }
+                }
+                *ptr = nullptr;
+                result = true;
+            }
+        }
+        return result;
+    }
+
+    void OnCodeLoad(u32 address, u32 size) {
+        // Check there is no overlapping
+        ASSERT(std::none_of(ptr_caches.cbegin(), ptr_caches.cend(),
+            [&](const auto& cache) {
+            return (address < cache.addr_end) && (address + size > cache.addr);
+        }));
+
+        ASSERT((address & Memory::PAGE_MASK) == 0 && (size & Memory::PAGE_MASK) == 0);
+
+        BlockPtrCache<T> cache{ address, address + size };
+        cache.data.assign(size, nullptr);
+
+        for (u32 i = address; i < address + size; i += Memory::PAGE_SIZE) {
+            page_pointers[i >> Memory::PAGE_BITS] = &cache.data[i - address];
+        }
+        ptr_caches.emplace_back(std::move(cache));
+    }
+
+    void OnCodeUnload(u32 address, u32 size) {
+        ptr_caches.erase(std::remove_if(ptr_caches.begin(), ptr_caches.end(),
+            [&, this](const auto& cache) {
+                if ((address < cache.addr_end) && (address + size > cache.addr)) {
+                    this->RemoveRange(cache.addr, cache.addr_end);
+                    for (u32 i = cache.addr; i < cache.addr_end; i += Memory::PAGE_SIZE) {
+                        page_pointers[i >> Memory::PAGE_BITS] = nullptr;
+                    }
+                    return true;
+                }
+                return false;
+            }),
+            ptr_caches.cend());
+    }
 
 protected:
-    void* GetPtr(u32 pc) const {
+    T GetPtr(u32 pc) const {
-        void** ptr = page_pointers[pc >> Memory::PAGE_BITS];
+        T* ptr = page_pointers[pc >> Memory::PAGE_BITS];
         if (ptr != nullptr) {
-            DEBUG_ASSERT(!index_mode || blocks_pc[pointer_to_id(ptr[pc & Memory::PAGE_MASK])] == pc);
+            const T value = ptr[pc & Memory::PAGE_MASK];
-            return ptr[pc & Memory::PAGE_MASK];
+            DEBUG_ASSERT(!index_mode || value == nullptr || blocks_pc[pointer_to_index(ptr[pc & Memory::PAGE_MASK])] == pc);
+            return value;
         }
         return nullptr;
     }
-    void*& GetNewPtr(u32 pc);
 
-    std::function<void*(u32)> id_to_pointer;
+    T* GetNewPtr(u32 pc) {
-    std::function<u32(void*)> pointer_to_id;
+        DEBUG_ASSERT(!index_mode || next_block == MAX_BLOCKS || ((next_block < MAX_BLOCKS) && blocks_pc[next_block] == INVALID_BLOCK));
+        DEBUG_ASSERT(GetPtr(pc) == nullptr);
+
+        T* page_ptr = page_pointers[pc >> Memory::PAGE_BITS];
+        if (page_ptr == nullptr) {
+            // pc isnt within mapped code
+            OnCodeLoad(pc & ~Memory::PAGE_MASK, Memory::PAGE_SIZE);
+            page_ptr = page_pointers[pc >> Memory::PAGE_BITS];
+        }
+
+        T* block_ptr = &page_ptr[pc & Memory::PAGE_MASK];
+
+        DEBUG_ASSERT(!index_mode || *block_ptr == nullptr);
+
+        if (index_mode) {
+            if (next_block == MAX_BLOCKS) {
+                Clear();
+            }
+
+            blocks_pc[next_block] = pc;
+            *block_ptr = index_to_pointer(next_block);
+
+            do {
+                ++next_block;
+            } while (next_block <= num_blocks && blocks_pc[next_block] != INVALID_BLOCK);
+
+            if (next_block > num_blocks) {
+                num_blocks++;
+            }
+        }
+
+        return block_ptr;
+    }
+
+    std::function<T(u32)> index_to_pointer;
+    std::function<u32(T)> pointer_to_index;
 
 private:
-    bool index_mode;
     OnClearCb OnClearCallback = nullptr;
+    const bool index_mode;
 
-    std::vector<BlockPtrCache> ptr_caches;
+    std::vector<BlockPtrCache<T>> ptr_caches;
-    std::array<void**, (1 << (32 - Memory::PAGE_BITS))> page_pointers;
+    std::array<T*, (1 << (32 - Memory::PAGE_BITS))> page_pointers = {};
 
     std::vector<u32> blocks_pc;
     u32 next_block = 0;
@@ -84,49 +237,52 @@ private:
 
 /// Use this if you only need to store a pointer
 template <typename T>
-class PtrCache final : public CacheBase {
+class PtrCache final : public CacheCommon<T> {
 public:
-    explicit PtrCache(OnClearCb clearcb = nullptr) : CacheBase(false, clearcb) {
+    explicit PtrCache(OnClearCb clearcb = nullptr) : CacheCommon<T>(false, clearcb) {
-        static_assert(std::is_pointer<T>::value, "T must be a pointer");
+        static_assert(std::is_pointer<T>::value, "T must be a pointer, use IndexedCache");
     }
     ~PtrCache() {}
 
     /// Get cached pointer for PC
     T GetPtr(u32 pc) {
-        return reinterpret_cast<T>(CacheBase::GetPtr(pc));
+        return CacheCommon<T>::GetPtr(pc);
     }
 
-    /// Get reference of pointer for PC
+    /// Set pointer for PC to value
-    T& GetNewPtr(u32 pc) {
+    T SetPtr(u32 pc, T value) {
-        return reinterpret_cast<T&>(CacheBase::GetNewPtr(pc));
+        return (*CacheCommon<T>::GetNewPtr(pc) = value);
     }
 };
 
 /// Index based cache
 template <typename T>
-class Cache final : public CacheBase {
+class IndexedCache final : public CacheCommon<T*> {
 public:
-    explicit Cache(OnClearCb clearcb = nullptr) : CacheBase(true, clearcb) {
+    explicit IndexedCache(OnClearCb clearcb = nullptr) : CacheCommon<T*>(true, clearcb) {
-        id_to_pointer = [this](u32 id) -> void* {
+
+        static_assert(!std::is_pointer<T>::value, "T can't be a pointer, use PtrCache");
+
+        CacheCommon<T*>::index_to_pointer = [this](u32 id) -> T* {
             return &blocks[id];
         };
-        pointer_to_id = [this](void* ptr) -> u32 {
+        CacheCommon<T*>::pointer_to_index = [this](T* ptr) -> u32 {
-            return static_cast<u32>(std::distance(blocks.begin(),
+            return static_cast<u32>(std::distance(blocks.cbegin(),
-                std::find_if(blocks.begin(), blocks.end(), [&](auto const& block) {
+                std::find_if(blocks.cbegin(), blocks.cend(), [&](const T& block) {
-                return (reinterpret_cast<T*>(ptr) == &block) ? true : false;
+                return (ptr == &block);
             })));
         };
     }
-    ~Cache() {}
+    ~IndexedCache() {}
 
     /// Get block cached for PC
     T* GetBlock(u32 pc) {
-        return reinterpret_cast<T*>(GetPtr(pc));
+        return CacheCommon<T*>::GetPtr(pc);
     }
 
     /// Allocate block for PC
-    T& GetNewBlock(u32 pc) {
+    T* GetNewBlock(u32 pc) {
-        return *reinterpret_cast<T*&>(GetNewPtr(pc));
+        return *CacheCommon<T*>::GetNewPtr(pc);
     }
 
 private:

src/core/arm/dyncom/arm_dyncom_interpreter.cpp

@@ -1133,7 +1133,7 @@ struct pkh_inst {
 typedef arm_inst * ARM_INST_PTR;
 
 #define CACHE_BUFFER_SIZE    (64 * 1024 * 2000)
-static char inst_buf[CACHE_BUFFER_SIZE];
+static u8 inst_buf[CACHE_BUFFER_SIZE];
 static int top = 0;
 static inline void *AllocBuffer(unsigned int size) {
     int start = top;
@@ -3900,7 +3900,7 @@ unsigned InterpreterMainLoop(ARMul_State* cpu) {
         // Find the cached instruction cream, otherwise translate it...
         ptr = instr_cache.GetPtr(cpu->Reg[15]);
         if (ptr == nullptr) {
-            ptr = instr_cache.GetNewPtr(cpu->Reg[15]) = reinterpret_cast<u8*>(&inst_buf[top]);
+            ptr = instr_cache.SetPtr(cpu->Reg[15], &inst_buf[top]);
             if (cpu->NumInstrsToExecute != 1) {
                 if (InterpreterTranslateBlock(cpu, cpu->Reg[15]) == FETCH_EXCEPTION)
                     goto END;