Commit fc2b84d5 by Antoine Labour Committed by Nicolas Capens

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/10029Reviewed-by: 's avatarNicolas Capens <capn@google.com> Reviewed-by: 's avatarAlexis Hétu <sugoi@google.com> Tested-by: 's avatarNicolas Capens <capn@google.com>
parent 007c6c5c
...@@ -1189,7 +1189,11 @@ namespace egl ...@@ -1189,7 +1189,11 @@ namespace egl
: Image(width, height, format, type, pitchP) {} : Image(width, height, format, type, pitchP) {}
ImageImplementation(GLsizei width, GLsizei height, sw::Format internalFormat, int multiSampleDepth, bool lockable) ImageImplementation(GLsizei width, GLsizei height, sw::Format internalFormat, int multiSampleDepth, bool lockable)
: Image(width, height, internalFormat, multiSampleDepth, 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 void *lockInternal(int x, int y, int z, sw::Lock lock, sw::Accessor client) override
{ {
...@@ -1229,7 +1233,9 @@ namespace egl ...@@ -1229,7 +1233,9 @@ namespace egl
Image::~Image() 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) if(parentTexture)
{ {
......
...@@ -1301,9 +1301,7 @@ namespace sw ...@@ -1301,9 +1301,7 @@ namespace sw
{ {
// sync() must be called before this destructor to ensure all locks have been released. // 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. // We can't call it here because the parent resource may already have been destroyed.
ASSERT(external.lock == LOCK_UNLOCKED); ASSERT(isUnlocked());
ASSERT(internal.lock == LOCK_UNLOCKED);
ASSERT(stencil.lock == LOCK_UNLOCKED);
if(!hasParent) if(!hasParent)
{ {
...@@ -1371,9 +1369,9 @@ namespace sw ...@@ -1371,9 +1369,9 @@ namespace sw
void Surface::unlockExternal() void Surface::unlockExternal()
{ {
resource->unlock();
external.unlockRect(); external.unlockRect();
resource->unlock();
} }
void *Surface::lockInternal(int x, int y, int z, Lock lock, Accessor client) void *Surface::lockInternal(int x, int y, int z, Lock lock, Accessor client)
...@@ -1455,9 +1453,9 @@ namespace sw ...@@ -1455,9 +1453,9 @@ namespace sw
void Surface::unlockInternal() void Surface::unlockInternal()
{ {
resource->unlock();
internal.unlockRect(); internal.unlockRect();
resource->unlock();
} }
void *Surface::lockStencil(int x, int y, int front, Accessor client) void *Surface::lockStencil(int x, int y, int front, Accessor client)
...@@ -1474,9 +1472,9 @@ namespace sw ...@@ -1474,9 +1472,9 @@ namespace sw
void Surface::unlockStencil() void Surface::unlockStencil()
{ {
resource->unlock();
stencil.unlockRect(); stencil.unlockRect();
resource->unlock();
} }
int Surface::bytes(Format format) int Surface::bytes(Format format)
......
...@@ -293,7 +293,8 @@ namespace sw ...@@ -293,7 +293,8 @@ namespace sw
inline int getStencilPitchB() const; inline int getStencilPitchB() const;
inline int getStencilSliceB() 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 getMultiSampleCount() const;
inline int getSuperSampleCount() const; inline int getSuperSampleCount() const;
...@@ -608,6 +609,13 @@ namespace sw ...@@ -608,6 +609,13 @@ namespace sw
return internal.depth > 4 ? internal.depth / 4 : 1; 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 bool Surface::isExternalDirty() const
{ {
return external.buffer && external.buffer != internal.buffer && external.dirty; return external.buffer && external.buffer != internal.buffer && external.dirty;
......
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