r216620 - Query CompilationDatabase right before running each compilation.
David Blaikie
dblaikie at gmail.com
Thu Aug 28 21:55:23 PDT 2014
On Thu, Aug 28, 2014 at 8:03 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
>
> 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?)
>
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
>>>> 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/b7de6b86/attachment.html>
More information about the cfe-commits
mailing list