Draft of #327 code fixes

Author: mattmccutchen-cci

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

@@ -25,7 +25,6 @@ extern bool EnablePropThruIType;
 extern bool ConsiderAllocUnsafe;
 extern bool AllTypes;
 extern bool NewSolver;
-extern std::string WorkingDir;
 extern std::string BaseDir;
 extern bool AllowSourcesOutsideBaseDir;
 extern std::vector<std::string> AllocatorFunctions;

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

@@ -104,7 +104,19 @@ bool hasFunctionBody(clang::Decl *D);
 
 std::string getStorageQualifierString(clang::Decl *D);
 
-void 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 variant 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 may
+// wish to 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

@@ -49,7 +49,6 @@ std::string StatsOutputJson;
 std::string WildPtrInfoJson;
 std::string PerWildPtrInfoJson;
 bool AllTypes;
-std::string WorkingDir;
 std::string BaseDir;
 bool AllowSourcesOutsideBaseDir;
 bool AddCheckedRegions;
@@ -250,32 +249,79 @@ _3CInterface::_3CInterface(const struct _3COptions &CCopt,
 
   ConstraintsBuilt = false;
 
-  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.");
+  int NumOutputOptions = 0;
+  if (CCopt.OutputPostfix != "-")
+    NumOutputOptions++;
+  if (CCopt.OutputDir != "")
+    NumOutputOptions++;
+  if (NumOutputOptions > 1) {
+    errs() << "3C initialization error: Cannot use both -output-postfix and "
+              "-output-dir\n";
+    Failed = true;
+    return;
+  }
+  if (NumOutputOptions == 0 && SourceFileList.size() > 1) {
+    errs() << "3C initialization error: Cannot specify more than one input "
+              "file when output is to stdout\n";
+    Failed = true;
+    return;
   }
-  WorkingDir = Cp.str();
+
+  std::string TmpPath;
+  std::error_code EC;
 
   if (BaseDir.empty()) {
-    BaseDir = WorkingDir;
-  } else {
-    // Get the absolute path of the base directory.
-    std::string TmpPath = BaseDir;
-    getAbsoluteFilePath(BaseDir, TmpPath);
-    BaseDir = TmpPath;
+    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;
+
+  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;
-    getAbsoluteFilePath(S, 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 (AbsPath.substr(0, BaseDir.size()) != BaseDir) {
+    if (!filePathStartsWith(AbsPath, BaseDir)) {
       errs()
-          << "3C command-line usage "
+          << "3C initialization "
           << (OutputDir != "" || !AllowSourcesOutsideBaseDir ? "error"
                                                              : "warning")
           << ": File \"" << AbsPath
@@ -299,22 +345,6 @@ _3CInterface::_3CInterface(const struct _3COptions &CCopt,
   }
 
   CurrCompDB = CompDB;
-
-  int NumOutputOptions = 0;
-  if (CCopt.OutputPostfix != "-")
-    NumOutputOptions++;
-  if (CCopt.OutputDir != "")
-    NumOutputOptions++;
-  if (NumOutputOptions > 1) {
-    errs() << "3C command-line usage error: Cannot use both -output-postfix "
-              "and -output-dir\n";
-    Failed = true;
-  }
-  if (NumOutputOptions == 0 && SourceFileList.size() > 1) {
-    errs() << "3C command-line usage error: Cannot specify more than one input "
-              "file when output is to stdout\n";
-    Failed = true;
-  }
 }
 
 bool _3CInterface::buildInitialConstraints() {

clang/lib/3C/PersistentSourceLoc.cpp

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

clang/lib/3C/RewriteUtils.cpp

@@ -164,7 +164,7 @@ static void emit(Rewriter &R, ASTContext &C) {
 
       // Check whether we are allowed to write this file.
       std::string FeAbsS = "";
-      getAbsoluteFilePath(FE->getName(), FeAbsS);
+      getCanonicalFilePath(FE->getName(), FeAbsS);
       if (!canWrite(FeAbsS)) {
         DiagnosticsEngine &DE = C.getDiagnostics();
         unsigned ID = DE.getCustomDiagID(
@@ -216,8 +216,13 @@ static void emit(Rewriter &R, ASTContext &C) {
         assert(OutputDir != "");
         // If this does not hold when OutputDir is set, it should have been a
         // fatal error in the _3CInterface constructor.
-        assert(FeAbsS.substr(0, BaseDir.size()) == BaseDir);
-        NFile = OutputDir + FeAbsS.substr(BaseDir.size());
+        assert(filePathStartsWith(FeAbsS, BaseDir));
+        // replace_path_prefix is not smart about separators, but this should be
+        // OK because getCanonicalFilePath should ensure that neither BaseDir
+        // nor OutputDir has a trailing separator.
+        SmallString<255> Tmp(FeAbsS);
+        llvm::sys::path::replace_path_prefix(Tmp, BaseDir, OutputDir);
+        NFile = Tmp.str();
         EC = llvm::sys::fs::create_directories(sys::path::parent_path(NFile));
         if (EC) {
           DiagnosticsEngine &DE = C.getDiagnostics();

clang/lib/3C/Utils.cpp

@@ -12,6 +12,7 @@
 #include "clang/3C/3CGlobalOptions.h"
 #include "clang/3C/ConstraintVariables.h"
 #include "llvm/Support/Path.h"
+#include <errno.h>
 
 using namespace llvm;
 using namespace clang;
@@ -142,12 +143,44 @@ bool isNULLExpression(clang::Expr *E, ASTContext &C) {
          E->isNullPointerConstant(C, Expr::NPC_ValueDependentIsNotNull);
 }
 
-void getAbsoluteFilePath(std::string FileName, std::string &AbsoluteFp) {
+std::error_code tryGetCanonicalFilePath(const std::string &FileName, std::string &AbsoluteFp) {
   // Get absolute path of the provided file
   // returns true if successful else false.
-  SmallString<255> AbsPath(FileName);
-  llvm::sys::fs::make_absolute(WorkingDir, AbsPath);
+  SmallString<255> AbsPath;
+  std::error_code EC;
+  if (FileName.empty()) {
+    // Strangely, llvm::sys::fs::real_path successfully returns the empty string
+    // in this case.
+    EC = std::error_code(ENOENT, std::generic_category());
+  } else {
+    EC = llvm::sys::fs::real_path(FileName, AbsPath);
+  }
+  if (EC) {
+    return EC;
+  }
   AbsoluteFp = AbsPath.str();
+  return EC;
+}
+
+void getCanonicalFilePath(const std::string &FileName, std::string &AbsoluteFp) {
+  std::error_code EC = tryGetCanonicalFilePath(FileName, AbsoluteFp);
+  assert(!EC && "tryGetCanonicalFilePath failed");
+}
+
+bool filePathStartsWith(const std::string &Path, const std::string &Prefix) {
+  // If the path exactly equals the prefix, don't ruin it by appending a
+  // separator to the prefix. (This may not happen in 3C yet, but let's get it
+  // right.)
+  if (Prefix.empty() || Path == Prefix) {
+    return true;
+  }
+  StringRef separator = llvm::sys::path::get_separator();
+  std::string PrefixWithTrailingSeparator = Prefix;
+  if (!StringRef(Prefix).endswith(separator)) {
+    PrefixWithTrailingSeparator += separator;
+  }
+  return Path.substr(0, PrefixWithTrailingSeparator.size()) ==
+         PrefixWithTrailingSeparator;
 }
 
 bool functionHasVarArgs(clang::FunctionDecl *FD) {
@@ -337,9 +370,7 @@ bool canWrite(const std::string &FilePath) {
     return true;
   // Get the absolute path of the file and check that
   // the file path starts with the base directory.
-  std::string FileAbsPath = FilePath;
-  getAbsoluteFilePath(FilePath, FileAbsPath);
-  return FileAbsPath.rfind(BaseDir, 0) == 0;
+  return filePathStartsWith(FilePath, BaseDir);
 }
 
 bool isInSysHeader(clang::Decl *D) {