Fix refcounting bugs where the sys refcount
could be corrupted during async type creation.
Change-Id: If42828e92990598b0cb5da81c82ea513f94725f2
Fix stack object deletion bug.
Change-Id: I2c723aa5ad15e0c99dc9cd0cfbc7db80bace172a
diff --git a/libs/rs/RenderScript.h b/libs/rs/RenderScript.h
index e63cc9b..d078d46 100644
--- a/libs/rs/RenderScript.h
+++ b/libs/rs/RenderScript.h
@@ -288,7 +288,7 @@
// Async commands for returning new IDS
-void * rsaTypeCreate(RsContext, RsElement, uint32_t dimCount,
+RsType rsaTypeCreate(RsContext, RsElement, uint32_t dimCount,
const RsDimension *dims, const uint32_t *vals);
diff --git a/libs/rs/rsAdapter.cpp b/libs/rs/rsAdapter.cpp
index ef69b75..1d1425c 100644
--- a/libs/rs/rsAdapter.cpp
+++ b/libs/rs/rsAdapter.cpp
@@ -27,15 +27,11 @@
Adapter1D::Adapter1D(Context *rsc) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
reset();
}
Adapter1D::Adapter1D(Context *rsc, Allocation *a) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
reset();
setAllocation(a);
}
@@ -76,7 +72,7 @@
void Adapter1D::serialize(OStream *stream) const
{
-
+
}
Adapter1D *Adapter1D::createFromStream(Context *rsc, IStream *stream)
@@ -145,15 +141,11 @@
Adapter2D::Adapter2D(Context *rsc) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
reset();
}
Adapter2D::Adapter2D(Context *rsc, Allocation *a) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
reset();
setAllocation(a);
}
@@ -200,7 +192,7 @@
void Adapter2D::serialize(OStream *stream) const
{
-
+
}
Adapter2D *Adapter2D::createFromStream(Context *rsc, IStream *stream)
diff --git a/libs/rs/rsAllocation.cpp b/libs/rs/rsAllocation.cpp
index 6748bb4..fc41a722 100644
--- a/libs/rs/rsAllocation.cpp
+++ b/libs/rs/rsAllocation.cpp
@@ -57,8 +57,6 @@
void Allocation::init(Context *rsc, const Type *type)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mPtr = NULL;
mCpuWrite = false;
@@ -478,7 +476,7 @@
uint32_t dataSize = stream->loadU32();
if(dataSize != type->getSizeBytes()) {
LOGE("failed to read allocation because numbytes written is not the same loaded type wants\n");
- delete type;
+ ObjectBase::checkDelete(type);
return NULL;
}
diff --git a/libs/rs/rsElement.cpp b/libs/rs/rsElement.cpp
index 9e6fbd5..dc021fc 100644
--- a/libs/rs/rsElement.cpp
+++ b/libs/rs/rsElement.cpp
@@ -30,8 +30,6 @@
Element::Element(Context *rsc) : ObjectBase(rsc)
{
mBits = 0;
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mFields = NULL;
mFieldCount = 0;
mHasReference = false;
@@ -140,7 +138,7 @@
for (uint32_t ct=0; ct < rsc->mStateElement.mElements.size(); ct++) {
Element *ee = rsc->mStateElement.mElements[ct];
if(ee->isEqual(elem)) {
- delete elem;
+ ObjectBase::checkDelete(elem);
ee->incUserRef();
return ee;
}
diff --git a/libs/rs/rsFont.cpp b/libs/rs/rsFont.cpp
index 633129a..d171a48 100644
--- a/libs/rs/rsFont.cpp
+++ b/libs/rs/rsFont.cpp
@@ -36,8 +36,6 @@
Font::Font(Context *rsc) : ObjectBase(rsc), mCachedGlyphs(NULL)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mInitialized = false;
mHasKerning = false;
mFace = NULL;
@@ -308,7 +306,7 @@
return newFont;
}
- delete newFont;
+ ObjectBase::checkDelete(newFont);
return NULL;
}
diff --git a/libs/rs/rsMesh.cpp b/libs/rs/rsMesh.cpp
index 8e43f24..761be93 100644
--- a/libs/rs/rsMesh.cpp
+++ b/libs/rs/rsMesh.cpp
@@ -33,8 +33,6 @@
Mesh::Mesh(Context *rsc) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mPrimitives = NULL;
mPrimitivesCount = 0;
mVertexBuffers = NULL;
diff --git a/libs/rs/rsObjectBase.cpp b/libs/rs/rsObjectBase.cpp
index 46b1750..724172e 100644
--- a/libs/rs/rsObjectBase.cpp
+++ b/libs/rs/rsObjectBase.cpp
@@ -22,6 +22,7 @@
#include "rsContextHostStub.h"
#endif
+
using namespace android;
using namespace android::renderscript;
@@ -34,101 +35,119 @@
mRSC = rsc;
mNext = NULL;
mPrev = NULL;
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
+
+#if RS_OBJECT_DEBUG
+ mStack.update(2);
+#endif
rsAssert(rsc);
add();
+ //LOGV("ObjectBase %p con", this);
}
ObjectBase::~ObjectBase()
{
//LOGV("~ObjectBase %p ref %i,%i", this, mUserRefCount, mSysRefCount);
+#if RS_OBJECT_DEBUG
+ mStack.dump();
+#endif
+
+ if(mPrev || mNext) {
+ // While the normal practice is to call remove before we call
+ // delete. Its possible for objects without a re-use list
+ // for avoiding duplication to be created on the stack. In those
+ // cases we need to remove ourself here.
+ asyncLock();
+ remove();
+ asyncUnlock();
+ }
+
rsAssert(!mUserRefCount);
rsAssert(!mSysRefCount);
- remove();
}
void ObjectBase::dumpLOGV(const char *op) const
{
if (mName.size()) {
- LOGV("%s RSobj %p, name %s, refs %i,%i from %s,%i links %p,%p,%p",
- op, this, mName.string(), mUserRefCount, mSysRefCount, mAllocFile, mAllocLine, mNext, mPrev, mRSC);
+ LOGV("%s RSobj %p, name %s, refs %i,%i links %p,%p,%p",
+ op, this, mName.string(), mUserRefCount, mSysRefCount, mNext, mPrev, mRSC);
} else {
- LOGV("%s RSobj %p, no-name, refs %i,%i from %s,%i links %p,%p,%p",
- op, this, mUserRefCount, mSysRefCount, mAllocFile, mAllocLine, mNext, mPrev, mRSC);
+ LOGV("%s RSobj %p, no-name, refs %i,%i links %p,%p,%p",
+ op, this, mUserRefCount, mSysRefCount, mNext, mPrev, mRSC);
}
}
void ObjectBase::incUserRef() const
{
- lockUserRef();
- mUserRefCount++;
- unlockUserRef();
- //LOGV("ObjectBase %p inc ref %i", this, mUserRefCount);
-}
-
-void ObjectBase::prelockedIncUserRef() const
-{
- mUserRefCount++;
+ android_atomic_inc(&mUserRefCount);
+ //LOGV("ObjectBase %p incU ref %i, %i", this, mUserRefCount, mSysRefCount);
}
void ObjectBase::incSysRef() const
{
- mSysRefCount ++;
- //LOGV("ObjectBase %p inc ref %i", this, mSysRefCount);
+ android_atomic_inc(&mSysRefCount);
+ //LOGV("ObjectBase %p incS ref %i, %i", this, mUserRefCount, mSysRefCount);
}
-bool ObjectBase::checkDelete() const
+void ObjectBase::preDestroy() const
{
- if (!(mSysRefCount | mUserRefCount)) {
- lockUserRef();
+}
- // Recheck the user ref count since it can be incremented from other threads.
- if (mUserRefCount) {
- unlockUserRef();
- return false;
- }
+bool ObjectBase::checkDelete(const ObjectBase *ref)
+{
+ if (!ref) {
+ return false;
+ }
- if (mRSC && mRSC->props.mLogObjects) {
- dumpLOGV("checkDelete");
- }
- unlockUserRef();
- delete this;
- return true;
+ asyncLock();
+ // This lock protects us against the non-RS threads changing
+ // the ref counts. At this point we should be the only thread
+ // working on them.
+ if (ref->mUserRefCount || ref->mSysRefCount) {
+ asyncUnlock();
+ return false;
+ }
+
+ ref->remove();
+ // At this point we can unlock because there should be no possible way
+ // for another thread to reference this object.
+ ref->preDestroy();
+ asyncUnlock();
+ delete ref;
+ return true;
+}
+
+
+bool ObjectBase::decUserRef() const
+{
+ //LOGV("ObjectBase %p decU ref %i, %i", this, mUserRefCount, mSysRefCount);
+ rsAssert(mUserRefCount > 0);
+ if ((android_atomic_dec(&mUserRefCount) <= 1) &&
+ (android_atomic_acquire_load(&mSysRefCount) <= 0)) {
+ return checkDelete(this);
}
return false;
}
-bool ObjectBase::decUserRef() const
-{
- lockUserRef();
- rsAssert(mUserRefCount > 0);
- //dumpLOGV("decUserRef");
- mUserRefCount--;
- unlockUserRef();
- bool ret = checkDelete();
- return ret;
-}
-
bool ObjectBase::zeroUserRef() const
{
- lockUserRef();
- // This can only happen during cleanup and is therefore
- // thread safe.
- mUserRefCount = 0;
- //dumpLOGV("zeroUserRef");
- unlockUserRef();
- bool ret = checkDelete();
- return ret;
+ //LOGV("ObjectBase %p zeroU ref %i, %i", this, mUserRefCount, mSysRefCount);
+ android_atomic_acquire_store(0, &mUserRefCount);
+ if (android_atomic_acquire_load(&mSysRefCount) <= 0) {
+ return checkDelete(this);
+ }
+ return false;
}
bool ObjectBase::decSysRef() const
{
+ //LOGV("ObjectBase %p decS ref %i, %i", this, mUserRefCount, mSysRefCount);
rsAssert(mSysRefCount > 0);
- mSysRefCount --;
- //dumpLOGV("decSysRef");
- return checkDelete();
+ if ((android_atomic_dec(&mSysRefCount) <= 1) &&
+ (android_atomic_acquire_load(&mUserRefCount) <= 0)) {
+ return checkDelete(this);
+ }
+ return false;
}
void ObjectBase::setName(const char *name)
@@ -141,19 +160,19 @@
mName.setTo(name, len);
}
-void ObjectBase::lockUserRef()
+void ObjectBase::asyncLock()
{
pthread_mutex_lock(&gObjectInitMutex);
}
-void ObjectBase::unlockUserRef()
+void ObjectBase::asyncUnlock()
{
pthread_mutex_unlock(&gObjectInitMutex);
}
void ObjectBase::add() const
{
- pthread_mutex_lock(&gObjectInitMutex);
+ asyncLock();
rsAssert(!mNext);
rsAssert(!mPrev);
@@ -164,12 +183,11 @@
}
mRSC->mObjHead = this;
- pthread_mutex_unlock(&gObjectInitMutex);
+ asyncUnlock();
}
void ObjectBase::remove() const
{
- lockUserRef();
//LOGV("calling remove rsc %p", mRSC);
if (!mRSC) {
rsAssert(!mPrev);
@@ -188,7 +206,6 @@
}
mPrev = NULL;
mNext = NULL;
- unlockUserRef();
}
void ObjectBase::zeroAllUserRef(Context *rsc)
@@ -219,7 +236,7 @@
void ObjectBase::dumpAll(Context *rsc)
{
- lockUserRef();
+ asyncLock();
LOGV("Dumping all objects");
const ObjectBase * o = rsc->mObjHead;
@@ -229,22 +246,22 @@
o = o->mNext;
}
- unlockUserRef();
+ asyncUnlock();
}
bool ObjectBase::isValid(const Context *rsc, const ObjectBase *obj)
{
- lockUserRef();
+ asyncLock();
const ObjectBase * o = rsc->mObjHead;
while (o) {
if (o == obj) {
- unlockUserRef();
+ asyncUnlock();
return true;
}
o = o->mNext;
}
- unlockUserRef();
+ asyncUnlock();
return false;
}
diff --git a/libs/rs/rsObjectBase.h b/libs/rs/rsObjectBase.h
index 8d1ace1..5f03db5 100644
--- a/libs/rs/rsObjectBase.h
+++ b/libs/rs/rsObjectBase.h
@@ -19,6 +19,11 @@
#include "rsUtils.h"
+#define RS_OBJECT_DEBUG 0
+
+#if RS_OBJECT_DEBUG
+ #include <utils/CallStack.h>
+#endif
namespace android {
namespace renderscript {
@@ -31,7 +36,6 @@
{
public:
ObjectBase(Context *rsc);
- virtual ~ObjectBase();
void incSysRef() const;
bool decSysRef() const;
@@ -39,7 +43,8 @@
void incUserRef() const;
bool decUserRef() const;
bool zeroUserRef() const;
- void prelockedIncUserRef() const;
+
+ static bool checkDelete(const ObjectBase *);
const char * getName() const {
return mName.string();
@@ -58,13 +63,18 @@
static bool isValid(const Context *rsc, const ObjectBase *obj);
- static void lockUserRef();
- static void unlockUserRef();
+ // The async lock is taken during object creation in non-rs threads
+ // and object deletion in the rs thread.
+ static void asyncLock();
+ static void asyncUnlock();
protected:
- const char *mAllocFile;
- uint32_t mAllocLine;
+ // Called inside the async lock for any object list management that is
+ // necessary in derived classes.
+ virtual void preDestroy() const;
+
Context *mRSC;
+ virtual ~ObjectBase();
private:
static pthread_mutex_t gObjectInitMutex;
@@ -72,14 +82,17 @@
void add() const;
void remove() const;
- bool checkDelete() const;
-
String8 mName;
mutable int32_t mSysRefCount;
mutable int32_t mUserRefCount;
mutable const ObjectBase * mPrev;
mutable const ObjectBase * mNext;
+
+#if RS_OBJECT_DEBUG
+ CallStack mStack;
+#endif
+
};
template<class T>
@@ -104,6 +117,10 @@
}
}
+ ObjectBaseRef & operator= (const ObjectBaseRef &ref) {
+ return ObjectBaseRef(ref);
+ }
+
~ObjectBaseRef() {
clear();
}
diff --git a/libs/rs/rsProgram.cpp b/libs/rs/rsProgram.cpp
index 10e00e6..72d1b02 100644
--- a/libs/rs/rsProgram.cpp
+++ b/libs/rs/rsProgram.cpp
@@ -31,8 +31,6 @@
Program::Program(Context *rsc) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mDirty = true;
mShaderID = 0;
mAttribCount = 0;
@@ -55,8 +53,6 @@
const uint32_t * params, uint32_t paramLength) :
ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mDirty = true;
mShaderID = 0;
mAttribCount = 0;
diff --git a/libs/rs/rsProgramFragment.cpp b/libs/rs/rsProgramFragment.cpp
index 81b4fa4..33399d5 100644
--- a/libs/rs/rsProgramFragment.cpp
+++ b/libs/rs/rsProgramFragment.cpp
@@ -36,9 +36,6 @@
uint32_t paramLength) :
Program(rsc, shaderText, shaderLength, params, paramLength)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
-
mConstantColor[0] = 1.f;
mConstantColor[1] = 1.f;
mConstantColor[2] = 1.f;
@@ -182,8 +179,8 @@
ProgramFragmentState::~ProgramFragmentState()
{
- delete mPF;
-
+ ObjectBase::checkDelete(mPF);
+ mPF = NULL;
}
void ProgramFragmentState::init(Context *rsc)
diff --git a/libs/rs/rsProgramRaster.cpp b/libs/rs/rsProgramRaster.cpp
index 62d060d..5f8c4ba 100644
--- a/libs/rs/rsProgramRaster.cpp
+++ b/libs/rs/rsProgramRaster.cpp
@@ -36,8 +36,6 @@
bool pointSprite) :
Program(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mPointSmooth = pointSmooth;
mLineSmooth = lineSmooth;
mPointSprite = pointSprite;
diff --git a/libs/rs/rsProgramStore.cpp b/libs/rs/rsProgramStore.cpp
index 586a89f..876299f 100644
--- a/libs/rs/rsProgramStore.cpp
+++ b/libs/rs/rsProgramStore.cpp
@@ -33,8 +33,6 @@
ProgramStore::ProgramStore(Context *rsc) :
Program(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mDitherEnable = true;
mBlendEnable = false;
mColorRWriteEnable = true;
@@ -236,8 +234,8 @@
ProgramStoreState::~ProgramStoreState()
{
- delete mPFS;
-
+ ObjectBase::checkDelete(mPFS);
+ mPFS = NULL;
}
void ProgramStoreState::init(Context *rsc)
@@ -258,9 +256,8 @@
void rsi_ProgramStoreBegin(Context * rsc, RsElement in, RsElement out)
{
- delete rsc->mStateFragmentStore.mPFS;
+ ObjectBase::checkDelete(rsc->mStateFragmentStore.mPFS);
rsc->mStateFragmentStore.mPFS = new ProgramStore(rsc);
-
}
void rsi_ProgramStoreDepthFunc(Context *rsc, RsDepthFunc func)
diff --git a/libs/rs/rsProgramVertex.cpp b/libs/rs/rsProgramVertex.cpp
index a785262..d12439f 100644
--- a/libs/rs/rsProgramVertex.cpp
+++ b/libs/rs/rsProgramVertex.cpp
@@ -37,9 +37,6 @@
uint32_t paramLength) :
Program(rsc, shaderText, shaderLength, params, paramLength)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
-
init(rsc);
}
diff --git a/libs/rs/rsSampler.cpp b/libs/rs/rsSampler.cpp
index 180d78e..cfae7b2 100644
--- a/libs/rs/rsSampler.cpp
+++ b/libs/rs/rsSampler.cpp
@@ -33,8 +33,6 @@
Sampler::Sampler(Context *rsc) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
// Should not get called.
rsAssert(0);
}
@@ -47,8 +45,6 @@
RsSamplerValue wrapR,
float aniso) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mMagFilter = magFilter;
mMinFilter = minFilter;
mWrapS = wrapS;
diff --git a/libs/rs/rsScript.cpp b/libs/rs/rsScript.cpp
index c5632b5..ef380d2 100644
--- a/libs/rs/rsScript.cpp
+++ b/libs/rs/rsScript.cpp
@@ -21,8 +21,6 @@
Script::Script(Context *rsc) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
memset(&mEnviroment, 0, sizeof(mEnviroment));
mSlots = NULL;
diff --git a/libs/rs/rsScriptC.cpp b/libs/rs/rsScriptC.cpp
index a2910d7..165fa71 100644
--- a/libs/rs/rsScriptC.cpp
+++ b/libs/rs/rsScriptC.cpp
@@ -35,8 +35,6 @@
ScriptC::ScriptC(Context *rsc) : Script(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mBccScript = NULL;
memset(&mProgram, 0, sizeof(mProgram));
}
@@ -524,11 +522,11 @@
{
ScriptCState *ss = &rsc->mScriptC;
- ObjectBaseRef<ScriptC> s = ss->mScript.get();
+ ObjectBaseRef<ScriptC> s(ss->mScript);
ss->mScript.clear();
+ s->incUserRef();
ss->runCompiler(rsc, s.get());
- s->incUserRef();
ss->clear(rsc);
return s.get();
}
diff --git a/libs/rs/rsType.cpp b/libs/rs/rsType.cpp
index 27b1b4f..82ad33e 100644
--- a/libs/rs/rsType.cpp
+++ b/libs/rs/rsType.cpp
@@ -27,8 +27,6 @@
Type::Type(Context *rsc) : ObjectBase(rsc)
{
- mAllocFile = __FILE__;
- mAllocLine = __LINE__;
mLODs = 0;
mLODCount = 0;
mAttribs = NULL;
@@ -36,7 +34,7 @@
clear();
}
-Type::~Type()
+void Type::preDestroy()
{
for (uint32_t ct = 0; ct < mRSC->mStateType.mTypes.size(); ct++) {
if (mRSC->mStateType.mTypes[ct] == this) {
@@ -44,6 +42,10 @@
break;
}
}
+}
+
+Type::~Type()
+{
if (mLODs) {
delete [] mLODs;
mLODs = NULL;
@@ -401,7 +403,7 @@
}
}
-void * rsaTypeCreate(RsContext con, RsElement _e, uint32_t dimCount,
+RsType rsaTypeCreate(RsContext con, RsElement _e, uint32_t dimCount,
const RsDimension *dims, const uint32_t *vals)
{
Context *rsc = static_cast<Context *>(con);
@@ -428,7 +430,7 @@
}
}
- ObjectBase::lockUserRef();
+ ObjectBase::asyncLock();
for (uint32_t ct=0; ct < stc->mTypes.size(); ct++) {
Type *t = stc->mTypes[ct];
if (t->getElement() != e) continue;
@@ -437,11 +439,11 @@
if (t->getDimZ() != dimZ) continue;
if (t->getDimLOD() != dimLOD) continue;
if (t->getDimFaces() != dimFaces) continue;
- t->prelockedIncUserRef();
- ObjectBase::unlockUserRef();
+ t->incUserRef();
+ ObjectBase::asyncUnlock();
return t;
}
- ObjectBase::unlockUserRef();
+ ObjectBase::asyncUnlock();
Type * st = new Type(rsc);
st->incUserRef();
@@ -453,9 +455,9 @@
st->setDimFaces(dimFaces);
st->compute();
- ObjectBase::lockUserRef();
+ ObjectBase::asyncLock();
stc->mTypes.push(st);
- ObjectBase::unlockUserRef();
+ ObjectBase::asyncUnlock();
return st;
}
diff --git a/libs/rs/rsType.h b/libs/rs/rsType.h
index a0c77ab..6b89413 100644
--- a/libs/rs/rsType.h
+++ b/libs/rs/rsType.h
@@ -124,6 +124,9 @@
bool isValidGLComponent(uint32_t fieldIdx);
void makeGLComponents();
+protected:
+ virtual void preDestroy();
+
private:
Type(const Type &);
};