modularize - header dependencies - version 2

[jtsoftware]

Summary

Here's another crack at it using the options subsystems this time. (Thanks for the suggestions Sean and Manuel!)

The original summary:

In using modularize to check a large group of platform headers for modules-readiness, I found that a few headers had dependencies, such that they required other headers to be included first to avoid compile errors on missing definitions.

This patch adds support to modularize to allow specifying depended-on headers in the header file list input to modularize, i.e.

header.h: dependency1.h dependency2.h

[klimek]

I'm prolly not the right person to review whether this patch makes sense overall, but for the part that parses the options, that looks fine to me. Would probably be nice to put findInputFile (perhaps in the plural) somewhere more accessible...

[silvas]

Does this patch make sense from a design standpoint? Aren't these kinds of implicit dependencies the things that modularize is trying to get rid of in the first place? Why would we expend effort to enshrine them, when we can just detect them and emit an error?

I've also made a few comments about the code, but I think the above design issue is more important.

modularize/Modularize.cpp

281

Is the quoting here actually necessary? I don't think that there is a shell-escaping step after this.

287–302

Does this function actually use any of the class members? It seems like it should be a free function.

292–294

clang-format

[doug.gregor]

The implementation looks fine.

Regarding Sean's question: applying modularize to a set of system headers can encounter a huge number of problems, not all of which are easy to fix. Adding a feature like this to modularize can help someone work around issues in the headers (which can also be worked around in the module map by listing headers in the appropriate order) to get better results from modularize with fewer widespread changes. For that reason, I think this is a useful feature to have. If we could fix all of the world's system headers, we would :(

[jtsoftware]

Committed in r189837 (and fixes r189983 r189984).

modularize/Modularize.cpp

281

It's needed because the path might have spaces (i.e. the default Sony sdk path does).

287–302

Got it.

292–294

Got it.

modularize/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   asmparser
   support
   mc
   )
 
 add_clang_executable(modularize
   Modularize.cpp
   PreprocessorTracker.cpp
   )
 
 target_link_libraries(modularize
   clangTooling
   clangBasic
+  clangDriver
   clangRewriteFrontend
   )

modularize/Modularize.cpp

 // multiple headers or whether a header produces different definitions under
 // different circumstances. These conditions cause modules built from the
 // headers to behave poorly, and should be fixed before introducing a module
 // map.
 //
 // Modularize takes as argument a file name for a file containing the
 // newline-separated list of headers to check with respect to each other.
 // Lines beginning with '#' and empty lines are ignored.
+// Header file names followed by a colon and other space-separated
+// file names will include those extra files as dependencies.
+// The file names can be relative or full paths, but must be on the
+// same line.
+//
 // Modularize also accepts regular front-end arguments.
 //
 // Usage:   modularize [-prefix (optional header path prefix)]
 //   (include-files_list) [(front-end-options) ...]
 //
 // Note that unless a "-prefix (header path)" option is specified,
 // non-absolute file paths in the header list file will be relative
 // to the header list file directory.  Use -prefix to specify a different

 // there is some problem with them, or the messages get too verbose.
 //
 // 3. Try to figure out the preprocessor conditional directives that
 // contribute to problems and tie them to the inconsistent definitions.
 //
 // 4. Check for correct and consistent usage of extern "C" {} and other
 // directives. Warn about #include inside extern "C" {}.
 //
-// 5. To support headers that depend on other headers to be included first
-// add support for a dependency list to the header list input.
-// I.e.: header.h: dependent1.h dependent2.h
-// (Implement using clang's "-include" option"?)
-//
-// 6. What else?
+// 5. What else?
 //
 // General clean-up and refactoring:
 //
 // 1. The Location class seems to be something that we might
 // want to design to be applicable to a wider range of tools, and stick it
 // somewhere into Tooling/ in mainline
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Driver/Options.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/OwningPtr.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Config/config.h"
+#include "llvm/Option/Arg.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/OptTable.h"
+#include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include <algorithm>
 #include <fstream>
 #include <iterator>
 #include <string>
 #include <vector>
 #include "PreprocessorTracker.h"
 
-using namespace clang::tooling;
 using namespace clang;
+using namespace clang::driver;
+using namespace clang::driver::options;
+using namespace clang::tooling;
 using namespace llvm;
+using namespace llvm::opt;
 using namespace Modularize;
 
 // Option to specify a file name for a list of header files to check.
 cl::opt<std::string>
 ListFileName(cl::Positional,
              cl::desc("<name of file containing list of headers to check>"));
 
 // Collect all other arguments, which will be passed to the front end.
 cl::list<std::string>
 CC1Arguments(cl::ConsumeAfter,
              cl::desc("<arguments to be passed to front end>..."));
 
 // Option to specify a prefix to be prepended to the header names.
 cl::opt<std::string> HeaderPrefix(
     "prefix", cl::init(""),
     cl::desc(
         "Prepend header file paths with this prefix."
         " If not specified,"
         " the files are considered to be relative to the header list file."));
 
-// Read the header list file and collect the header file names.
-error_code getHeaderFileNames(SmallVectorImpl<std::string> &HeaderFileNames,
-                              StringRef ListFileName, StringRef HeaderPrefix) {
+typedef SmallVector<std::string, 4> DependentsVector;
+typedef StringMap<DependentsVector> DependencyMap;
 
+// Read the header list file and collect the header file names and
+// optional dependencies.
+error_code getHeaderFileNames(SmallVectorImpl<std::string> &HeaderFileNames,
+                              DependencyMap &Dependencies,
+                              StringRef ListFileName,
+                              StringRef HeaderPrefix) {
   // By default, use the path component of the list file name.
   SmallString<256> HeaderDirectory(ListFileName);
   sys::path::remove_filename(HeaderDirectory);
+  SmallString<256> CurrentDirectory;
+  sys::fs::current_path(CurrentDirectory);
 
   // Get the prefix if we have one.
   if (HeaderPrefix.size() != 0)
     HeaderDirectory = HeaderPrefix;
 
   // Read the header list file into a buffer.
   OwningPtr<MemoryBuffer> listBuffer;
   if (error_code ec = MemoryBuffer::getFile(ListFileName, listBuffer)) {
     return ec;
   }
 
   // Parse the header list into strings.
   SmallVector<StringRef, 32> Strings;
   listBuffer->getBuffer().split(Strings, "\n", -1, false);
 
   // Collect the header file names from the string list.
   for (SmallVectorImpl<StringRef>::iterator I = Strings.begin(),
                                             E = Strings.end();
        I != E; ++I) {
-    StringRef Line = (*I).trim();
+    StringRef Line = I->trim();
     // Ignore comments and empty lines.
     if (Line.empty() || (Line[0] == '#'))
       continue;
+    std::pair<StringRef, StringRef> TargetAndDependents = Line.split(':');
     SmallString<256> HeaderFileName;
     // Prepend header file name prefix if it's not absolute.
-    if (sys::path::is_absolute(Line))
-      HeaderFileName = Line;
+    if (sys::path::is_absolute(TargetAndDependents.first))
+      llvm::sys::path::native(TargetAndDependents.first, HeaderFileName);
     else {
+      if (HeaderDirectory.size() != 0)
         HeaderFileName = HeaderDirectory;
-      sys::path::append(HeaderFileName, Line);
+      else
+        HeaderFileName = CurrentDirectory;
+      sys::path::append(HeaderFileName, TargetAndDependents.first);
+      llvm::sys::path::native(HeaderFileName.str(), HeaderFileName);
+    }
+    // Handle optional dependencies.
+    DependentsVector Dependents;
+    SmallVector<StringRef, 4> DependentsList;
+    TargetAndDependents.second.split(DependentsList, " ", -1, false);
+    int Count = DependentsList.size();
+    for (int Index = 0; Index < Count; ++Index) {
+      SmallString<256> Dependent;
+      if (sys::path::is_absolute(DependentsList[Index]))
+        Dependent = DependentsList[Index];
+      else {
+        if (HeaderDirectory.size() != 0)
+          Dependent = HeaderDirectory;
+        else
+          Dependent = CurrentDirectory;
+        sys::path::append(Dependent, DependentsList[Index]);
       }
-    // Save the resulting header file path.
+      llvm::sys::path::native(Dependent.str(), Dependent);
+      Dependents.push_back(Dependent.str());
+    }
+    // Save the resulting header file path and dependencies.
     HeaderFileNames.push_back(HeaderFileName.str());
+    Dependencies[HeaderFileName.str()] = Dependents;
   }
 
   return error_code::success();
 }
 
+// We provide this derivation to add in "-include (file)" arguments for header
+// dependencies.
+class AddDependenciesAdjuster : public ArgumentsAdjuster {
+public:
+  AddDependenciesAdjuster(DependencyMap &Dependencies)
+    : Dependencies(Dependencies) {}
+private:
+  // Callback for adjusting commandline arguments.
+  CommandLineArguments Adjust(const CommandLineArguments &Args) {
+    llvm::StringRef InputFile = findInputFile(Args);
+    DependentsVector &FileDependents = Dependencies[InputFile];
+    int Count = FileDependents.size();
+    if (Count == 0)
+      return Args;
+    CommandLineArguments NewArgs(Args);
+    for (int Index = 0; Index < Count; ++Index) {
+      NewArgs.push_back("-include");
+      std::string File(
+        std::string("\"") + FileDependents[Index] + std::string("\""));
+      NewArgs.push_back(FileDependents[Index]);
+    }
+    return NewArgs;
+  }
+  // Helper function for finding the input file in an arguments list.
+  llvm::StringRef findInputFile(const CommandLineArguments &CLArgs) {
+    OwningPtr<OptTable> Opts(createDriverOptTable());
+    const unsigned IncludedFlagsBitmask = options::CC1Option;
+    unsigned MissingArgIndex, MissingArgCount;
+    SmallVector<const char*, 256> Argv;
+    for (CommandLineArguments::const_iterator I = CLArgs.begin(),
+          E = CLArgs.end();
+        I != E; ++I)
+      Argv.push_back(I->c_str());
+    OwningPtr<InputArgList> Args(
+      Opts->ParseArgs(Argv.data(), Argv.data() + Argv.size(),
+                      MissingArgIndex, MissingArgCount,
+                      IncludedFlagsBitmask));
+    std::vector<std::string> Inputs = Args->getAllArgValues(OPT_INPUT);
+    return Inputs.back();
+  }
+  DependencyMap &Dependencies;
+};
+
 // FIXME: The Location class seems to be something that we might
 // want to design to be applicable to a wider range of tools, and stick it
 // somewhere into Tooling/ in mainline
 struct Location {
   const FileEntry *File;
   unsigned Line, Column;
 
   Location() : File(), Line(), Column() {}
int main(int Argc, const char **Argv) {
   cl::ParseCommandLineOptions(Argc, Argv, "modularize.\n");
 
   // No go if we have no header list file.
   if (ListFileName.size() == 0) {
     cl::PrintHelpMessage();
     return 1;
   }
 
-  // Get header file names.
+  // Get header file names and dependencies.
   SmallVector<std::string, 32> Headers;
-  if (error_code EC = getHeaderFileNames(Headers, ListFileName, HeaderPrefix)) {
+  DependencyMap Dependencies;
+  if (error_code EC = getHeaderFileNames(
+      Headers, Dependencies, ListFileName, HeaderPrefix)) {
     errs() << Argv[0] << ": error: Unable to get header list '" << ListFileName
            << "': " << EC.message() << '\n';
     return 1;
   }
   
   // Create the compilation database.
   SmallString<256> PathBuf;
   sys::fs::current_path(PathBuf);
   OwningPtr<CompilationDatabase> Compilations;
   Compilations.reset(
       new FixedCompilationDatabase(Twine(PathBuf), CC1Arguments));
 
   // Create preprocessor tracker, to watch for macro and conditional problems.
   OwningPtr<PreprocessorTracker> PPTracker(PreprocessorTracker::create());
 
   // Parse all of the headers, detecting duplicates.
   EntityMap Entities;
   ClangTool Tool(*Compilations, Headers);
+  Tool.appendArgumentsAdjuster(new AddDependenciesAdjuster(Dependencies));
   int HadErrors =
       Tool.run(new ModularizeFrontendActionFactory(Entities, *PPTracker));
 
   // Create a place to save duplicate entity locations, separate bins per kind.
   typedef SmallVector<Location, 8> LocationArray;
   typedef SmallVector<LocationArray, Entry::EK_NumberOfKinds> EntryBinArray;
   EntryBinArray EntryBins;
   int KindIndex;

test/modularize/Inputs/IsDependent.h

+// This header depends on SomeTypes.h for the TypeInt typedef.
+
+typedef TypeInt NewTypeInt;
+typedef OtherTypeInt OtherNewTypeInt;

test/modularize/Inputs/SomeOtherTypes.h

+// Declare another type for the dependency check.
+// This file dependent on SomeTypes.h being included first.
+
+typedef TypeInt OtherTypeInt;

test/modularize/NoProblemsDependencies.modularize

+# RUN: modularize %s -x c++
+
+Inputs/IsDependent.h: Inputs/SomeTypes.h Inputs/SomeOtherTypes.h