Refactor GLES routine cache keys to use Memset<T>
This ports the Vulkan side change to OpenGL ES and silences the GCC 8
class-memaccess warning/error, while ensuring proper key initialization.
Defines a Memset<T> class to be used as the first base class of cache
key types, which makes it explicit that their underlying memory will be
fully initialized before any member constructors are run.
In particular this fixes Blitter::Options state having uninitialized
bits after the bitfield, and Blitter::State having uninitialized padding
bytes after Options so 'sourceFormat' is 32-bit aligned.
Also adds is_memcmparable<T> for checking if memcmp() can be used to
implement operator==() for cache keys. It's equivalent to
std::is_trivially_copyable except it provides a fallback for STL
implementations that don't support it.
Also fix class-memset violations in LLVM 7.0 with their solution from
a later version.
Bug: b/135744933
Change-Id: Ic1e5c2c6b944a5133fc55840c0508bc2cdd1d66a
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/33488
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Chris Forbes <chrisforbes@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
diff --git a/src/Renderer/Blitter.hpp b/src/Renderer/Blitter.hpp
index 9c6b4c0..c546060 100644
--- a/src/Renderer/Blitter.hpp
+++ b/src/Renderer/Blitter.hpp
@@ -53,13 +53,14 @@
bool clampToEdge : 1;
};
- struct State : Options
+ struct State : Memset<State>, Options
{
- State() = default;
- State(const Options &options) : Options(options) {}
+ State() : Memset(this, 0) {}
+ State(const Options &options) : Memset(this, 0), Options(options) {}
bool operator==(const State &state) const
{
+ static_assert(is_memcmparable<State>::value, "Cannot memcmp State");
return memcmp(this, &state, sizeof(State)) == 0;
}
diff --git a/src/Renderer/LRUCache.hpp b/src/Renderer/LRUCache.hpp
index bdd0950..ebbf9b2 100644
--- a/src/Renderer/LRUCache.hpp
+++ b/src/Renderer/LRUCache.hpp
@@ -17,6 +17,9 @@
#include "Common/Math.hpp"
+#include <cstring>
+#include <type_traits>
+
namespace sw
{
template<class Key, class Data>
@@ -43,6 +46,46 @@
Key **ref;
Data *data;
};
+
+ // Helper class for clearing the memory of objects at construction.
+ // Useful as the first base class of cache keys which may contain padding bytes or bits otherwise left uninitialized.
+ template<class T>
+ struct Memset
+ {
+ Memset(T *object, int val)
+ {
+ static_assert(std::is_base_of<Memset<T>, T>::value, "Memset<T> must only clear the memory of a type of which it is a base class");
+
+ // GCC 8+ warns that
+ // "‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘T’;
+ // use assignment or value-initialization instead [-Werror=class-memaccess]"
+ // This is benign iff it happens before any of the base or member constructrs are called.
+ #if defined(__GNUC__) && (__GNUC__ >= 8)
+ #pragma GCC diagnostic push
+ #pragma GCC diagnostic ignored "-Wclass-memaccess"
+ #endif
+
+ memset(object, 0, sizeof(T));
+
+ #if defined(__GNUC__) && (__GNUC__ >= 8)
+ #pragma GCC diagnostic pop
+ #endif
+ }
+ };
+
+ // Traits-like helper class for checking if objects can be compared using memcmp().
+ // Useful for statically asserting if a cache key can implement operator==() with memcmp().
+ template<typename T>
+ struct is_memcmparable
+ {
+ // std::is_trivially_copyable is not available in older GCC versions.
+ #if !defined(__GNUC__) || __GNUC__ > 5
+ static const bool value = std::is_trivially_copyable<T>::value;
+ #else
+ // At least check it doesn't have virtual methods.
+ static const bool value = !std::is_polymorphic<T>::value;
+ #endif
+ };
}
namespace sw
diff --git a/src/Renderer/PixelProcessor.cpp b/src/Renderer/PixelProcessor.cpp
index 0b80727..c927d5d 100644
--- a/src/Renderer/PixelProcessor.cpp
+++ b/src/Renderer/PixelProcessor.cpp
@@ -22,7 +22,7 @@
#include "Shader/Constants.hpp"
#include "Common/Debug.hpp"
-#include <string.h>
+#include <cstring>
namespace sw
{
@@ -32,12 +32,12 @@
bool precachePixel = false;
- unsigned int PixelProcessor::States::computeHash()
+ uint32_t PixelProcessor::States::computeHash()
{
- unsigned int *state = (unsigned int*)this;
- unsigned int hash = 0;
+ uint32_t *state = reinterpret_cast<uint32_t*>(this);
+ uint32_t hash = 0;
- for(unsigned int i = 0; i < sizeof(States) / 4; i++)
+ for(unsigned int i = 0; i < sizeof(States) / sizeof(uint32_t); i++)
{
hash ^= state[i];
}
@@ -45,11 +45,6 @@
return hash;
}
- PixelProcessor::State::State()
- {
- memset(this, 0, sizeof(State));
- }
-
bool PixelProcessor::State::operator==(const State &state) const
{
if(hash != state.hash)
@@ -57,6 +52,7 @@
return false;
}
+ static_assert(is_memcmparable<State>::value, "Cannot memcmp State");
return memcmp(static_cast<const States*>(this), static_cast<const States*>(&state), sizeof(States)) == 0;
}
@@ -79,7 +75,7 @@
PixelProcessor::~PixelProcessor()
{
delete routineCache;
- routineCache = 0;
+ routineCache = nullptr;
}
void PixelProcessor::setFloatConstant(unsigned int index, const float value[4])
diff --git a/src/Renderer/PixelProcessor.hpp b/src/Renderer/PixelProcessor.hpp
index 4fa627c..b901ffe 100644
--- a/src/Renderer/PixelProcessor.hpp
+++ b/src/Renderer/PixelProcessor.hpp
@@ -28,9 +28,11 @@
class PixelProcessor
{
public:
- struct States
+ struct States : Memset<States>
{
- unsigned int computeHash();
+ States() : Memset(this, 0) {}
+
+ uint32_t computeHash();
int shaderID;
@@ -113,8 +115,6 @@
struct State : States
{
- State();
-
bool operator==(const State &state) const;
int colorWriteActive(int index) const
@@ -132,7 +132,7 @@
return pixelFogMode != FOG_NONE;
}
- unsigned int hash;
+ uint32_t hash;
};
struct Stencil
diff --git a/src/Renderer/SetupProcessor.cpp b/src/Renderer/SetupProcessor.cpp
index d8b9b91..f23068f 100644
--- a/src/Renderer/SetupProcessor.cpp
+++ b/src/Renderer/SetupProcessor.cpp
@@ -22,6 +22,8 @@
#include "Shader/Constants.hpp"
#include "Common/Debug.hpp"
+#include <cstring>
+
namespace sw
{
extern bool complementaryDepthBuffer;
@@ -29,12 +31,12 @@
bool precacheSetup = false;
- unsigned int SetupProcessor::States::computeHash()
+ uint32_t SetupProcessor::States::computeHash()
{
- unsigned int *state = (unsigned int*)this;
- unsigned int hash = 0;
+ uint32_t *state = reinterpret_cast<uint32_t*>(this);
+ uint32_t hash = 0;
- for(unsigned int i = 0; i < sizeof(States) / 4; i++)
+ for(unsigned int i = 0; i < sizeof(States) / sizeof(uint32_t); i++)
{
hash ^= state[i];
}
@@ -42,11 +44,6 @@
return hash;
}
- SetupProcessor::State::State(int i)
- {
- memset(this, 0, sizeof(State));
- }
-
bool SetupProcessor::State::operator==(const State &state) const
{
if(hash != state.hash)
@@ -54,19 +51,20 @@
return false;
}
+ static_assert(is_memcmparable<State>::value, "Cannot memcmp States");
return memcmp(static_cast<const States*>(this), static_cast<const States*>(&state), sizeof(States)) == 0;
}
SetupProcessor::SetupProcessor(Context *context) : context(context)
{
- routineCache = 0;
+ routineCache = nullptr;
setRoutineCacheSize(1024);
}
SetupProcessor::~SetupProcessor()
{
delete routineCache;
- routineCache = 0;
+ routineCache = nullptr;
}
SetupProcessor::State SetupProcessor::update() const
diff --git a/src/Renderer/SetupProcessor.hpp b/src/Renderer/SetupProcessor.hpp
index de12afd..4807e27 100644
--- a/src/Renderer/SetupProcessor.hpp
+++ b/src/Renderer/SetupProcessor.hpp
@@ -33,9 +33,11 @@
class SetupProcessor
{
public:
- struct States
+ struct States : Memset<States>
{
- unsigned int computeHash();
+ States() : Memset(this, 0) {}
+
+ uint32_t computeHash();
bool isDrawPoint : 1;
bool isDrawLine : 1;
@@ -76,11 +78,9 @@
struct State : States
{
- State(int i = 0);
-
bool operator==(const State &states) const;
- unsigned int hash;
+ uint32_t hash;
};
typedef bool (*RoutinePointer)(Primitive *primitive, const Triangle *triangle, const Polygon *polygon, const DrawData *draw);
diff --git a/src/Renderer/VertexProcessor.cpp b/src/Renderer/VertexProcessor.cpp
index 9bd786e..246e278 100644
--- a/src/Renderer/VertexProcessor.cpp
+++ b/src/Renderer/VertexProcessor.cpp
@@ -22,7 +22,7 @@
#include "Common/Math.hpp"
#include "Common/Debug.hpp"
-#include <string.h>
+#include <cstring>
namespace sw
{
@@ -36,12 +36,12 @@
}
}
- unsigned int VertexProcessor::States::computeHash()
+ uint32_t VertexProcessor::States::computeHash()
{
- unsigned int *state = (unsigned int*)this;
- unsigned int hash = 0;
+ uint32_t *state = reinterpret_cast<uint32_t*>(this);
+ uint32_t hash = 0;
- for(unsigned int i = 0; i < sizeof(States) / 4; i++)
+ for(unsigned int i = 0; i < sizeof(States) / sizeof(uint32_t); i++)
{
hash ^= state[i];
}
@@ -49,11 +49,6 @@
return hash;
}
- VertexProcessor::State::State()
- {
- memset(this, 0, sizeof(State));
- }
-
bool VertexProcessor::State::operator==(const State &state) const
{
if(hash != state.hash)
@@ -61,6 +56,7 @@
return false;
}
+ static_assert(is_memcmparable<State>::value, "Cannot memcmp States");
return memcmp(static_cast<const States*>(this), static_cast<const States*>(&state), sizeof(States)) == 0;
}
@@ -118,14 +114,14 @@
updateModelMatrix[i] = true;
}
- routineCache = 0;
+ routineCache = nullptr;
setRoutineCacheSize(1024);
}
VertexProcessor::~VertexProcessor()
{
delete routineCache;
- routineCache = 0;
+ routineCache = nullptr;
}
void VertexProcessor::setInputStream(int index, const Stream &stream)
diff --git a/src/Renderer/VertexProcessor.hpp b/src/Renderer/VertexProcessor.hpp
index 329bdac..8fdd65f 100644
--- a/src/Renderer/VertexProcessor.hpp
+++ b/src/Renderer/VertexProcessor.hpp
@@ -44,9 +44,11 @@
class VertexProcessor
{
public:
- struct States
+ struct States : Memset<States>
{
- unsigned int computeHash();
+ States() : Memset(this, 0) {}
+
+ uint32_t computeHash();
uint64_t shaderID;
@@ -140,11 +142,9 @@
struct State : States
{
- State();
-
bool operator==(const State &state) const;
- unsigned int hash;
+ uint32_t hash;
};
struct FixedFunction
diff --git a/src/Vulkan/VkDescriptorSetLayout.cpp b/src/Vulkan/VkDescriptorSetLayout.cpp
index e4d87ce..9f6eb96 100644
--- a/src/Vulkan/VkDescriptorSetLayout.cpp
+++ b/src/Vulkan/VkDescriptorSetLayout.cpp
@@ -259,13 +259,13 @@
{
if(newSampler)
{
- memcpy(&sampler, newSampler, sizeof(sampler));
+ memcpy(reinterpret_cast<void*>(&sampler), newSampler, sizeof(sampler));
}
else
{
// Descriptor ID's start at 1, allowing to detect descriptor update
// bugs by checking for 0. Also avoid reading random values.
- memset(&sampler, 0, sizeof(sampler));
+ memset(reinterpret_cast<void*>(&sampler), 0, sizeof(sampler));
}
}
diff --git a/src/WSI/VkSwapchainKHR.cpp b/src/WSI/VkSwapchainKHR.cpp
index 7c9ae2a..b038f20 100644
--- a/src/WSI/VkSwapchainKHR.cpp
+++ b/src/WSI/VkSwapchainKHR.cpp
@@ -31,7 +31,7 @@
imageCount(pCreateInfo->minImageCount),
retired(false)
{
- memset(images, 0, imageCount * sizeof(PresentImage));
+ memset(reinterpret_cast<void*>(images), 0, imageCount * sizeof(PresentImage));
}
void SwapchainKHR::destroy(const VkAllocationCallbacks *pAllocator)
diff --git a/third_party/llvm-7.0/llvm/include/llvm/ADT/SmallVector.h b/third_party/llvm-7.0/llvm/include/llvm/ADT/SmallVector.h
index acb4426..e4ddd12 100644
--- a/third_party/llvm-7.0/llvm/include/llvm/ADT/SmallVector.h
+++ b/third_party/llvm-7.0/llvm/include/llvm/ADT/SmallVector.h
@@ -299,7 +299,7 @@
// use memcpy here. Note that I and E are iterators and thus might be
// invalid for memcpy if they are equal.
if (I != E)
- memcpy(Dest, I, (E - I) * sizeof(T));
+ memcpy(reinterpret_cast<void *>(Dest), I, (E - I) * sizeof(T));
}
/// Double the size of the allocated memory, guaranteeing space for at
@@ -310,7 +310,7 @@
void push_back(const T &Elt) {
if (LLVM_UNLIKELY(this->size() >= this->capacity()))
this->grow();
- memcpy(this->end(), &Elt, sizeof(T));
+ memcpy(reinterpret_cast<void *>(this->end()), &Elt, sizeof(T));
this->set_size(this->size() + 1);
}
