Synchronize in ~ImageImplementation
The egl::Image destructor synchronizes with the threads accessing that image.
However, that is too late because by the time ~Image runs, ImageImplementation
has already been destructed - concurrently with the other threads running, i.e.
data race. In particular, since those threads access virtual member functions on
Image, they may end up calling the base class ones (which are pure) instead of
the derived class ones.
So make sure to synchronize in ~ImageImplementation instead.
Bug: swiftshader:62
Change-Id: I91240d1dbb45dd126c65d86f9aecf77833b4488d
Reviewed-on: https://swiftshader-review.googlesource.com/10029
Reviewed-by: Nicolas Capens <capn@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
Tested-by: Nicolas Capens <capn@google.com>
diff --git a/src/OpenGL/common/Image.cpp b/src/OpenGL/common/Image.cpp
index cfed057..61d91db 100644
--- a/src/OpenGL/common/Image.cpp
+++ b/src/OpenGL/common/Image.cpp
@@ -1189,7 +1189,11 @@
: Image(width, height, format, type, pitchP) {}
ImageImplementation(GLsizei width, GLsizei height, sw::Format internalFormat, int multiSampleDepth, bool lockable)
: Image(width, height, internalFormat, multiSampleDepth, lockable) {}
- ~ImageImplementation() override {}
+
+ ~ImageImplementation() override
+ {
+ sync(); // Wait for any threads that use this image to finish.
+ }
void *lockInternal(int x, int y, int z, sw::Lock lock, sw::Accessor client) override
{
@@ -1229,7 +1233,9 @@
Image::~Image()
{
- sync(); // Wait for any threads that use this image to finish.
+ // sync() must be called in the destructor of the most derived class to ensure their vtable isn't destroyed
+ // before all threads are done using this image. Image itself is abstract so it can't be the most derived.
+ ASSERT(isUnlocked());
if(parentTexture)
{
diff --git a/src/Renderer/Surface.cpp b/src/Renderer/Surface.cpp
index f29aaae..e964584 100644
--- a/src/Renderer/Surface.cpp
+++ b/src/Renderer/Surface.cpp
@@ -1301,9 +1301,7 @@
{
// sync() must be called before this destructor to ensure all locks have been released.
// We can't call it here because the parent resource may already have been destroyed.
- ASSERT(external.lock == LOCK_UNLOCKED);
- ASSERT(internal.lock == LOCK_UNLOCKED);
- ASSERT(stencil.lock == LOCK_UNLOCKED);
+ ASSERT(isUnlocked());
if(!hasParent)
{
@@ -1371,9 +1369,9 @@
void Surface::unlockExternal()
{
- resource->unlock();
-
external.unlockRect();
+
+ resource->unlock();
}
void *Surface::lockInternal(int x, int y, int z, Lock lock, Accessor client)
@@ -1455,9 +1453,9 @@
void Surface::unlockInternal()
{
- resource->unlock();
-
internal.unlockRect();
+
+ resource->unlock();
}
void *Surface::lockStencil(int x, int y, int front, Accessor client)
@@ -1474,9 +1472,9 @@
void Surface::unlockStencil()
{
- resource->unlock();
-
stencil.unlockRect();
+
+ resource->unlock();
}
int Surface::bytes(Format format)
diff --git a/src/Renderer/Surface.hpp b/src/Renderer/Surface.hpp
index 16ff78c..b54565e 100644
--- a/src/Renderer/Surface.hpp
+++ b/src/Renderer/Surface.hpp
@@ -293,7 +293,8 @@
inline int getStencilPitchB() const;
inline int getStencilSliceB() const;
- void sync(); // Wait for lock(s) to be released
+ void sync(); // Wait for lock(s) to be released.
+ inline bool isUnlocked() const; // Only reliable after sync().
inline int getMultiSampleCount() const;
inline int getSuperSampleCount() const;
@@ -608,6 +609,13 @@
return internal.depth > 4 ? internal.depth / 4 : 1;
}
+ bool Surface::isUnlocked() const
+ {
+ return external.lock == LOCK_UNLOCKED &&
+ internal.lock == LOCK_UNLOCKED &&
+ stencil.lock == LOCK_UNLOCKED;
+ }
+
bool Surface::isExternalDirty() const
{
return external.buffer && external.buffer != internal.buffer && external.dirty;
