Bugzilla – Full Text Bug Listing
| Summary: | AbstractIndexAstChecker allows AST to outlive index read lock | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Sergey Prigogin <eclipse.sprigogin> | ||||||||||
| Component: | cdt-codan | Assignee: | Sergey Prigogin <eclipse.sprigogin> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Elena Laskavaia <elaskavaia.cdt> | ||||||||||
| Severity: | critical | ||||||||||||
| Priority: | P3 | CC: | cdtdoug, malaperle | ||||||||||
| Version: | 8.0 | Flags: | eclipse.sprigogin:
review+
|
||||||||||
| Target Milestone: | 8.0 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
Codan may use something similar to RefactoringASTCache to implement proper index locking. If it makes sense to reuse the same class, RefactoringASTCache can be moved to core and renamed to remove explicit reference to refactoring. Where is this class I cannot even find it? Yes it would be good to use common one because that is what I am trying to do here too - ast cache org.eclipse.cdt.internal.ui.refactoring.RefactoringASTCache It should be there if you type it in Open Type (Ctrl+shift+t). I think it would make sense to move this to core. It will be useful for quick fixes especially. There is already a class called ASTCache though. ASTCash, Alena? ;) I've realized that RefactoringASTCache cannot be moved to core since it depends on org.eclipse.cdt.internal.ui.editor.ASTProvider. We can rename it to DisposableASTCache though. Created attachment 192884 [details]
Proposed fix
I had to make few backward-incompatible API changes. CxxModelsCache is no longer a singleton. ICAstChecker has been moved to an internal package since we don't expect users to implement it without subclassing AbstractIndexAstChecker.
Interaction between CodanBuilder, AbstractIndexAstChecker and CxxModelsCache could be simplified if they all were located in the same plugin. What is the reason for keeping org.eclipse.cdt.codan.core and org.eclipse.cdt.codan.core.cxx separate?
Reason is to keep cdt part separates so people who could integrate other languages do not need to pull cdt in In you fix I think you are adding dependency from codan.core to codan.cxx.core I do not want this dependency for reasons outlines above You patch does not apply on trunk. I did some changes for comments support couple week ago I think you did not update it since We can add this interface to get rid of this dependency
package org.eclipse.cdt.codan.core.model;
import org.eclipse.core.resources.IFile;
/**
* Interface for checkers that can be run when user is typing, checker has to be
* very quick to run in this mode
* <p>
* <strong>EXPERIMENTAL</strong>. This class or interface has been added as part
* of a work in progress. There is no guarantee that this API will work or that
* it will remain the same.
* </p>
*
* @noextend This interface is not intended to be extended by clients.
* @since 2.0
*/
public interface IRunnableInEditorCheckerWithSharedModel extends IRunnableInEditorChecker{
/**
* @param file - current file
* @param model - shared code mode
*/
Object processModel(IFile file, Object model) throws InterruptedException;
}
Another problem - it suppose to be inter-procedural (inter-file) cache. Some checkers require access to multiple ast - it suppose to cache results of this - how does it suppose to work now? Created attachment 192895 [details]
New proposed fix.
Here is a much cleaner solution. It doesn't introduce any plugin dependencies but doesn't sacrifice type safety either. I think it addressed all the comments.
(In reply to comment #10) > Another problem - it suppose to be inter-procedural (inter-file) cache. > Some checkers require access to multiple ast - it suppose to cache results of > this - how does it suppose to work now? I added methods that accept ITranslationUnit. ITranslationUnit is preferable to IFile since conceivably checkers may use an AST of a header file that is located outside workspace. Created attachment 192908 [details]
New fix with minor modifications
Added context parameter to IRunnableInEditorChecker.processModel method.
I was thinking too of breaking API by changing processModel signature. It should not be that bad since nobody should really use this method directly? Do you want to post this API breaking on CDT forum just to make sure? Created attachment 192966 [details]
Final version of the fix
Btw I don't remember why before and after return boolean in IChecker. If you can't think of anything either may as well change to void while we breaking API's And if context is not stored in checker we should pass it before, after and enabledInContext functions too (In reply to comment #16) > Btw I don't remember why before and after return boolean in IChecker. If you > can't think of anything either may as well change to void while we breaking > API's I have doubts about 'before' and 'after' methods. They complicate the API and look unnecessary to me. (In reply to comment #17) > And if context is not stored in checker we should pass it before, after and > enabledInContext functions too Unfortunately, several objects are stored inside a checker. That includes context, problemReporter and modelCache. Ideally, checkers should be stateless, but they were not stateless before my change. ok just commit what you have, we can discuss further changes later Fixed in HEAD > 20110412. *** cdt cvs genie on behalf of sprigogin *** Bug 337486 - AbstractIndexAstChecker allows AST to outlive index read lock. [*] AbstractIndexAstChecker.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/AbstractIndexAstChecker.java?root=Tools_Project&r1=1.17&r2=1.18 [*] CxxModelsCache.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/CxxModelsCache.java?root=Tools_Project&r1=1.8&r2=1.9 [*] ReturnChecker.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java?root=Tools_Project&r1=1.19&r2=1.20 [*] CodanFastCxxAstTestCase.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/CodanFastCxxAstTestCase.java?root=Tools_Project&r1=1.4&r2=1.5 [+] ICodanDisposable.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICodanDisposable.java?root=Tools_Project&revision=1.1&view=markup [*] AbstractChecker.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/AbstractChecker.java?root=Tools_Project&r1=1.17&r2=1.18 [*] IRunnableInEditorChecker.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IRunnableInEditorChecker.java?root=Tools_Project&r1=1.3&r2=1.4 [*] IChecker.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IChecker.java?root=Tools_Project&r1=1.9&r2=1.10 [*] ICheckerInvocationContext.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICheckerInvocationContext.java?root=Tools_Project&r1=1.1&r2=1.2 [*] CheckerInvocationContext.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CheckerInvocationContext.java?root=Tools_Project&r1=1.2&r2=1.3 [*] CodanBuilder.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CodanBuilder.java?root=Tools_Project&r1=1.19&r2=1.20 I installed the latest Hudson build 642 today and now having deadlocks related to AST: "Worker-0" prio=6 tid=0x52153800 nid=0x1228 waiting for monitor entry [0x5434f000] java.lang.Thread.State: BLOCKED (on object monitor) at org.eclipse.cdt.internal.core.model.ASTCache.releaseSharedAST(ASTCache.java:259) - waiting to lock <0x12b61908> (a java.lang.Object) at org.eclipse.cdt.internal.core.model.ASTCache.runOnAST(ASTCache.java:231) at org.eclipse.cdt.internal.ui.editor.ASTProvider.runOnAST(ASTProvider.java:334) at org.eclipse.cdt.internal.ui.viewsupport.SelectionListenerWithASTManager$PartListenerGroup.calculateASTandInform(SelectionListenerWithASTManager.java:168) at org.eclipse.cdt.internal.ui.viewsupport.SelectionListenerWithASTManager$PartListenerGroup$3.run(SelectionListenerWithASTManager.java:142) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54) "main" prio=6 tid=0x008f6800 nid=0x170c waiting for monitor entry [0x0012f000] java.lang.Thread.State: BLOCKED (on object monitor) at org.eclipse.cdt.internal.ui.editor.ASTProvider.canUseCache(ASTProvider.java:373) - waiting to lock <0x12b3e640> (a org.eclipse.cdt.internal.ui.editor.ASTProvider) at org.eclipse.cdt.internal.ui.editor.ASTProvider.runOnAST(ASTProvider.java:332) at org.eclipse.cdt.internal.ui.editor.CElementHyperlinkDetector.detectHyperlinks(CElementHyperlinkDetector.java:71) at org.eclipse.ui.texteditor.HyperlinkDetectorRegistry$HyperlinkDetectorDelegate.detectHyperlinks(HyperlinkDetectorRegistry.java:80) at org.eclipse.jface.text.hyperlink.HyperlinkManager.findHyperlinks(HyperlinkManager.java:286) - locked <0x05ee3070> (a [Lorg.eclipse.jface.text.hyperlink.IHyperlinkDetector;) at org.eclipse.jface.text.hyperlink.HyperlinkManager.findHyperlinks(HyperlinkManager.java:258) "Worker-11" prio=6 tid=0x540c6800 nid=0x480 waiting for monitor entry [0x5913f000] java.lang.Thread.State: BLOCKED (on object monitor) at org.eclipse.cdt.internal.core.model.ASTCache.isActiveElement(ASTCache.java:371) - waiting to lock <0x12b61908> (a java.lang.Object) at org.eclipse.cdt.internal.ui.editor.ASTProvider.canUseCache(ASTProvider.java:373) - locked <0x12b3e640> (a org.eclipse.cdt.internal.ui.editor.ASTProvider) at org.eclipse.cdt.internal.ui.editor.ASTProvider.runOnAST(ASTProvider.java:332) at org.eclipse.cdt.internal.ui.search.actions.OpenDeclarationsJob.performNavigation(OpenDeclarationsJob.java:144) at org.eclipse.cdt.internal.ui.search.actions.OpenDeclarationsJob.run(OpenDeclarationsJob.java:120) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54) Locked ownable synchronizers: - None "Worker-10" prio=6 tid=0x549d0400 nid=0x1500 waiting for monitor entry [0x5903f000] java.lang.Thread.State: BLOCKED (on object monitor) at org.eclipse.cdt.internal.ui.editor.ASTProvider.canUseCache(ASTProvider.java:373) - waiting to lock <0x12b3e640> (a org.eclipse.cdt.internal.ui.editor.ASTProvider) at org.eclipse.cdt.internal.ui.editor.ASTProvider.runOnAST(ASTProvider.java:332) at org.eclipse.cdt.internal.ui.search.actions.OpenDeclarationsJob.performNavigation(OpenDeclarationsJob.java:144) at org.eclipse.cdt.internal.ui.search.actions.OpenDeclarationsJob.run(OpenDeclarationsJob.java:120) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54) "Worker-4" prio=6 tid=0x54882400 nid=0x1784 waiting on condition [0x58a3f000] java.lang.Thread.State: WAITING (parking) at sun.misc.Unsafe.park(Native Method) - parking to wait for <0x0349aed8> (a java.util.concurrent.Semaphore$NonfairSync) at java.util.concurrent.locks.LockSupport.park(Unknown Source) at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(Unknown Source) at java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedInterruptibly(Unknown Source) at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireSharedInterruptibly(Unknown Source) at java.util.concurrent.Semaphore.acquire(Unknown Source) at org.eclipse.cdt.internal.core.model.ASTCache.acquireSharedAST(ASTCache.java:248) - locked <0x12b61908> (a java.lang.Object) at org.eclipse.cdt.internal.core.model.ASTCache.runOnAST(ASTCache.java:223) at org.eclipse.cdt.internal.ui.editor.ASTProvider.runOnAST(ASTProvider.java:334) at org.eclipse.cdt.internal.ui.search.actions.OpenDeclarationsJob.performNavigation(OpenDeclarationsJob.java:144) (In reply to comment #23) A fix for the deadlock has been submitted. |
An index-based AST should never be accessed after releasing the read index lock that was held when the AST was created. Failure to do so may result in errors like: java.lang.ArrayIndexOutOfBoundsException: 4422 at org.eclipse.cdt.internal.core.pdom.db.Database.getChunk(Database.java:274) at org.eclipse.cdt.internal.core.pdom.db.Database.getByte(Database.java:461) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPClassType.isAnonymous(PDOMCPPClassType.java:231) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPClassScope$PopulateMap.visit(PDOMCPPClassScope.java:75) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPClassScope.getBindingMap(PDOMCPPClassScope.java:236) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPClassScope.getBindingsViaCache(PDOMCPPClassScope.java:174) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPClassScope.getBindings(PDOMCPPClassScope.java:156) at org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPSemantics.getBindingsFromScope(CPPSemantics.java:1196) at org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPSemantics.lookup(CPPSemantics.java:920) at org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPSemantics.resolveBinding(CPPSemantics.java:251) at org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPVisitor.resolveBinding(CPPVisitor.java:1230) at org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPVisitor.createBinding(CPPVisitor.java:272) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTName.createIntermediateBinding(CPPASTName.java:55) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTNameBase.resolveBinding(CPPASTNameBase.java:86) at org.eclipse.cdt.codan.internal.checkers.ProblemBindingChecker$1.visit(ProblemBindingChecker.java:71) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTName.accept(CPPASTName.java:157) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTFieldReference.accept(CPPASTFieldReference.java:177) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTFunctionCallExpression.accept(CPPASTFunctionCallExpression.java:168) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTExpressionStatement.accept(CPPASTExpressionStatement.java:65) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTCompoundStatement.accept(CPPASTCompoundStatement.java:72) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTFunctionDefinition.accept(CPPASTFunctionDefinition.java:181) at org.eclipse.cdt.internal.core.dom.parser.ASTTranslationUnit.accept(ASTTranslationUnit.java:271) at org.eclipse.cdt.codan.internal.checkers.ProblemBindingChecker.processAst(ProblemBindingChecker.java:63) at org.eclipse.cdt.codan.core.cxx.model.AbstractIndexAstChecker.processFile(AbstractIndexAstChecker.java:58) at org.eclipse.cdt.codan.core.cxx.model.AbstractIndexAstChecker.processResource(AbstractIndexAstChecker.java:70) at org.eclipse.cdt.codan.internal.core.CodanBuilder.processResource(CodanBuilder.java:133) at org.eclipse.cdt.codan.internal.core.CodanBuilder.processResource(CodanBuilder.java:94) at org.eclipse.cdt.codan.internal.core.CodanBuilder.processResource(CodanBuilder.java:159) at org.eclipse.cdt.codan.internal.core.CodanBuilder.processResource(CodanBuilder.java:94) at org.eclipse.cdt.codan.internal.core.CodanBuilder.processResource(CodanBuilder.java:159) at org.eclipse.cdt.codan.internal.core.CodanBuilder.processResource(CodanBuilder.java:94) at org.eclipse.cdt.codan.internal.core.CodanBuilder.processResource(CodanBuilder.java:159) at org.eclipse.cdt.codan.internal.core.CodanBuilder.processResource(CodanBuilder.java:94) at org.eclipse.cdt.codan.internal.ui.actions.RunCodeAnalysis$1.run(RunCodeAnalysis.java:59) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)