[cfe-commits] [PATCH] Implements support to run standalone tools

[nobled]

On Mon, Jan 30, 2012 at 2:49 PM, Manuel Klimek <klimek at google.com> wrote:
> Ping + a re-based version of the patch.
>


This looks really cool. Aside from the create()/New() half-rename confusion,
just some minor nits:

> +/// \brief Returns a new FrontendActionFactory for any type that provides an
> +/// implementation of NewFrontendAction().
> +///
> +/// FactoryT must implement: FrontendAction *NewFrontendAction().
> +///
> +/// Example:
> +/// struct ProvidesFrontendActions {
> +///   FrontendActionFactory *NewFrontendAction();
You meant "FrontendAction *" here, right?

> +/// } Factory;
> +/// FrontendActionFactory *FactoryAdapter =
> +///   newFrontendActionFactory(&Factory);
> +template <typename FactoryT>
> +FrontendActionFactory *newFrontendActionFactory(FactoryT *ActionFactory);
> +
> +/// \brief Runs (and deletes) the tool on 'Code' with the -fsynatx-only flag.
syntax*

> +///
> +/// \param ToolAction The action to run over the code.
> +/// \param Code C++ code.
> +///
> +/// \return - True if 'ToolAction' was successfully executed.
> +bool runSyntaxOnlyToolOnCode(
> +    clang::FrontendAction *ToolAction, llvm::StringRef Code);
> +
> +/// \brief Converts a vector<string> into a vector<char*> suitable to pass
> +/// to main-style functions taking (int Argc, char *Argv[]).
> +std::vector<char*> commandLineToArgv(const std::vector<std::string> *Command);
This looks like it can just take an ArrayRef<const std::string>...

> +/// \see JsonCompileCommandLineDatabase
> +CompileCommand findCompileArgsInJsonDatabase(
> +    llvm::StringRef FileName, llvm::StringRef JsonDatabase,
> +    std::string &ErrorMessage);
Just a note on the whole patch: you don't need the llvm:: prefix on StringRef
or ArrayRef, or a few other common types in clang code anymore. You just have
to include "clang/Basic/LLVM.h" to use them inside the clang namespace.


> +  /// \brief Returns the file manager used in the tool.
> +  ///
> +  /// The file manager is shared between all translation units.
> +  FileManager &getFiles() { return Files; }
> +
> + private:
> +  /// \brief Add translation units to run the tool over.
> +  ///
> +  /// Translation units not found in JsonDatabaseDirectory (see constructore)
constructor*

> +  /// will be skipped.
> +  void addTranslationUnits(
> +      llvm::StringRef JsonDatabaseDirectory,
> +      llvm::ArrayRef<std::string> TranslationUnits);
> +
> +  // We store command lines as pair (file name, command line).
> +  typedef std::pair< std::string, std::vector<std::string> > CommandLine;
> +  std::vector<CommandLine> CommandLines;
> +
> +  FileManager Files;
> +  // Maps <file name> -> <file content>.
> +  std::map<std::string, std::string> MappedFileContents;
> +};
> +
> +template <typename T>
> +FrontendActionFactory *newFrontendActionFactory() {
> +  class SimpleFrontendActionFactory : public FrontendActionFactory {
> +  public:
> +    virtual clang::FrontendAction *New() { return new T; }
You mean "create()"?

> +  };
> +
> +  return new SimpleFrontendActionFactory;
> +}
> +
> +template <typename FactoryT>
> +FrontendActionFactory *newFrontendActionFactory(FactoryT *ActionFactory) {
> +  class FrontendActionFactoryAdapter : public FrontendActionFactory {
> +  public:
> +    explicit FrontendActionFactoryAdapter(FactoryT *ActionFactory)
> +      : ActionFactory(ActionFactory) {}
> +
> +    virtual clang::FrontendAction *New() {
"create()" here, too... It looks like these templates aren't covered by the
unittest/getting instantiated at all.

diff --git a/lib/Tooling/CMakeLists.txt b/lib/Tooling/CMakeLists.txt
new file mode 100644
index 0000000..b0e1235
--- /dev/null
> +++ b/lib/Tooling/CMakeLists.txt
@@ -0,0 +1,6 @@
> +set(LLVM_LINK_COMPONENTS support)
> +SET(LLVM_USED_LIBS clangBasic clangFrontend clangAST clangRewrite)
Here and in unittest/Tooling/Makefile, you set a dependency on the Rewrite lib,
but I don't see any Rewrite/ includes. Is there a reason for that?


> +bool ToolInvocation::run() {
> +  const std::vector<char*> Argv = commandLineToArgv(&CommandLine);
> +  const char *const BinaryName = Argv[0];
> +  DiagnosticOptions DefaultDiagnosticOptions;
> +  TextDiagnosticPrinter DiagnosticPrinter(
> +      llvm::errs(), DefaultDiagnosticOptions);
> +  DiagnosticsEngine Diagnostics(llvm::IntrusiveRefCntPtr<clang::DiagnosticIDs>(
> +      new DiagnosticIDs()), &DiagnosticPrinter, false);
> +
> +  const llvm::OwningPtr<clang::driver::Driver> Driver(
> +      newDriver(&Diagnostics, BinaryName));
> +  // Since the input might only be virtual, don't check whether it exists.
> +  Driver->setCheckInputsExist(false);
> +  const llvm::OwningPtr<clang::driver::Compilation> Compilation(
> +      Driver->BuildCompilation(llvm::ArrayRef<const char*>(
> +          &Argv[0], Argv.size() - 1)));
(The fact that the Driver entrypoint need a song and dance to produce
a char * array
is annoying, but that's obviously a pre-existing problem with the
API... I'm hoping
to fix that in the future.)

> +ClangTool::ClangTool(int argc, char **argv)
> +    : Files((FileSystemOptions())) {
> +  if (argc < 3) {
> +    llvm::outs() << "Usage: " << argv[0] << " <cmake-output-dir> "
> +                 << "<file1> <file2> ...\n";
> +    exit(1);
This looks like a good place to use llvm::report_fatal_error() instead. Same
for the two other exit(1) calls below.

> +  }
> +  addTranslationUnits(argv[1], std::vector<std::string>(argv + 2, argv + argc));
> +}
> +
> +void ClangTool::addTranslationUnits(
> +    llvm::StringRef JsonDatabaseDirectory,
> +    llvm::ArrayRef<std::string> TranslationUnits) {
TranslationUnits can just be an ArrayRef<StringRef> here.

> +  llvm::SmallString<1024> JsonDatabasePath(JsonDatabaseDirectory);
> +  llvm::sys::path::append(JsonDatabasePath, "compile_commands.json");
> +  llvm::OwningPtr<llvm::MemoryBuffer> JsonDatabase;
> +  llvm::error_code Result =
> +      llvm::MemoryBuffer::getFile(JsonDatabasePath, JsonDatabase);
> +  if (Result != 0) {
> +    llvm::outs() << "Error while opening JSON database: " << Result.message()
> +                 << "\n";
> +    exit(1);
> +  }
> +  llvm::StringRef BaseDirectory(::getenv("PWD"));
> +  for (unsigned I = 0; I < TranslationUnits.size(); ++I) {
> +    llvm::SmallString<1024> File(getAbsolutePath(
> +        TranslationUnits[I], BaseDirectory));
> +    std::string ErrorMessage;
> +    clang::tooling::CompileCommand LookupResult =
> +        clang::tooling::findCompileArgsInJsonDatabase(
> +            File.str(), JsonDatabase->getBuffer(), ErrorMessage);
> +    if (!ErrorMessage.empty()) {
> +      llvm::outs() << "Error while parsing JSON database: " << ErrorMessage
> +                   << "\n";
> +      exit(1);
> +    }
> +    if (!LookupResult.CommandLine.empty()) {
> +      if (!LookupResult.Directory.empty()) {
> +        // FIXME: What should happen if CommandLine includes -working-directory
> +        // as well?
> +        LookupResult.CommandLine.push_back(
> +            "-working-directory=" + LookupResult.Directory);
> +      }
> +      CommandLines.push_back(make_pair(File.str(), LookupResult.CommandLine));
> +    } 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::outs() << "Skipping " << File << ". Command line not found.\n";
> +    }
> +  }
> +}
> +
> +void ClangTool::mapVirtualFiles(
> +    const std::map<std::string, std::string> &FileContents) {
At first glance, MappedFileContents in both ToolInvocation and ClangTool
looks like it can just be a std::map<StringRef,StringRef>. Does it really need
to have ownership of its own malloc'd copy of the file contents?

> +  MappedFileContents.insert(FileContents.begin(), FileContents.end());
> +}
> +
> +int ClangTool::run(FrontendActionFactory *ActionFactory) {
It also looks like the ClangTool class isn't covered by the unittest, either.

Looking forward to this landing!


> On Wed, Jan 25, 2012 at 3:59 PM, Manuel Klimek <klimek at google.com> wrote:
>> On Tue, Jan 24, 2012 at 4:43 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>> On Tue, 2012-01-24 at 11:03 +0100, Manuel Klimek wrote:
>>>> The attached patch adds support to run clang tools (FrontendActions)
>>>> as standalone tools, or repeatedly in-memory in a process.
>>>> This is useful for unit testing, map-reduce-style applications, source
>>>> transformation daemons, and command line tools.
>>>
>>> I am also interested in having this kind of functionality. A few quick
>>> comments:
>>>
>>> 1. The coding standards say that function names should begin with a
>>> lower-case letter.
>>
>> Done. I jumped on the opportunity to dogfood refactoring support in
>> our current tooling branch and wrote a script that changed all
>> incorrectly named functions automatically (and created a sed-script to
>> post-produce comment changes, which made me notice a bug in a
>> comment).
>>
>>> 2. The comments contain several references to CMake; what, if anything,
>>> in this patch is tied to CMake, or designed to be compatible with CMake?
>>>
>>> 2b.
>>>
>>>
>>>> +/// \param JsonDatabase A JSON formatted list of compile commands.
>>>> This lookup
>>>> +/// command supports only a subset of the JSON standard as written by
>>>> CMake.
>>>>
>>>
>>> Please be more verbose here. What is not supported?
>>>
>>> Generally, I think that it would be helpful for you to provide a
>>> paragraph or two explaining how this extension is to be used, what kind
>>> of things can be specified in JSON inputs, how this ties into CMake (or
>>> not), etc. with a few small examples. Some of this can be gleaned from
>>> the test case, but some nicely-formatted text (without all of the
>>> escaping) would, IMHO, be earlier to read.
>>
>> Hopefully better expressed now. Please let me know if you want more /
>> different details.
>>
>> Thanks a lot for the review!
>> /Manuel
>>
>>>
>>>  -Hal
>>>
>>>>
>>>> Cheers,
>>>> /Manuel
>>>>
>>>> Rietveld link:
>>>> http://codereview.appspot.com/5570054/
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>> --
>>> Hal Finkel
>>> Postdoctoral Appointee
>>> Leadership Computing Facility
>>> Argonne National Laboratory
>>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>