| Summary: | c++: searching for declaration sometimes requires writelock when template declaration is instantiated | ||
|---|---|---|---|
| Product: | [Applications] kdevelop | Reporter: | Leandro Santiago da Silva <leandrosansilva> |
| Component: | general | Assignee: | kdevelop-bugs-null |
| Status: | RESOLVED FIXED | ||
| Severity: | crash | CC: | adamwood, aleixpol, amantia, daniel, david.nolden.kde, leandrosansilva |
| Priority: | NOR | ||
| Version: | 4.3.60 | ||
| Target Milestone: | 4.3.0 | ||
| Platform: | Ubuntu | ||
| OS: | Linux | ||
| Latest Commit: | http://commits.kde.org/kdevelop/e112a01c5c5b3562320afed848725f91e920a3c7 | Version Fixed In: | |
| Sentry Crash Report: | |||
| Attachments: |
crash backtrace from gdb
test case New crash information added by DrKonqi KDevelop console output. Kdevplatform patch Kdevelop patch |
||
|
Description
Leandro Santiago da Silva
2012-03-30 19:18:20 UTC
sigh this is because the duchain was not write locked :-S david: searching for a declaration should only require the readlock, but in templatedeclaration::instantiate there is code that creates a temporary context and deletes it afterwards (delete newTemplateContext;) which can cause the above - any idea what to do here? @leandro: do you have a reproducible test case for this? *** Bug 300440 has been marked as a duplicate of this bug. *** (In reply to comment #1) > sigh this is because the duchain was not write locked :-S > > david: searching for a declaration should only require the readlock, but in > templatedeclaration::instantiate there is code that creates a temporary > context and deletes it afterwards (delete newTemplateContext;) which can > cause the above - any idea what to do here? > > @leandro: do you have a reproducible test case for this? Millian, sorry for the delay (of some months!). O don't know if it will happen in another installation, but I tried and tried again with master branch versions of kdevelop and this bug still happens. The funniest part is it occours only when I have opened a copy of llvm source tree. The copy I'm using if from the LLVM official git mirror: http://llvm.org/git/llvm.git Today I recompiled kdevelop and opened llvm and again after some minutes of project analysis, kdevelop crashed. I'm also sending the output of the backtrace I could get with gdb. Created attachment 71719 [details]
crash backtrace from gdb
Created attachment 71868 [details]
test case
Attached test case.
I created a standard hello world cmake project.
I simply added #include <boost/proto/matches.hpp> and it crashed almost immediately.
Included in the gzipped test case is the full debug output in a txt file that clearly shows near the end that the boost include is responsible.
Any help tracking this down would be greatly appreciated as it's likely to mean I have to switch away from kdevelop very soon. I'm happy to put in a good deal of effort myself to fix this but I don't know enough about the locking within duchain to get very far.
I had intended to state that I'm using boost 1.49.0 Created attachment 71999 [details]
New crash information added by DrKonqi
kdevelop (4.3.60) on KDE Platform 4.8.3 (4.8.3) using Qt 4.8.1
- What I was doing when the application crashed:
I imported the LLVM CMake project. Only it
- Unusual behavior I noticed:
KDevelop crashed.
To invalidate the hypotesis of a dirty installation, I reinstalled Kubuntu 12.04 in a clean partition and compiled KDevelop (and related libraries) from a clean source code tree. The probelm still happens.
-- Backtrace (Reduced):
#6 0x00007f189a87a445 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#7 0x00007f189a87dbab in __GI_abort () at abort.c:91
[...]
#11 0x00007f1806c6b366 in Cpp::TemplateDeclaration::deleteAllInstantiations (this=0x7f17f25bdb60) at /home/tenchi/projects/kdevelop/kdevelop/languages/cpp/cppduchain/templatedeclaration.cpp:712
#12 0x00007f1806c6b78b in Cpp::TemplateDeclaration::~TemplateDeclaration (this=0x7f17f25bdb60, __in_chrg=<optimized out>) at /home/tenchi/projects/kdevelop/kdevelop/languages/cpp/cppduchain/templatedeclaration.cpp:316
#13 0x00007f1806c37b6a in Cpp::SpecialTemplateDeclaration<TemplateParameterDeclaration>::~SpecialTemplateDeclaration (this=0x7f17f25bdb30, __in_chrg=<optimized out>) at /home/tenchi/projects/kdevelop/kdevelop/languages/cpp/cppduchain/templatedeclaration.h:247
I don't know if it will happen, but I'm also sending console output of kdevelop in moment of crashing. Created attachment 72000 [details]
KDevelop console output.
Created attachment 72252 [details]
Kdevplatform patch
This may not be an ideal fix but it has certainly improved the stability of my build. The patch is in two parts. First a kdevplatform patch to store the temporary contexts required by template instantiations, along with appropriate clean up. The second part is a small patch to kdevelop so that instead of deleting directly, the temporary context deletion is postponed.
Created attachment 72253 [details]
Kdevelop patch
Just an update: One way to reproduce this bug is getting the llvm source code from git (http://llvm.org/git/llvm.git) and then putting clang (http://llvm.org/svn/llvm-project/cfe/trunk) source code in llvm/tools/ as described in clang page. This bug happens here only when I have clang as a llvm subproject. When I open only llvm without clang, kdevelop doesn't crash. *** Bug 307352 has been marked as a duplicate of this bug. *** *** Bug 306394 has been marked as a duplicate of this bug. *** I think the issue is rather, that when one clones a declaration with an internal context, both the original and the clone reference the same context... Maybe it would be sufficient to also clone the internal context and ensure that it will not be "inDUChain()".
Here is a different patch that simply prevents the call to internalContext->setOwner for cloned declarations... Not really nice but seems to work. Of course, the correct way would be the above, this is just a proof-of-concept.
diff --git a/language/duchain/declaration.cpp b/language/duchain/declaration.cpp
index 6a8cdcc..e5f5df2 100644
--- a/language/duchain/declaration.cpp
+++ b/language/duchain/declaration.cpp
@@ -160,7 +160,7 @@ Declaration::~Declaration()
TopDUContext* topContext = this->topContext();
//Only perform the actions when the top-context isn't being deleted, or when it hasn't been stored to disk
- if(persistentlyDestroying()) {
+ if(persistentlyDestroying() && !d_func()->m_wasCloned) {
DUCHAIN_D_DYNAMIC(Declaration);
// Inserted by the builder after construction has finished.
if( d->m_internalContext.context() )
@@ -677,6 +677,7 @@ Declaration* Declaration::clonePrivate() const {
Declaration* Declaration::clone() const {
Declaration* ret = clonePrivate();
ret->d_func_dynamic()->m_inSymbolTable = false;
+ ret->d_func_dynamic()->m_wasCloned = true;
return ret;
}
diff --git a/language/duchain/declarationdata.h b/language/duchain/declarationdata.h
index f484f9d..4616358 100644
--- a/language/duchain/declarationdata.h
+++ b/language/duchain/declarationdata.h
@@ -62,6 +62,7 @@ public:
bool m_alwaysForceDirect : 1;
bool m_isAutoDeclaration : 1;
bool m_isExplicitlyDeleted : 1;
+ bool m_wasCloned : 1;
};
}
Actually, a global write lock should not be required to delete the temporary context, because it's not in the duchain, and thus it's not visible to other threads.
Can you check, whether simply removing the ENSURE_CHAIN_WRITE_LOCK would fix the issue?
The correct solution would be, to replace it with "if(dynamic_cast<Declaration*>(this)->inDUChain() { ENSURE_CHAIN_WRITE_LOCKED }". This is how the check is performed in KDevelop::DUContext and KDevelop::Declaration.
Since the code is called from within the destructor, dynamic_cast doesn't work. I guess we should simply remove this check. for those interested in trying out a patch: https://git.reviewboard.kde.org/r/106757/ Git commit e112a01c5c5b3562320afed848725f91e920a3c7 by Milian Wolff. Committed on 08/10/2012 at 11:39. Pushed by mwolff into branch 'master'. Make temporary DUContext for default template parameters obsolete. Use a thread-local QMultiHash<IndexedQualifiedIdentifier, IndexedType> lookup table to support default template parameters. This obsoletes the necessity of the temporary DUContext which may introduce instabilities as described in bug 297133. One big caveat was cloning template declarations which contain an internal context. This resulted in both, the original and clone to reference the same internal context. Upon destruction of the clone, the ownership of the internal context was tried to be changed. This can crash, as it could happen while only holding a read lock, yet the referenced internal context is in the DUChain and thus must only be altered while holding a write lock. This change also makes it possible to remove the code surrounding Declaration::clone and Declaration::clonePrivate, as this was the only places which actually used it. Considering the major pitfalls and caveats of this API, I say this is a very good thing. Furthermore, while introducing the thread-local data, I fixed the two safety recursion counters: Previously they where shared between threads which does not make any sense when we want to count the recursion depth that is thread-specific. REVIEW: 106757 M +137 -84 languages/cpp/cppduchain/templatedeclaration.cpp M +5 -0 languages/cpp/cppduchain/templateparameterdeclaration.cpp M +5 -0 languages/cpp/cppduchain/templateparameterdeclaration.h http://commits.kde.org/kdevelop/e112a01c5c5b3562320afed848725f91e920a3c7 Thanks a lot, I can load again my project in KDevelop! The above patch doesn't seem to have been included in 4.4.0 and 4.4.1 but seems to apply fine and fix the issue in those releases. If you do another 4.4.* release, can you please consider including commit e112a01c5c5b3562320afed848725f91e920a3c7? Without it KDevelop is so unstable for me it's basically useless. Also, is there a better way to find out what realeases bug marked as FIXED is actually fixed in than cloning the whole git repo and grepping the logs? Thanks. |