Add -output-dir and other file handling improvements (#412)

- Add -output-dir option to write updated files to a directory structure
  parallel to the base dir (#347). When -output-dir is used, a source
  file outside the base dir can't be handled because there is no way to
  compute its output path. For consistency, this is now an error even
  when -output-dir is not used.

- Convert all 3C regression tests from -output-postfix to -output-dir to
  avoid leaving temporary files in the clang/test/3C directory (#378).

- Expand "3c -help" documentation. In particular, direct the user to pass
  "--" when they don't want to use a compilation database to avoid
  accidentally using unwanted compiler options and suppress the warning
  if no compilation database is found (#343).

- For consistency, have stdout mode output the main file even if it is
  unchanged (#328).

- Fix bugs in matching of file paths against the base dir (#327).

- Other minor bug fixes: see the pull request description for details.

Co-authored-by: John Kastner <[email protected]>

Author: mattmccutchen-cci

clang-tools-extra/clangd/tool/ClangdMain.cpp

@@ -486,8 +486,16 @@ int main(int argc, char *argv[]) {
 
   // See clang/docs/checkedc/3C/clang-tidy.md#_3c-name-prefix
   // NOLINTNEXTLINE(readability-identifier-naming)
-  _3CInterface _3CInterface(CcOptions, OptionsParser.getSourcePathList(),
-                            &(OptionsParser.getCompilations()));
+  std::unique_ptr<_3CInterface> _3CInterfacePtr(
+      _3CInterface::create(CcOptions, OptionsParser.getSourcePathList(),
+                           &(OptionsParser.getCompilations())));
+  if (!_3CInterfacePtr) {
+    // _3CInterface::create has already printed an error message. Just exit.
+    return 1;
+  }
+  // See clang/docs/checkedc/3C/clang-tidy.md#_3c-name-prefix
+  // NOLINTNEXTLINE(readability-identifier-naming)
+  _3CInterface &_3CInterface = *_3CInterfacePtr;
 #else
   llvm::cl::ParseCommandLineOptions(
       argc, argv,

clang/include/clang/3C/3C.h

@@ -32,6 +32,7 @@ struct _3COptions {
   bool Verbose;
 
   std::string OutputPostfix;
+  std::string OutputDir;
 
   std::string ConstraintOutputJson;
 
@@ -50,6 +51,7 @@ struct _3COptions {
   bool EnablePropThruIType;
 
   std::string BaseDir;
+  bool AllowSourcesOutsideBaseDir;
 
   bool EnableAllTypes;
 
@@ -86,9 +88,19 @@ class _3CInterface {
   // Mutex for this interface.
   std::mutex InterfaceMutex;
 
-  _3CInterface(const struct _3COptions &CCopt,
-               const std::vector<std::string> &SourceFileList,
-               clang::tooling::CompilationDatabase *CompDB);
+  // If the parameters are invalid, this function prints an error message to
+  // stderr and returns null.
+  //
+  // There's no way for a constructor to report failure (we do not use
+  // exceptions), so use a factory method instead. Ideally we'd use an
+  // "optional" datatype that doesn't force heap allocation, but the only such
+  // datatype that is accepted in our codebase
+  // (https://llvm.org/docs/ProgrammersManual.html#fallible-constructors) seems
+  // too unwieldy to use right now.
+  static std::unique_ptr<_3CInterface> create(
+      const struct _3COptions &CCopt,
+      const std::vector<std::string> &SourceFileList,
+      clang::tooling::CompilationDatabase *CompDB);
 
   // Constraint Building.
 
@@ -121,6 +133,10 @@ class _3CInterface {
   bool writeConvertedFileToDisk(const std::string &FilePath);
 
 private:
+  _3CInterface(const struct _3COptions &CCopt,
+               const std::vector<std::string> &SourceFileList,
+               clang::tooling::CompilationDatabase *CompDB, bool &Failed);
+
   // Are constraints already built?
   bool ConstraintsBuilt;
   void invalidateAllConstraintsWithReason(Constraint *ConstraintToRemove);

clang/include/clang/3C/3CGlobalOptions.h

@@ -17,6 +17,8 @@
 
 extern bool Verbose;
 extern bool DumpIntermediate;
+extern std::string OutputPostfix;
+extern std::string OutputDir;
 extern bool HandleVARARGS;
 extern bool SeperateMultipleFuncDecls;
 extern bool EnablePropThruIType;

clang/include/clang/3C/RewriteUtils.h

@@ -238,15 +238,13 @@ class ArrayBoundsRewriter {
 
 class RewriteConsumer : public ASTConsumer {
 public:
-  explicit RewriteConsumer(ProgramInfo &I, std::string &OPostfix)
-      : Info(I), OutputPostfix(OPostfix) {}
+  explicit RewriteConsumer(ProgramInfo &I) : Info(I) {}
 
   virtual void HandleTranslationUnit(ASTContext &Context);
 
 private:
   ProgramInfo &Info;
   static std::map<std::string, std::string> ModifiedFuncSignatures;
-  std::string &OutputPostfix;
 
   // A single header file can be included in multiple translations units. This
   // set ensures that the diagnostics for a header file are not emitted each

clang/include/clang/3C/Utils.h

@@ -104,7 +104,19 @@ bool hasFunctionBody(clang::Decl *D);
 
 std::string getStorageQualifierString(clang::Decl *D);
 
-bool getAbsoluteFilePath(std::string FileName, std::string &AbsoluteFp);
+// Use this version for user input that has not yet been validated.
+std::error_code tryGetCanonicalFilePath(const std::string &FileName,
+                                        std::string &AbsoluteFp);
+
+// This version asserts success. It may be used for convenience for files we are
+// already accessing and thus believe should exist, modulo race conditions and
+// the like.
+void getCanonicalFilePath(const std::string &FileName, std::string &AbsoluteFp);
+
+// This compares entire path components: it's smart enough to know that "foo.c"
+// does not start with "foo". It's not smart about anything else, so you should
+// probably put both paths through getCanonicalFilePath first.
+bool filePathStartsWith(const std::string &Path, const std::string &Prefix);
 
 bool isNULLExpression(clang::Expr *E, clang::ASTContext &C);
 
@@ -147,6 +159,9 @@ bool isCastSafe(clang::QualType DstType, clang::QualType SrcType);
 
 // Check if the provided file path belongs to the input project
 // and can be rewritten.
+//
+// For accurate results, the path must have gone through getCanonicalFilePath.
+// Note that the file name of a PersistentSourceLoc is always canonical.
 bool canWrite(const std::string &FilePath);
 
 // Check if the provided variable has void as one of its type.

clang/lib/3C/3C.cpp

@@ -38,6 +38,7 @@ static cl::opt<bool>
 bool DumpIntermediate;
 bool Verbose;
 std::string OutputPostfix;
+std::string OutputDir;
 std::string ConstraintOutputJson;
 std::vector<std::string> AllocatorFunctions;
 bool DumpStats;
@@ -89,7 +90,7 @@ class RewriteAction : public ASTFrontendAction {
 
   virtual std::unique_ptr<ASTConsumer>
   CreateASTConsumer(CompilerInstance &Compiler, StringRef InFile) {
-    return std::unique_ptr<ASTConsumer>(new T(Info, OutputPostfix));
+    return std::unique_ptr<ASTConsumer>(new T(Info));
   }
 
 private:
@@ -194,13 +195,27 @@ void runSolver(ProgramInfo &Info, std::set<std::string> &SourceFiles) {
   }
 }
 
+std::unique_ptr<_3CInterface>
+_3CInterface::create(const struct _3COptions &CCopt,
+                     const std::vector<std::string> &SourceFileList,
+                     CompilationDatabase *CompDB) {
+  bool Failed = false;
+  std::unique_ptr<_3CInterface> _3CInter(
+      new _3CInterface(CCopt, SourceFileList, CompDB, Failed));
+  if (Failed) {
+    return nullptr;
+  }
+  return _3CInter;
+}
+
 _3CInterface::_3CInterface(const struct _3COptions &CCopt,
                            const std::vector<std::string> &SourceFileList,
-                           CompilationDatabase *CompDB) {
+                           CompilationDatabase *CompDB, bool &Failed) {
 
   DumpIntermediate = CCopt.DumpIntermediate;
   Verbose = CCopt.Verbose;
   OutputPostfix = CCopt.OutputPostfix;
+  OutputDir = CCopt.OutputDir;
   ConstraintOutputJson = CCopt.ConstraintOutputJson;
   StatsOutputJson = CCopt.StatsOutputJson;
   WildPtrInfoJson = CCopt.WildPtrInfoJson;
@@ -231,35 +246,97 @@ _3CInterface::_3CInterface(const struct _3COptions &CCopt,
 
   ConstraintsBuilt = false;
 
-  // Get the absolute path of the base directory.
-  std::string TmpPath = BaseDir;
-  getAbsoluteFilePath(BaseDir, TmpPath);
-  BaseDir = TmpPath;
+  if (OutputPostfix != "-" && !OutputDir.empty()) {
+    errs() << "3C initialization error: Cannot use both -output-postfix and "
+              "-output-dir\n";
+    Failed = true;
+    return;
+  }
+  if (OutputPostfix == "-" && OutputDir.empty() && SourceFileList.size() > 1) {
+    errs() << "3C initialization error: Cannot specify more than one input "
+              "file when output is to stdout\n";
+    Failed = true;
+    return;
+  }
+
+  std::string TmpPath;
+  std::error_code EC;
 
   if (BaseDir.empty()) {
-    SmallString<256> Cp;
-    if (std::error_code Ec = sys::fs::current_path(Cp)) {
-      errs() << "could not get current working dir\n";
-      assert(false && "Unable to get determine working directory.");
-    }
+    BaseDir = ".";
+  }
+
+  // Get the canonical path of the base directory.
+  TmpPath = BaseDir;
+  EC = tryGetCanonicalFilePath(BaseDir, TmpPath);
+  if (EC) {
+    errs() << "3C initialization error: Failed to canonicalize base directory "
+              "\"" << BaseDir << "\": " << EC.message() << "\n";
+    Failed = true;
+    return;
+  }
+  BaseDir = TmpPath;
 
-    BaseDir = Cp.str();
+  if (!OutputDir.empty()) {
+    // tryGetCanonicalFilePath will fail if the output dir doesn't exist yet, so
+    // create it first.
+    EC = llvm::sys::fs::create_directories(OutputDir);
+    if (EC) {
+      errs() << "3C initialization error: Failed to create output directory \""
+             << OutputDir << "\": " << EC.message() << "\n";
+      Failed = true;
+      return;
+    }
+    TmpPath = OutputDir;
+    EC = tryGetCanonicalFilePath(OutputDir, TmpPath);
+    if (EC) {
+      errs() << "3C initialization error: Failed to canonicalize output "
+                "directory \"" << OutputDir << "\": " << EC.message() << "\n";
+      Failed = true;
+      return;
+    }
+    OutputDir = TmpPath;
   }
 
   SourceFiles = SourceFileList;
 
+  bool SawInputOutsideBaseDir = false;
   for (const auto &S : SourceFiles) {
     std::string AbsPath;
-    if (getAbsoluteFilePath(S, AbsPath))
-      FilePaths.insert(AbsPath);
+    EC = tryGetCanonicalFilePath(S, AbsPath);
+    if (EC) {
+      errs() << "3C initialization error: Failed to canonicalize source file "
+                "path \"" << S << "\": " << EC.message() << "\n";
+      Failed = true;
+      continue;
+    }
+    FilePaths.insert(AbsPath);
+    if (!filePathStartsWith(AbsPath, BaseDir)) {
+      errs()
+          << "3C initialization "
+          << (OutputDir != "" || !CCopt.AllowSourcesOutsideBaseDir ? "error"
+                                                                   : "warning")
+          << ": File \"" << AbsPath
+          << "\" specified on the command line is outside the base directory\n";
+      SawInputOutsideBaseDir = true;
+    }
+  }
+  if (SawInputOutsideBaseDir) {
+    errs() << "The base directory is currently \"" << BaseDir
+           << "\" and can be changed with the -base-dir option.\n";
+    if (OutputDir != "") {
+      Failed = true;
+      errs() << "When using -output-dir, input files outside the base "
+                "directory cannot be handled because there is no way to "
+                "compute their output paths.\n";
+    } else if (!CCopt.AllowSourcesOutsideBaseDir) {
+      Failed = true;
+      errs() << "You can use the -allow-sources-outside-base-dir option to "
+                "temporarily downgrade this error to a warning.\n";
+    }
   }
 
   CurrCompDB = CompDB;
-
-  if (OutputPostfix == "-" && FilePaths.size() > 1) {
-    errs() << "If rewriting more than one , can't output to stdout\n";
-    assert(false && "Rewriting more than one files requires OutputPostfix");
-  }
 }
 
 bool _3CInterface::buildInitialConstraints() {
@@ -295,15 +372,15 @@ bool _3CInterface::solveConstraints() {
                              "build constraint before trying to solve them.");
   // 2. Solve constraints.
   if (Verbose)
-    outs() << "Solving constraints\n";
+    errs() << "Solving constraints\n";
 
   if (DumpIntermediate)
     GlobalProgramInfo.dump();
 
   runSolver(GlobalProgramInfo, FilePaths);
 
   if (Verbose)
-    outs() << "Constraints solved\n";
+    errs() << "Constraints solved\n";
 
   if (WarnRootCause)
     GlobalProgramInfo.computeInterimConstraintState(FilePaths);

clang/lib/3C/ConstraintBuilder.cpp

@@ -660,7 +660,7 @@ void ConstraintBuilderConsumer::HandleTranslationUnit(ASTContext &C) {
   TV.setProgramInfoTypeVars();
 
   if (Verbose)
-    outs() << "Done analyzing\n";
+    errs() << "Done analyzing\n";
 
   Info.exitCompilationUnit();
   return;

clang/lib/3C/PersistentSourceLoc.cpp

@@ -70,8 +70,8 @@ PersistentSourceLoc PersistentSourceLoc::mkPSL(clang::SourceRange SR,
     std::string FeAbsS = "";
     if (Fe != nullptr)
       ToConv = Fe->getName();
-    if (getAbsoluteFilePath(ToConv, FeAbsS))
-      Fn = sys::path::remove_leading_dotslash(FeAbsS);
+    getCanonicalFilePath(ToConv, FeAbsS);
+    Fn = sys::path::remove_leading_dotslash(FeAbsS);
   }
   PersistentSourceLoc PSL(Fn, FESL.getExpansionLineNumber(),
                           FESL.getExpansionColumnNumber(), EndCol);