Constrain the most common constructs in non-writable files to not change. (#391)

This PR addresses function and variable declarations (because they are
the most obvious case and reasonably straightforward) and checked
regions (because they came up in some existing regression tests). We'll
leave #387 open for the tail of unhandled cases.

Also:

- When 3C tries to change a non-writable file, issue an error diagnostic
  (not an assertion failure because there are known unhandled cases)
  rather than silently discarding the change.

- Add a `-dump-unwritable-changes` flag to the `3c` tool to dump the new
  version of the file when that diagnostic appears.

- Add an error diagnostic when 3C tries to change a file under the base
  dir other than the main file in stdout mode. This is a separate
  feature (part of #328) but ended up being easy to implement along with
  the diagnostic for a non-writable file.

- Add tests for all the fixes (but not `-dump-unwritable-changes`).

- Fix a bug where `-warn-all-root-cause` didn't work without
  `-warn-root-cause`, because this affected one of the new tests. The
  use of `-warn-all-root-cause` without `-warn-root-cause` in the
  affected test will serve as a regression test for this fix as well.

Fixes part of #387 and a few unrelated minor issues.

Author: mattmccutchen-cci

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

@@ -70,6 +70,8 @@ struct _3COptions {
   // that generates diagnostics, except for the declaration merging diagnostics
   // that are currently fatal) and uses the default "expected" prefix.
   bool VerifyDiagnosticOutput;
+
+  bool DumpUnwritableChanges;
 };
 
 // The main interface exposed by the 3C to interact with the tool.
@@ -92,9 +94,8 @@ class _3CInterface {
   // Build initial constraints.
   bool buildInitialConstraints();
 
-  // Constraint Solving. The flag: ComputeInterimState requests to compute
-  // interim constraint solver state.
-  bool solveConstraints(bool ComputeInterimState = false);
+  // Constraint Solving.
+  bool solveConstraints();
 
   // Interactivity.
 

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

@@ -28,6 +28,7 @@ extern std::vector<std::string> AllocatorFunctions;
 extern bool AddCheckedRegions;
 extern bool WarnRootCause;
 extern bool WarnAllRootCause;
+extern bool DumpUnwritableChanges;
 
 #ifdef FIVE_C
 extern bool RemoveItypes;

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

@@ -42,7 +42,6 @@ class CheckedRegionAdder
   findParentCompound(const clang::ast_type_traits::DynTypedNode &N, int);
   bool isParentChecked(const clang::ast_type_traits::DynTypedNode &N);
   bool isWrittenChecked(const clang::CompoundStmt *);
-  bool isFunctionBody(clang::CompoundStmt *S);
   clang::ASTContext *Context;
   clang::Rewriter &Writer;
   std::map<llvm::FoldingSetNodeID, AnnotationNeeded> &Map;

clang/lib/3C/3C.cpp

@@ -55,6 +55,7 @@ bool WarnRootCause;
 bool WarnAllRootCause;
 std::set<std::string> FilePaths;
 bool VerifyDiagnosticOutput;
+bool DumpUnwritableChanges;
 
 #ifdef FIVE_C
 bool RemoveItypes;
@@ -214,6 +215,7 @@ _3CInterface::_3CInterface(const struct _3COptions &CCopt,
   WarnRootCause = CCopt.WarnRootCause || CCopt.WarnAllRootCause;
   WarnAllRootCause = CCopt.WarnAllRootCause;
   VerifyDiagnosticOutput = CCopt.VerifyDiagnosticOutput;
+  DumpUnwritableChanges = CCopt.DumpUnwritableChanges;
 
 #ifdef FIVE_C
   RemoveItypes = CCopt.RemoveItypes;
@@ -285,7 +287,7 @@ bool _3CInterface::buildInitialConstraints() {
   return true;
 }
 
-bool _3CInterface::solveConstraints(bool ComputeInterimState) {
+bool _3CInterface::solveConstraints() {
   std::lock_guard<std::mutex> Lock(InterfaceMutex);
   assert(ConstraintsBuilt && "Constraints not yet built. We need to call "
                              "build constraint before trying to solve them.");
@@ -301,7 +303,7 @@ bool _3CInterface::solveConstraints(bool ComputeInterimState) {
   if (Verbose)
     outs() << "Constraints solved\n";
 
-  if (ComputeInterimState)
+  if (WarnRootCause)
     GlobalProgramInfo.computeInterimConstraintState(FilePaths);
 
   if (DumpIntermediate)

clang/lib/3C/CheckedRegions.cpp

@@ -27,15 +27,14 @@ using namespace llvm;
 using namespace clang;
 
 
-// Check if the compound statement is a function body
+// If S is a function body, then return the FunctionDecl, otherwise return null.
 // Used in both visitors so abstracted to a function
-bool isTopLevel(ASTContext *Context, CompoundStmt *S) {
+FunctionDecl *getFunctionDeclOfBody(ASTContext *Context, CompoundStmt *S) {
   const auto &Parents = Context->getParents(*S);
   if (Parents.empty()) {
-    return false;
+    return nullptr;
   }
-  // Ensure that our parent is a functiondecl
-  return Parents[0].get<FunctionDecl>() != nullptr;
+  return const_cast<FunctionDecl *>(Parents[0].get<FunctionDecl>());
 }
 
 // CheckedRegionAdder
@@ -47,7 +46,7 @@ bool CheckedRegionAdder::VisitCompoundStmt(CompoundStmt *S) {
   S->Profile(Id, *Context, true);
   switch (Map[Id]) {
   case IS_UNCHECKED:
-    if (isParentChecked(DTN) && !isFunctionBody(S)) {
+    if (isParentChecked(DTN) && getFunctionDeclOfBody(Context, S) == nullptr) {
       auto Loc = S->getBeginLoc();
       Writer.InsertTextBefore(Loc, "_Unchecked ");
     }
@@ -106,10 +105,6 @@ CheckedRegionAdder::findParentCompound(const ast_type_traits::DynTypedNode &N,
 
 
 
-bool CheckedRegionAdder::isFunctionBody(CompoundStmt *S) {
-  return isTopLevel(Context, S);
-}
-
 bool CheckedRegionAdder::isParentChecked(
     const ast_type_traits::DynTypedNode &DTN) {
   if (const auto *Parent = findParentCompound(DTN).first) {
@@ -163,27 +158,41 @@ bool CheckedRegionFinder::VisitDoStmt(DoStmt *S) {
 }
 
 bool CheckedRegionFinder::VisitCompoundStmt(CompoundStmt *S) {
-  // Visit all subblocks, find all unchecked types
   bool Localwild = false;
+
+  // Is this compound statement the body of a function?
+  FunctionDecl *FD = getFunctionDeclOfBody(Context, S);
+  if (FD != nullptr) {
+    auto PSL = PersistentSourceLoc::mkPSL(FD, *Context);
+    if (!canWrite(PSL.getFileName())) {
+      // The "location" of the function is in an unwritable file. Processing it
+      // might result in modifying an unwritable file, so skip it completely.
+      // This check could have both false positives and false negatives if the
+      // code uses `#include` to assemble a function definition from multiple
+      // files, some writable and some not, but that would be "unusual c code -
+      // low priority".
+      //
+      // Currently, it's OK to perform this check only at the function level
+      // because a function is normally in a single file and 3C doesn't add
+      // checked annotations at higher levels (e.g., `#pragma CHECKED_SCOPE`)
+      return false;
+    }
+
+    // Need to check return type
+    auto retType = FD->getReturnType().getTypePtr();
+    if (retType->isPointerType()) {
+      CVarOption CV = Info.getVariable(FD, Context);
+      Localwild |= isWild(CV) || containsUncheckedPtr(FD->getReturnType());
+    }
+  }
+
+  // Visit all subblocks, find all unchecked types
   for (const auto &SubStmt : S->children()) {
     CheckedRegionFinder Sub(Context, Writer, Info, Seen, Map, EmitWarnings);
     Sub.TraverseStmt(SubStmt);
     Localwild |= Sub.Wild;
   }
 
-  // If we are a function def, need to check return type
-  if (isTopLevel(Context, S)) {
-    const auto &Parents = Context->getParents(*S);
-    assert(!Parents.empty());
-    FunctionDecl* Parent = const_cast<FunctionDecl*>(Parents[0].get<FunctionDecl>());
-    assert(Parent != nullptr);
-    auto retType = Parent->getReturnType().getTypePtr();
-    if (retType->isPointerType()) {
-      CVarOption CV = Info.getVariable(Parent, Context);
-      Localwild |= isWild(CV) || containsUncheckedPtr(Parent->getReturnType());
-    }
-  }
-
   markChecked(S, Localwild);
 
   Wild = false;

clang/lib/3C/ProgramInfo.cpp

@@ -634,6 +634,9 @@ void ProgramInfo::addVariable(clang::DeclaratorDecl *D,
     llvm_unreachable("unknown decl type");
 
   assert("We shouldn't be adding a null CV to Variables map." && NewCV);
+  if (!canWrite(PLoc.getFileName())) {
+    NewCV->constrainToWild(CS, "Declaration in non-writable file", &PLoc);
+  }
   constrainWildIfMacro(NewCV, D->getLocation());
   Variables[PLoc] = NewCV;
 }
@@ -843,8 +846,7 @@ bool ProgramInfo::computeInterimConstraintState(
       CAtoms Tmp;
       getVarsFromConstraint(C, Tmp);
       AllValidVars.insert(Tmp.begin(), Tmp.end());
-      if (FilePaths.count(FileName) ||
-          FileName.find(BaseDir) != std::string::npos)
+      if (canWrite(FileName))
         ValidVarsVec.insert(ValidVarsVec.begin(), Tmp.begin(), Tmp.end());
     }
   }

clang/lib/3C/RewriteUtils.cpp

@@ -136,52 +136,93 @@ static void emit(Rewriter &R, ASTContext &C, std::string &OutputPostfix) {
   if (Verbose)
     errs() << "Writing files out\n";
 
-  // Check if we are outputing to stdout or not, if we are, just output the
-  // main file ID to stdout.
   SourceManager &SM = C.getSourceManager();
-  if (OutputPostfix == "-") {
-    if (const RewriteBuffer *B = R.getRewriteBufferFor(SM.getMainFileID()))
-      B->write(outs());
-  } else {
-    // Iterate over each modified rewrite buffer
-    for (auto Buffer = R.buffer_begin(); Buffer != R.buffer_end(); ++Buffer) {
-      if (const FileEntry *FE = SM.getFileEntryForID(Buffer->first)) {
-        assert(FE->isValid());
-
-        // Produce a path/file name for the rewritten source file.
-        // That path should be the same as the old one, with a
-        // suffix added between the file name and the extension.
-        // For example \foo\bar\a.c should become \foo\bar\a.checked.c
-        // if the OutputPostfix parameter is "checked" .
-        std::string PfName = sys::path::filename(FE->getName()).str();
-        std::string DirName = sys::path::parent_path(FE->getName()).str();
-        std::string FileName = sys::path::remove_leading_dotslash(PfName).str();
-        std::string Ext = sys::path::extension(FileName).str();
-        std::string Stem = sys::path::stem(FileName).str();
-        std::string NFile = Stem + "." + OutputPostfix + Ext;
-        if (!DirName.empty())
-          NFile = DirName + sys::path::get_separator().str() + NFile;
-
-        // Write this file if it was specified as a file on the command line.
-        std::string FeAbsS = "";
-        if (getAbsoluteFilePath(FE->getName(), FeAbsS))
-          FeAbsS = sys::path::remove_leading_dotslash(FeAbsS);
-
-        if (canWrite(FeAbsS)) {
-          std::error_code EC;
-          raw_fd_ostream Out(NFile, EC, sys::fs::F_None);
-
-          if (!EC) {
-            if (Verbose)
-              outs() << "writing out " << NFile << "\n";
-            Buffer->second.write(Out);
-          } else
-            errs() << "could not open file " << NFile << "\n";
-          // This is awkward. What to do? Since we're iterating, we could have
-          // created other files successfully. Do we go back and erase them? Is
-          // that surprising? For now, let's just keep going.
+  // Iterate over each modified rewrite buffer
+  for (auto Buffer = R.buffer_begin(); Buffer != R.buffer_end(); ++Buffer) {
+    if (const FileEntry *FE = SM.getFileEntryForID(Buffer->first)) {
+      assert(FE->isValid());
+
+      auto MaybeDumpUnwritableChange = [&]() {
+        if (DumpUnwritableChanges) {
+          errs() << "=== Beginning of new version of " << FE->getName() << " ===\n";
+          Buffer->second.write(errs());
+          errs() << "=== End of new version of " << FE->getName() << " ===\n";
+        } else {
+          DiagnosticsEngine &DE = C.getDiagnostics();
+          unsigned ID = DE.getCustomDiagID(
+              DiagnosticsEngine::Note,
+              "use the -dump-unwritable-changes option to see the new version "
+              "of the file");
+          DE.Report(SM.translateFileLineCol(FE, 1, 1), ID);
         }
+      };
+
+      // Check whether we are allowed to write this file.
+      std::string FeAbsS = "";
+      if (getAbsoluteFilePath(FE->getName(), FeAbsS))
+        FeAbsS = sys::path::remove_leading_dotslash(FeAbsS);
+      if (!canWrite(FeAbsS)) {
+        DiagnosticsEngine &DE = C.getDiagnostics();
+        unsigned ID = DE.getCustomDiagID(
+            DiagnosticsEngine::Error,
+            "3C internal error: 3C generated changes to this file even though it "
+            "is not allowed to write to the file "
+            "(https://github.com/correctcomputation/checkedc-clang/issues/387)");
+        DE.Report(SM.translateFileLineCol(FE, 1, 1), ID);
+        MaybeDumpUnwritableChange();
+        continue;
       }
+
+      if (OutputPostfix == "-") {
+        // Stdout mode
+        // TODO: If we don't have a rewrite buffer for the main file, that means
+        // we generated no changes to the file and we should print its original
+        // content
+        // (https://github.com/correctcomputation/checkedc-clang/issues/328#issuecomment-760243604).
+        if (Buffer->first == SM.getMainFileID()) {
+          // This is the new version of the main file. Print it to stdout.
+          Buffer->second.write(outs());
+        } else {
+          DiagnosticsEngine &DE = C.getDiagnostics();
+          unsigned ID = DE.getCustomDiagID(
+              DiagnosticsEngine::Error,
+              "3C generated changes to this file, which is under the base dir "
+              "but is not the main file and thus cannot be written in stdout "
+              "mode");
+          DE.Report(SM.translateFileLineCol(FE, 1, 1), ID);
+          MaybeDumpUnwritableChange();
+        }
+        continue;
+      }
+
+      // -output-postfix mode
+
+      // Produce a path/file name for the rewritten source file.
+      // That path should be the same as the old one, with a
+      // suffix added between the file name and the extension.
+      // For example \foo\bar\a.c should become \foo\bar\a.checked.c
+      // if the OutputPostfix parameter is "checked" .
+      std::string PfName = sys::path::filename(FE->getName()).str();
+      std::string DirName = sys::path::parent_path(FE->getName()).str();
+      std::string FileName = sys::path::remove_leading_dotslash(PfName).str();
+      std::string Ext = sys::path::extension(FileName).str();
+      std::string Stem = sys::path::stem(FileName).str();
+      std::string NFile = Stem + "." + OutputPostfix + Ext;
+      if (!DirName.empty())
+        NFile = DirName + sys::path::get_separator().str() + NFile;
+
+      std::error_code EC;
+      raw_fd_ostream Out(NFile, EC, sys::fs::F_None);
+
+      if (!EC) {
+        if (Verbose)
+          outs() << "writing out " << NFile << "\n";
+        Buffer->second.write(Out);
+      } else
+        errs() << "could not open file " << NFile << "\n";
+      // This is awkward. What to do? Since we're iterating, we could have
+      // created other files successfully. Do we go back and erase them? Is
+      // that surprising? For now, let's just keep going.
     }
   }
 }

clang/test/3C/base_subdir/canwrite_constraints_function_and_variable.c

@@ -0,0 +1,37 @@
+// Test that non-canWrite files are constrained not to change so that the final
+// annotations of other files are consistent with the original annotations of
+// the non-canWrite files. The currently supported cases are function and
+// variable declarations and checked regions.
+// (https://github.com/correctcomputation/checkedc-clang/issues/387)
+
+// TODO: When https://github.com/correctcomputation/checkedc-clang/issues/327 is
+// fixed, replace the absolute -I option with a .. in the #include directive.
+//
+// TODO: Windows compatibility?
+
+// "Lower" case: -base-dir should default to the working directory, so we should
+// not allow canwrite_constraints_function_and_variable.h to change, and the
+// internal types of q and the return should remain wild.
+//
+// RUN: cd %S && 3c -addcr -extra-arg=-I${PWD%/*} -output-postfix=checked -warn-all-root-cause -verify %s
+// RUN: FileCheck -match-full-lines -check-prefixes=CHECK_LOWER --input-file %S/canwrite_constraints_function_and_variable.checked.c %s
+// RUN: test ! -f %S/../canwrite_constraints_function_and_variable.checked.h
+// RUN: rm %S/canwrite_constraints_function_and_variable.checked.c
+
+// "Higher" case: When -base-dir is set to the parent directory, we can change
+// canwrite_constraints_function_and_variable.h, so both q and the return should
+// become checked.
+//
+// RUN: cd %S && 3c -addcr -extra-arg=-I${PWD%/*} -base-dir=${PWD%/*} -output-postfix=checked %s
+// RUN: FileCheck -match-full-lines -check-prefixes=CHECK_HIGHER --input-file %S/canwrite_constraints_function_and_variable.checked.c %s
+// RUN: FileCheck -match-full-lines -check-prefixes=CHECK_HIGHER --input-file %S/../canwrite_constraints_function_and_variable.checked.h %S/../canwrite_constraints_function_and_variable.h
+// RUN: rm %S/canwrite_constraints_function_and_variable.checked.c %S/../canwrite_constraints_function_and_variable.checked.h
+
+#include "canwrite_constraints_function_and_variable.h"
+
+int *bar(int *q) {
+  // CHECK_LOWER: int *bar(int *q : itype(_Ptr<int>)) : itype(_Ptr<int>) {
+  // CHECK_HIGHER: _Ptr<int> bar(_Ptr<int> q) {
+  foo(q);
+  return foo_var;
+}

clang/test/3C/base_subdir/canwrite_constraints_typedef.c

@@ -0,0 +1,25 @@
+// An example of a case (typedefs) that is not yet handled by the canWrite
+// constraints code and causes 3C to generate a change to an unwritable file.
+// Test that 3C generates an error diagnostic.
+// (https://github.com/correctcomputation/checkedc-clang/issues/387)
+
+// TODO: Ditto the TODO comments from
+// canwrite_constraints_function_and_variable.c re the RUN commands.
+// RUN: cd %S && 3c -addcr -extra-arg=-I${PWD%/*} -verify %s | FileCheck -match-full-lines %s
+
+// expected-error@unwritable_typedef.h:1 {{3C internal error: 3C generated changes to this file even though it is not allowed to write to the file}}
+// expected-note@unwritable_typedef.h:1 {{-dump-unwritable-changes}}
+
+#include "unwritable_typedef.h"
+
+foo_typedef p = ((void *)0);
+
+// To make sure we are testing what we want to test, make sure bar is rewritten
+// as if foo_typedef is unconstrained. If foo_typedef were constrained, we'd
+// expect bar to be rewritten differently.
+int *bar(void) {
+  // CHECK: _Ptr<int> bar(void) _Checked {
+  return p;
+  // Make sure 3C isn't inserting a cast or something clever like that.
+  // CHECK: return p;
+}

clang/test/3C/base_subdir/readme

@@ -0,0 +1,2 @@
+This subdirectory is used as the -base-dir by tests that want to have files
+outside the -base-dir.