r216620 - Query CompilationDatabase right before running each compilation.
Sean Silva
chisophugis at gmail.com
Thu Aug 28 20:03:31 PDT 2014
On Wed, Aug 27, 2014 at 4:28 PM, Alexander Kornienko <alexfh at google.com>
wrote:
> On Thu, Aug 28, 2014 at 1:03 AM, Sean Silva <chisophugis at gmail.com> wrote:
>
>> On Wed, Aug 27, 2014 at 2:36 PM, Alexander Kornienko <alexfh at google.com>
>> 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?)
Also, what is there to stop individual compilations of the same file from
having incompatible state?
>
>
>>
>>
>>> 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
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140828/1abf1808/attachment.html>
More information about the cfe-commits
mailing list