diff options
author | Sergey Prigogin | 2011-04-12 22:13:46 +0000 |
---|---|---|
committer | Sergey Prigogin | 2011-04-12 22:13:46 +0000 |
commit | 87ae09ee72282f39509436d1e59529ac0356a7bb (patch) | |
tree | 243ac446d59aad12f5c59c6060084d53c9eaabc1 /codan/org.eclipse.cdt.codan.core.cxx | |
parent | c24ce74075b49689a30a655692de8809a2c574db (diff) | |
download | org.eclipse.cdt-87ae09ee72282f39509436d1e59529ac0356a7bb.tar.gz org.eclipse.cdt-87ae09ee72282f39509436d1e59529ac0356a7bb.tar.xz org.eclipse.cdt-87ae09ee72282f39509436d1e59529ac0356a7bb.zip |
Bug 337486 - AbstractIndexAstChecker allows AST to outlive index read lock.
Diffstat (limited to 'codan/org.eclipse.cdt.codan.core.cxx')
2 files changed, 178 insertions, 162 deletions
diff --git a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/AbstractIndexAstChecker.java b/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/AbstractIndexAstChecker.java index b1874505c51..2a99930285b 100644 --- a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/AbstractIndexAstChecker.java +++ b/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/AbstractIndexAstChecker.java @@ -6,13 +6,14 @@ * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * Alena Laskavaia - initial API and implementation + * Alena Laskavaia - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.core.cxx.model; -import org.eclipse.cdt.codan.core.CodanCorePlugin; import org.eclipse.cdt.codan.core.cxx.Activator; import org.eclipse.cdt.codan.core.model.AbstractCheckerWithProblemPreferences; +import org.eclipse.cdt.codan.core.model.ICheckerInvocationContext; import org.eclipse.cdt.codan.core.model.IProblem; import org.eclipse.cdt.codan.core.model.IProblemLocation; import org.eclipse.cdt.codan.core.model.IProblemLocationFactory; @@ -23,95 +24,112 @@ import org.eclipse.cdt.core.dom.ast.IASTMacroExpansionLocation; import org.eclipse.cdt.core.dom.ast.IASTName; import org.eclipse.cdt.core.dom.ast.IASTNode; import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit; -import org.eclipse.cdt.core.index.IIndex; -import org.eclipse.cdt.internal.core.resources.ResourceLookup; +import org.eclipse.cdt.core.model.CoreModel; +import org.eclipse.cdt.core.model.ICElement; +import org.eclipse.cdt.core.model.ITranslationUnit; import org.eclipse.core.resources.IFile; import org.eclipse.core.resources.IProject; import org.eclipse.core.resources.IResource; import org.eclipse.core.runtime.CoreException; -import org.eclipse.core.runtime.IPath; -import org.eclipse.core.runtime.Path; +import org.eclipse.core.runtime.OperationCanceledException; /** - * Convenience implementation of checker that work on index based ast of a c/c++ + * Convenience implementation of checker that works on index-based AST of a C/C++ * program. * * Clients may extend this class. */ -public abstract class AbstractIndexAstChecker extends AbstractCheckerWithProblemPreferences implements ICAstChecker, - IRunnableInEditorChecker { - private IFile file; - private ICodanCommentMap commentmap; +public abstract class AbstractIndexAstChecker extends AbstractCheckerWithProblemPreferences + implements ICAstChecker, IRunnableInEditorChecker { + private CxxModelsCache modelCache; - protected IFile getFile() { - return file; + @Override + public synchronized boolean processResource(IResource resource) throws OperationCanceledException { + if (!shouldProduceProblems(resource)) + return false; + if (!(resource instanceof IFile)) + return true; + + processFile((IFile) resource); + return false; } - protected IProject getProject() { - return file == null ? null : file.getProject(); - } + private void processFile(IFile file) throws OperationCanceledException { + ICheckerInvocationContext context = getContext(); + synchronized (context) { + modelCache = context.get(CxxModelsCache.class); + if (modelCache == null) { + ICElement celement = CoreModel.getDefault().create(file); + if (!(celement instanceof ITranslationUnit)) { + return; + } + modelCache = new CxxModelsCache((ITranslationUnit) celement); + context.add(modelCache); + } + } - void processFile(IFile file) throws CoreException, InterruptedException { - commentmap = null; - IASTTranslationUnit ast = CxxModelsCache.getInstance().getAst(file); - if (ast == null) - return; - // lock the index for read access - IIndex index = CxxModelsCache.getInstance().getIndex(file); - index.acquireReadLock(); try { - // traverse the ast using the visitor pattern. - this.file = file; - processAst(ast); + IASTTranslationUnit ast = modelCache.getAST(); + if (ast != null) { + synchronized (ast) { + processAst(ast); + } + } + } catch (CoreException e) { + Activator.log(e); } finally { - this.file = null; - index.releaseReadLock(); + modelCache = null; } } - public synchronized boolean processResource(IResource resource) { - if (!shouldProduceProblems(resource)) - return false; - if (resource instanceof IFile) { - IFile file = (IFile) resource; + /* + * (non-Javadoc) + * + * @see IRunnableInEditorChecker#processModel(Object, ICheckerInvocationContext) + */ + public synchronized void processModel(Object model, ICheckerInvocationContext context) { + if (model instanceof IASTTranslationUnit) { + setContext(context); + IASTTranslationUnit ast = (IASTTranslationUnit) model; + synchronized (context) { + modelCache = context.get(CxxModelsCache.class); + if (modelCache == null) { + modelCache = new CxxModelsCache(ast); + context.add(modelCache); + } + } try { - processFile(file); - } catch (CoreException e) { - CodanCorePlugin.log(e); - } catch (InterruptedException e) { - // ignore + processAst(ast); + } finally { + modelCache = null; + setContext(null); } - return false; } - return true; } + @Override + public boolean runInEditor() { + return true; + } public void reportProblem(String id, IASTNode astNode, Object... args) { IProblemLocation loc = getProblemLocation(astNode); - if (loc!=null) reportProblem(id, loc, args); + if (loc != null) + reportProblem(id, loc, args); } + public void reportProblem(IProblem problem, IASTNode astNode, Object... args) { IProblemLocation loc = getProblemLocation(astNode); - if (loc!=null) reportProblem(problem, loc, args); + if (loc != null) + reportProblem(problem, loc, args); } - @SuppressWarnings("restriction") protected IProblemLocation getProblemLocation(IASTNode astNode) { IASTFileLocation astLocation = astNode.getFileLocation(); - IPath location = new Path(astLocation.getFileName()); - IFile astFile = ResourceLookup.selectFileForLocation(location, getProject()); - if (astFile == null) { - astFile = file; - } - if (astFile == null) { - Activator.log("Cannot resolve location: " + location); //$NON-NLS-1$ - return null; - } - return getProblemLocation(astNode, astLocation, astFile); + return getProblemLocation(astNode, astLocation); } - private IProblemLocation getProblemLocation(IASTNode astNode, IASTFileLocation astLocation, IFile astFile) { + private IProblemLocation getProblemLocation(IASTNode astNode, IASTFileLocation astLocation) { int line = astLocation.getStartingLineNumber(); IProblemLocationFactory locFactory = getRuntime().getProblemLocationFactory(); if (hasMacroLocation(astNode) && astNode instanceof IASTName) { @@ -119,59 +137,35 @@ public abstract class AbstractIndexAstChecker extends AbstractCheckerWithProblem if (imageLocation != null) { int start = imageLocation.getNodeOffset(); int end = start + imageLocation.getNodeLength(); - return locFactory.createProblemLocation(astFile, start, end, line); + return locFactory.createProblemLocation(getFile(), start, end, line); } } if (line == astLocation.getEndingLineNumber()) { - return locFactory.createProblemLocation(astFile, astLocation.getNodeOffset(), + return locFactory.createProblemLocation(getFile(), astLocation.getNodeOffset(), astLocation.getNodeOffset() + astLocation.getNodeLength(), line); } - return locFactory.createProblemLocation(astFile, line); + return locFactory.createProblemLocation(getFile(), line); } private boolean hasMacroLocation(IASTNode astNode) { - return astNode.getNodeLocations().length == 1 && astNode.getNodeLocations()[0] instanceof IASTMacroExpansionLocation; + return astNode.getNodeLocations().length == 1 && + astNode.getNodeLocations()[0] instanceof IASTMacroExpansionLocation; } - @Override - public boolean runInEditor() { - return true; + protected IFile getFile() { + return modelCache.getFile(); } - /* - * (non-Javadoc) - * - * @see - * org.eclipse.cdt.codan.core.model.IRunnableInEditorChecker#processModel - * (java.lang.Object) - */ - @SuppressWarnings("restriction") - public synchronized void processModel(Object model) { - if (model instanceof IASTTranslationUnit) { - CxxModelsCache.getInstance().clearCash(); - IASTTranslationUnit ast = (IASTTranslationUnit) model; - IPath location = new Path(ast.getFilePath()); - IFile astFile = ResourceLookup.selectFileForLocation(location, getProject()); - file = astFile; - commentmap = null; - processAst(ast); - } + protected IProject getProject() { + IFile file = getFile(); + return file == null ? null : file.getProject(); } - protected ICodanCommentMap getCommentMap() { - if (commentmap == null) { - try { - CxxModelsCache cxxcache = CxxModelsCache.getInstance(); - synchronized (cxxcache) { - IASTTranslationUnit ast = cxxcache.getAst(getFile()); - commentmap = cxxcache.getCommentedNodeMap(ast); - return commentmap; - } + protected CxxModelsCache getModelCache() { + return modelCache; + } - } catch (Exception e) { - Activator.log(e); - } - } - return commentmap; + protected ICodanCommentMap getCommentMap() { + return modelCache.getCommentedNodeMap(); } } diff --git a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/CxxModelsCache.java b/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/CxxModelsCache.java index 6bf238782e6..63bf6095617 100644 --- a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/CxxModelsCache.java +++ b/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/CxxModelsCache.java @@ -6,7 +6,8 @@ * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * Alena Laskavaia - initial API and implementation + * Alena Laskavaia - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.core.cxx.model; @@ -15,32 +16,71 @@ import java.util.WeakHashMap; import org.eclipse.cdt.codan.core.cxx.Activator; import org.eclipse.cdt.codan.core.cxx.internal.model.CodanCommentMap; import org.eclipse.cdt.codan.core.cxx.internal.model.cfg.CxxControlFlowGraph; +import org.eclipse.cdt.codan.core.model.ICodanDisposable; import org.eclipse.cdt.codan.core.model.cfg.IControlFlowGraph; import org.eclipse.cdt.core.CCorePlugin; import org.eclipse.cdt.core.dom.ast.IASTFunctionDefinition; import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit; import org.eclipse.cdt.core.index.IIndex; import org.eclipse.cdt.core.model.CoreModel; -import org.eclipse.cdt.core.model.ICElement; +import org.eclipse.cdt.core.model.ICProject; import org.eclipse.cdt.core.model.ITranslationUnit; import org.eclipse.cdt.internal.core.dom.rewrite.commenthandler.ASTCommenter; import org.eclipse.core.resources.IFile; +import org.eclipse.core.runtime.Assert; import org.eclipse.core.runtime.CoreException; +import org.eclipse.core.runtime.OperationCanceledException; /** * Cache data models for resource so checkers can share it */ -public class CxxModelsCache { - private IFile file; +public class CxxModelsCache implements ICodanDisposable { + private static final int PARSE_MODE = ITranslationUnit.AST_SKIP_ALL_HEADERS + | ITranslationUnit.AST_CONFIGURE_USING_SOURCE_CONTEXT + | ITranslationUnit.AST_SKIP_TRIVIAL_EXPRESSIONS_IN_AGGREGATE_INITIALIZERS + | ITranslationUnit.AST_PARSE_INACTIVE_CODE; + + private final IFile file; + private final ITranslationUnit tu; private IASTTranslationUnit ast; - private ITranslationUnit tu; private IIndex index; - private WeakHashMap<IASTFunctionDefinition, IControlFlowGraph> cfgmap = new WeakHashMap<IASTFunctionDefinition, IControlFlowGraph>(0); + private final WeakHashMap<IASTFunctionDefinition, IControlFlowGraph> cfgmap; private ICodanCommentMap commentMap; - private static CxxModelsCache instance = new CxxModelsCache(); + private boolean disposed; + + CxxModelsCache(ITranslationUnit tu) { + this.tu = tu; + this.file = tu != null ? (IFile) tu.getResource() : null; + cfgmap = new WeakHashMap<IASTFunctionDefinition, IControlFlowGraph>(0); + } + + CxxModelsCache(IASTTranslationUnit ast) { + this(ast.getOriginatingTranslationUnit()); + this.ast = ast; + } + + public IASTTranslationUnit getAST() throws OperationCanceledException, CoreException { + return getAST(tu); + } + + public IASTTranslationUnit getAST(ITranslationUnit tu) + throws OperationCanceledException, CoreException { + if (!this.tu.equals(tu)) { + throw new IllegalArgumentException(); + } + if (ast == null) { + getIndex(); + ast= tu.getAST(index, PARSE_MODE); + } + return ast; + } + + public ITranslationUnit getTranslationUnit() { + return tu; + } - public static CxxModelsCache getInstance() { - return instance; + public IFile getFile() { + return file; } public synchronized IControlFlowGraph getControlFlowGraph(IASTFunctionDefinition func) { @@ -48,85 +88,67 @@ public class CxxModelsCache { if (cfg != null) return cfg; cfg = CxxControlFlowGraph.build(func); - if (cfgmap.size() > 20) { // if too many function better drop the cash XXX should be LRU + // TODO(Alena Laskavaia): Change to LRU. + if (cfgmap.size() > 20) { // if too many function better drop the cash cfgmap.clear(); } cfgmap.put(func, cfg); return cfg; } - public synchronized IASTTranslationUnit getAst(IFile file) throws CoreException, InterruptedException { - if (file.equals(this.file)) { - return ast; + public synchronized ICodanCommentMap getCommentedNodeMap() { + return getCommentedNodeMap(tu); + } + + public synchronized ICodanCommentMap getCommentedNodeMap(ITranslationUnit tu) { + if (!this.tu.equals(tu)) { + throw new IllegalArgumentException(); } - // create translation unit and access index - ICElement celement = CoreModel.getDefault().create(file); - if (!(celement instanceof ITranslationUnit)) - return null; // not a C/C++ file - clearCash(); - this.file = file; - //System.err.println("Making ast for "+file); - tu = (ITranslationUnit) celement; - index = CCorePlugin.getIndexManager().getIndex(tu.getCProject()); - // lock the index for read access - index.acquireReadLock(); - try { - // create index based ast - ast = tu.getAST(index, ITranslationUnit.AST_SKIP_INDEXED_HEADERS); - if (ast == null) - return null;// - return ast; - } finally { - index.releaseReadLock(); + if (commentMap == null) { + if (ast == null) { + throw new IllegalStateException("getCommentedNodeMap called before getAST"); //$NON-NLS-1$ + } + commentMap = new CodanCommentMap(ASTCommenter.getCommentedNodeMap(ast)); } + return commentMap; } - - public synchronized ICodanCommentMap getCommentedNodeMap(IASTTranslationUnit ast) { - if (this.ast == ast) { + /** + * Returns the index that can be safely used for reading until the cache is disposed. + * + * @return The index. + */ + public synchronized IIndex getIndex() throws CoreException, OperationCanceledException { + Assert.isTrue(!disposed, "CxxASTCache is already disposed."); //$NON-NLS-1$ + if (this.index == null) { + ICProject[] projects = CoreModel.getDefault().getCModel().getCProjects(); + IIndex index = CCorePlugin.getIndexManager().getIndex(projects); try { index.acquireReadLock(); - try { - commentMap = new CodanCommentMap(ASTCommenter.getCommentedNodeMap(ast)); - } finally { - index.releaseReadLock(); - } - return commentMap; } catch (InterruptedException e) { - return null; + throw new OperationCanceledException(); } + this.index = index; } - throw new IllegalArgumentException("Not cached"); - } - - public ICodanCommentMap getCommentedNodeMap(IFile file) { - try { - IASTTranslationUnit ast = getAst(file); - return getCommentedNodeMap(ast); - } catch (InterruptedException e) { - return null; - } catch (CoreException e) { - Activator.log(e); - return null; - } + return this.index; } /** - * Clear cash for current file + * @see IDisposable#dispose() + * This method should not be called concurrently with any other method. */ - public void clearCash() { - cfgmap.clear(); - ast = null; - tu = null; - index = null; - commentMap = null; + public void dispose() { + Assert.isTrue(!disposed, "CxxASTCache.dispose() called more than once."); //$NON-NLS-1$ + disposed = true; + if (index != null) { + index.releaseReadLock(); + } } - public synchronized IIndex getIndex(IFile file) throws CoreException, InterruptedException { - if (file.equals(this.file)) { - return index; - } - getAst(file); // to init variables - return index; + @Override + protected void finalize() throws Throwable { + if (!disposed) + Activator.log("CxxASTCache was not disposed."); //$NON-NLS-1$ + super.finalize(); } } |