[cfe-dev] r137887: [libclang] Workaround potential race condition with code completion AllocatedResults being freed after a CXTranslationUnit.

[Dmitri Gribenko]

Hello,

While working towards fixing the bug in CXString when it reads a one
past end character, I stumbled across r137887.

Specifically, I am worried about this workaround:

137887   kremenek         // Normally, clients of CXString shouldn't
care whether or not
137887   kremenek         // a CXString is managed by a pool or by
explicitly malloc'ed memory.
137887   kremenek         // However, there are cases when
AllocatedResults outlives the
137887   kremenek         // CXTranslationUnit.  This is a workaround
that failure mode.
137887   kremenek         if (cxstring::isManagedByPool(cursorUSR)) {
137887   kremenek           CXString heapStr =
137887   kremenek
cxstring::createCXString(clang_getCString(cursorUSR), true);

I want to make two points here:

(1) There are a lot of other strings accessible from
CXCodeCompleteResults, which are allocated on the ASTContext's
allocator of the translation unit.  So this workaround is only for
some specific case, it is not general.

(1.5) The issue itself looks not like a race condition (none of the
libclang objects are thread-safe), but a bug in the client code.

(2) Fixing the reading one past end character bug will move more
CXStrings to the CXStringPool -- StringRefs that need to be copied
will get allocated there (to minimize malloc traffic).  Of course, it
would be invalid to free them after the corresponding
CXTranslationUnit.

Can we remove this workaround now?  Will (2) have a lot of bad effects
because of assumptions in clients?

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/