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