On Thu, Aug 28, 2014 at 8:03 PM, Sean Silva <[email protected]> wrote:
> > > > On Wed, Aug 27, 2014 at 4:28 PM, Alexander Kornienko <[email protected]> > wrote: > >> On Thu, Aug 28, 2014 at 1:03 AM, Sean Silva <[email protected]> >> wrote: >> >>> On Wed, Aug 27, 2014 at 2:36 PM, Alexander Kornienko <[email protected]> >>> wrote: >>> >>>> Author: alexfh >>>> Date: Wed Aug 27 16:36:39 2014 >>>> New Revision: 216620 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=216620&view=rev >>>> Log: >>>> Query CompilationDatabase right before running each compilation. >>>> >>>> Summary: >>>> Query CompilationDatabase right before running each compilation. This >>>> allows >>>> supporting compilation databases that change external state required for >>>> successful compilation. >>>> >>> >>> Could you give an example of this? It's not clear to me what this means. >>> >> >> This means, that in some implementations the call to >> Compilations.getCompileCommands(File) may make changes in the file system >> to allow compilation of the File (e.g. generate headers from .td files). >> State of the file system required for compiling one file may be >> incompatible with the state required for compiling another file, so we >> actually need to run the tool on the file right after we call >> getCompileCommands for this file. >> > > This is really unusual. I wouldn't expect a call getCompileCommands to be > mucking about on a filesystem. Especially since it is marked const. Maybe > split out a specific method for preparing the filesystem for compilation of > the file? IIRC, in C++11 it's really a very bad idea to have a const method > that is not safe to call in parallel (David, do you remember what the rule > is for this?) > I don't believe C++11 actually makes this any worse - while C++11 does have defined threading semantics, it doesn't actually require that types are thread safe. I know Herb or others have certainly talked about definitions of "thread compatibility" that are related to const-ness, but I don't think any of that is part of the standard. I could be wrong, though. > Also, what is there to stop individual compilations of the same file from > having incompatible state? > Yeah, it's certainly worth documenting what the contract is here (& maybe considering other APIs). Probably wouldn't be too bad to make it non-const and/or have a "prep/end" function or somesuch (make return a move-only token that represents the valid state - when that's destroyed the state is no longer required - but perhaps that's just overengineering). - David > > >> >> >>> >>> >>>> Reviewers: klimek, djasper >>>> >>>> Reviewed By: djasper >>>> >>>> Subscribers: klimek, cfe-commits >>>> >>>> Differential Revision: http://reviews.llvm.org/D5086 >>>> >>>> Modified: >>>> cfe/trunk/include/clang/Tooling/Tooling.h >>>> cfe/trunk/lib/Tooling/Tooling.cpp >>>> >>>> Modified: cfe/trunk/include/clang/Tooling/Tooling.h >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Tooling.h?rev=216620&r1=216619&r2=216620&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/include/clang/Tooling/Tooling.h (original) >>>> +++ cfe/trunk/include/clang/Tooling/Tooling.h Wed Aug 27 16:36:39 2014 >>>> @@ -293,8 +293,8 @@ class ClangTool { >>>> FileManager &getFiles() { return *Files; } >>>> >>>> private: >>>> - // We store compile commands as pair (file name, compile command). >>>> - std::vector< std::pair<std::string, CompileCommand> > >>>> CompileCommands; >>>> + const CompilationDatabase &Compilations; >>>> + std::vector<std::string> SourcePaths; >>>> >>>> llvm::IntrusiveRefCntPtr<FileManager> Files; >>>> // Contains a list of pairs (<file name>, <file content>). >>>> >>>> Modified: cfe/trunk/lib/Tooling/Tooling.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Tooling.cpp?rev=216620&r1=216619&r2=216620&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/lib/Tooling/Tooling.cpp (original) >>>> +++ cfe/trunk/lib/Tooling/Tooling.cpp Wed Aug 27 16:36:39 2014 >>>> @@ -273,28 +273,10 @@ bool FrontendActionFactory::runInvocatio >>>> >>>> ClangTool::ClangTool(const CompilationDatabase &Compilations, >>>> ArrayRef<std::string> SourcePaths) >>>> - : Files(new FileManager(FileSystemOptions())), >>>> DiagConsumer(nullptr) { >>>> + : Compilations(Compilations), SourcePaths(SourcePaths), >>>> + Files(new FileManager(FileSystemOptions())), >>>> DiagConsumer(nullptr) { >>>> ArgsAdjusters.push_back(new ClangStripOutputAdjuster()); >>>> ArgsAdjusters.push_back(new ClangSyntaxOnlyAdjuster()); >>>> - for (const auto &SourcePath : SourcePaths) { >>>> - std::string File(getAbsolutePath(SourcePath)); >>>> - >>>> - std::vector<CompileCommand> CompileCommandsForFile = >>>> - Compilations.getCompileCommands(File); >>>> - if (!CompileCommandsForFile.empty()) { >>>> - for (CompileCommand &CompileCommand : CompileCommandsForFile) { >>>> - CompileCommands.push_back( >>>> - std::make_pair(File, std::move(CompileCommand))); >>>> - } >>>> - } else { >>>> - // FIXME: There are two use cases here: doing a fuzzy >>>> - // "find . -name '*.cc' |xargs tool" match, where as a user I >>>> don't care >>>> - // about the .cc files that were not found, and the use case >>>> where I >>>> - // specify all files I want to run over explicitly, where this >>>> should >>>> - // be an error. We'll want to add an option for this. >>>> - llvm::errs() << "Skipping " << File << ". Compile command not >>>> found.\n"; >>>> - } >>>> - } >>>> } >>>> >>>> void ClangTool::setDiagnosticConsumer(DiagnosticConsumer *D) { >>>> @@ -333,36 +315,49 @@ int ClangTool::run(ToolAction *Action) { >>>> llvm::sys::fs::getMainExecutable("clang_tool", &StaticSymbol); >>>> >>>> bool ProcessingFailed = false; >>>> - for (const auto &Command : CompileCommands) { >>>> - // FIXME: chdir is thread hostile; on the other hand, creating the >>>> same >>>> - // behavior as chdir is complex: chdir resolves the path once, thus >>>> - // guaranteeing that all subsequent relative path operations work >>>> - // on the same path the original chdir resulted in. This makes a >>>> difference >>>> - // for example on network filesystems, where symlinks might be >>>> switched >>>> - // during runtime of the tool. Fixing this depends on having a >>>> file system >>>> - // abstraction that allows openat() style interactions. >>>> - if (chdir(Command.second.Directory.c_str())) >>>> - llvm::report_fatal_error("Cannot chdir into \"" + >>>> - Twine(Command.second.Directory) + >>>> "\n!"); >>>> - std::vector<std::string> CommandLine = Command.second.CommandLine; >>>> - for (ArgumentsAdjuster *Adjuster : ArgsAdjusters) >>>> - CommandLine = Adjuster->Adjust(CommandLine); >>>> - assert(!CommandLine.empty()); >>>> - CommandLine[0] = MainExecutable; >>>> - // FIXME: We need a callback mechanism for the tool writer to >>>> output a >>>> - // customized message for each file. >>>> - DEBUG({ >>>> - llvm::dbgs() << "Processing: " << Command.first << ".\n"; >>>> - }); >>>> - ToolInvocation Invocation(std::move(CommandLine), Action, >>>> Files.get()); >>>> - Invocation.setDiagnosticConsumer(DiagConsumer); >>>> - for (const auto &MappedFile : MappedFileContents) { >>>> - Invocation.mapVirtualFile(MappedFile.first, MappedFile.second); >>>> + for (const auto &SourcePath : SourcePaths) { >>>> + std::string File(getAbsolutePath(SourcePath)); >>>> + >>>> + std::vector<CompileCommand> CompileCommandsForFile = >>>> + Compilations.getCompileCommands(File); >>>> >>> >>> The analysis that I did in >>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2013-September/032234.html still >>> holds AFAIK, so this line here takes O(project size) time to execute for a >>> JSON compilation database. >>> >> >> Does it? JSONCompilationDatabase stores commands in llvm::StringMap >> indexed by filename. I would be surprised, if it took O(N) to fetch an item. >> > > Oops, sorry. For some reason I was thinking that this was loading the > database back from disk. I think I was reading "compilation databases that > change external state" as needing to be reloaded from disk, and was looking > for the call that did that. Sorry for the noise. > > -- Sean Silva > > >> >> >>> The containing loop has O(project size) iterations when running a >>> refactoring tool over a codebase, which means that the performance of this >>> routine is now O(N^2), no? That seems unacceptable for our "default" >>> suggested way for people to use clang tooling (JSON compilation database + >>> single tool). >>> >> >>> -- Sean Silva >>> >>> >>>> + if (CompileCommandsForFile.empty()) { >>>> + // FIXME: There are two use cases here: doing a fuzzy >>>> + // "find . -name '*.cc' |xargs tool" match, where as a user I >>>> don't care >>>> + // about the .cc files that were not found, and the use case >>>> where I >>>> + // specify all files I want to run over explicitly, where this >>>> should >>>> + // be an error. We'll want to add an option for this. >>>> + llvm::errs() << "Skipping " << File << ". Compile command not >>>> found.\n"; >>>> + continue; >>>> } >>>> - if (!Invocation.run()) { >>>> - // FIXME: Diagnostics should be used instead. >>>> - llvm::errs() << "Error while processing " << Command.first << >>>> ".\n"; >>>> - ProcessingFailed = true; >>>> + for (CompileCommand &CompileCommand : CompileCommandsForFile) { >>>> + // FIXME: chdir is thread hostile; on the other hand, creating >>>> the same >>>> + // behavior as chdir is complex: chdir resolves the path once, >>>> thus >>>> + // guaranteeing that all subsequent relative path operations work >>>> + // on the same path the original chdir resulted in. This makes a >>>> + // difference for example on network filesystems, where symlinks >>>> might be >>>> + // switched during runtime of the tool. Fixing this depends on >>>> having a >>>> + // file system abstraction that allows openat() style >>>> interactions. >>>> + if (chdir(CompileCommand.Directory.c_str())) >>>> + llvm::report_fatal_error("Cannot chdir into \"" + >>>> + Twine(CompileCommand.Directory) + >>>> "\n!"); >>>> + std::vector<std::string> CommandLine = >>>> CompileCommand.CommandLine; >>>> + for (ArgumentsAdjuster *Adjuster : ArgsAdjusters) >>>> + CommandLine = Adjuster->Adjust(CommandLine); >>>> + assert(!CommandLine.empty()); >>>> + CommandLine[0] = MainExecutable; >>>> + // FIXME: We need a callback mechanism for the tool writer to >>>> output a >>>> + // customized message for each file. >>>> + DEBUG({ llvm::dbgs() << "Processing: " << File << ".\n"; }); >>>> + ToolInvocation Invocation(std::move(CommandLine), Action, >>>> Files.get()); >>>> + Invocation.setDiagnosticConsumer(DiagConsumer); >>>> + for (const auto &MappedFile : MappedFileContents) { >>>> + Invocation.mapVirtualFile(MappedFile.first, MappedFile.second); >>>> + } >>>> + if (!Invocation.run()) { >>>> + // FIXME: Diagnostics should be used instead. >>>> + llvm::errs() << "Error while processing " << File << ".\n"; >>>> + ProcessingFailed = true; >>>> + } >>>> } >>>> } >>>> return ProcessingFailed ? 1 : 0; >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
