Commit 1885f694 by Nicolas Capens Committed by Nicolas Capens

Fix handling Memset<> assignment and comparison

Previously the compiler would implicitly declare a copy constructor and assignment operator for classes derived from Memset<>, if they didn't have user-defined ones. This can cause them to have uninitialized padding bytes. By defining them ourselves using memcpy() we can ensure they're zero-initialized. Also define equality and less-than operators. The latter makes classes derived from Memset<> suitable as std::map<> keys. is_memcmparable<> now no longer works on classed derived from Memset<>, because it uses std::is_trivially_copyable<> which isn't true when a user-defined copy constructor or assignment operator is provided. Instead just rely on Memset<>'s new equality operators. Bug: b/131246679 Change-Id: I6e4963db8186955d8d3d3ef356fa42ef6a024c64 Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/42728 Presubmit-Ready: Nicolas Capens <nicolascapens@google.com> Reviewed-by: 's avatarAntonio Maiorano <amaiorano@google.com> Tested-by: 's avatarNicolas Capens <nicolascapens@google.com> Kokoro-Presubmit: kokoro <noreply+kokoro@google.com>
parent 088add4e
...@@ -89,12 +89,6 @@ class Blitter ...@@ -89,12 +89,6 @@ class Blitter
, destSamples(destSamples) , destSamples(destSamples)
{} {}
bool operator==(const State &state) const
{
static_assert(is_memcmparable<State>::value, "Cannot memcmp State");
return memcmp(this, &state, sizeof(State)) == 0;
}
vk::Format sourceFormat; vk::Format sourceFormat;
vk::Format destFormat; vk::Format destFormat;
int srcSamples = 0; int srcSamples = 0;
......
...@@ -73,19 +73,6 @@ private: ...@@ -73,19 +73,6 @@ private:
std::unordered_map<Key, Data, Hasher> constCache; std::unordered_map<Key, Data, Hasher> constCache;
}; };
// 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 } // namespace sw
namespace sw { namespace sw {
......
...@@ -18,10 +18,19 @@ ...@@ -18,10 +18,19 @@
#include <cstring> #include <cstring>
#include <type_traits> #include <type_traits>
// 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 constructors are called.
#if defined(__GNUC__) && (__GNUC__ >= 8)
# pragma GCC diagnostic push
# pragma GCC diagnostic ignored "-Wclass-memaccess"
#endif
namespace sw { namespace sw {
// Helper class for clearing the memory of objects at construction. // Memset<> is a helper class for clearing the memory of objects at construction.
// Useful as the first base class of cache keys which may contain padding // It is useful as the *first* base class of map keys which may contain padding
// bytes or bits otherwise left uninitialized. // bytes or bits otherwise left uninitialized.
template<class T> template<class T>
struct Memset struct Memset
...@@ -29,24 +38,49 @@ struct Memset ...@@ -29,24 +38,49 @@ struct Memset
Memset(T *object, int val) 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"); 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");
::memset(object, 0, sizeof(T));
}
// GCC 8+ warns that // Don't rely on the implicitly declared copy constructor and copy assignment operator.
// "‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘T’; // They can leave padding bytes uninitialized.
// use assignment or value-initialization instead [-Werror=class-memaccess]" Memset(const Memset &rhs)
// This is benign iff it happens before any of the base or member constructrs are called. {
#if defined(__GNUC__) && (__GNUC__ >= 8) ::memcpy(this, &rhs, sizeof(T));
# pragma GCC diagnostic push }
# pragma GCC diagnostic ignored "-Wclass-memaccess"
#endif
memset(object, 0, sizeof(T)); Memset &operator=(const Memset &rhs)
{
::memcpy(this, &rhs, sizeof(T));
return *this;
}
#if defined(__GNUC__) && (__GNUC__ >= 8) // The compiler won't declare an implicit move constructor and move assignment operator
# pragma GCC diagnostic pop // due to having a user-defined copy constructor and copy assignment operator. Delete
#endif // them for explicitness. We always want memcpy() being called.
Memset(const Memset &&rhs) = delete;
Memset &operator=(const Memset &&rhs) = delete;
friend bool operator==(const T &a, const T &b)
{
return ::memcmp(&a, &b, sizeof(T)) == 0;
}
friend bool operator!=(const T &a, const T &b)
{
return ::memcmp(&a, &b, sizeof(T)) != 0;
}
friend bool operator<(const T &a, const T &b)
{
return ::memcmp(&a, &b, sizeof(T)) < 0;
} }
}; };
} // namespace sw } // namespace sw
// Restore -Wclass-memaccess
#if defined(__GNUC__) && (__GNUC__ >= 8)
# pragma GCC diagnostic pop
#endif
#endif // sw_Memset_hpp #endif // sw_Memset_hpp
\ No newline at end of file
...@@ -44,8 +44,7 @@ bool PixelProcessor::State::operator==(const State &state) const ...@@ -44,8 +44,7 @@ bool PixelProcessor::State::operator==(const State &state) const
return false; return false;
} }
static_assert(is_memcmparable<State>::value, "Cannot memcmp State"); return *static_cast<const States *>(this) == static_cast<const States &>(state);
return memcmp(static_cast<const States *>(this), static_cast<const States *>(&state), sizeof(States)) == 0;
} }
PixelProcessor::PixelProcessor() PixelProcessor::PixelProcessor()
......
...@@ -47,8 +47,7 @@ bool SetupProcessor::State::operator==(const State &state) const ...@@ -47,8 +47,7 @@ bool SetupProcessor::State::operator==(const State &state) const
return false; return false;
} }
static_assert(is_memcmparable<State>::value, "Cannot memcmp States"); return *static_cast<const States *>(this) == static_cast<const States &>(state);
return memcmp(static_cast<const States *>(this), static_cast<const States *>(&state), sizeof(States)) == 0;
} }
SetupProcessor::SetupProcessor() SetupProcessor::SetupProcessor()
......
...@@ -51,8 +51,7 @@ bool VertexProcessor::State::operator==(const State &state) const ...@@ -51,8 +51,7 @@ bool VertexProcessor::State::operator==(const State &state) const
return false; return false;
} }
static_assert(is_memcmparable<State>::value, "Cannot memcmp States"); return *static_cast<const States *>(this) == static_cast<const States &>(state);
return memcmp(static_cast<const States *>(this), static_cast<const States *>(&state), sizeof(States)) == 0;
} }
VertexProcessor::VertexProcessor() VertexProcessor::VertexProcessor()
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment