Fix canwrite_constraints test and constrain itypes in unwritable files. (#593)

* Re-enable the canwrite_constraints test except for diagnostic
verification.

Reformat a CHECK comment to match the code reformatting done in
2a6a28e. (This wasn't caught at the
time because the test was XFAILed.) After this fix, the test passes
except for #581.

Also remove some obsolete TODO comments: there are no known problems
with these tests on Windows.

* Don't allow itypes in unwritable files to change.

Also take the opportunity to improve the documentation of
equateWithItype based on what I learned. Some things are probably still
wrong with the code and/or the documentation, but I think this is better
than the status quo.

Fixes #581.

* Add variable to make equateWithItype conditionals easier to understand.

Author: mattmccutchen-cci

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

@@ -162,12 +162,24 @@ class ConstraintVariable {
   void setValidDecl() { IsForDecl = true; }
   bool isForValidDecl() const { return IsForDecl; }
 
-  // Copies ConstAtoms from SrcVars vector into the main VarAtoms vector. This
-  // causes the solved type for the variable to be the same as the the type in
-  // source. This is currently called on function parameters with itypes when
-  // we don't want to allow the itype to solve to a fully checked type or an
-  // itype with a different pointer type.
-  virtual void equateWithItype(ProgramInfo &CS, bool ForceEquate) = 0;
+  // By default, 3C allows itypes to be re-solved arbitrarily. But in several
+  // cases, we need to restrict itype re-solving; this function applies those
+  // restrictions. (It isn't needed for fully checked types because 3C doesn't
+  // allow checked types to be re-solved yet.)
+  //
+  // In some cases, we don't want the checked portion of the type to change, but
+  // the itype can still become a fully checked type; we achieve that by copying
+  // ConstAtoms from SrcVars vector into the main VarAtoms vector, which forces
+  // the solved checked type for the variable to be the same as it was in the
+  // source. In other cases, we don't want the itype to change at all; to
+  // achieve that, we additionally constrain the internal variables to not
+  // change.
+  //
+  // Some cases in which the itype must not change at all are indicated by
+  // passing a reason for the "root cause of wildness" as ReasonUnchangeable.
+  // Otherwise ReasonUnchangeable should be set to the empty string.
+  virtual void equateWithItype(ProgramInfo &CS,
+                               const std::string &ReasonUnchangeable) = 0;
 
   virtual ConstraintVariable *getCopy(Constraints &CS) = 0;
 
@@ -473,7 +485,8 @@ class PointerVariableConstraint : public ConstraintVariable {
 
   ~PointerVariableConstraint() override{};
 
-  void equateWithItype(ProgramInfo &CS, bool ForceEquate) override;
+  void equateWithItype(ProgramInfo &CS,
+                       const std::string &ReasonUnchangeable) override;
 };
 
 typedef PointerVariableConstraint PVConstraint;
@@ -524,7 +537,8 @@ class FVComponentVariable {
   PVConstraint *getInternal() const { return InternalConstraint; }
   PVConstraint *getExternal() const { return ExternalConstraint; }
 
-  void equateWithItype(ProgramInfo &CS, bool ForceEquate) const;
+  void equateWithItype(ProgramInfo &CS,
+                       const std::string &ReasonUnchangeable) const;
 
   bool solutionEqualTo(Constraints &CS, const FVComponentVariable *CV,
                        bool ComparePtyp) const;
@@ -646,7 +660,8 @@ class FunctionVariableConstraint : public ConstraintVariable {
   bool isSolutionChecked(const EnvironmentMap &E) const override;
   bool isSolutionFullyChecked(const EnvironmentMap &E) const override;
 
-  void equateWithItype(ProgramInfo &CS, bool ForceEquate) override;
+  void equateWithItype(ProgramInfo &CS,
+                       const std::string &ReasonUnchangeable) override;
 
   ~FunctionVariableConstraint() override {}
 };

clang/lib/3C/ConstraintVariables.cpp

@@ -1954,8 +1954,8 @@ Atom *PointerVariableConstraint::getAtom(unsigned AtomIdx, Constraints &CS) {
   return nullptr;
 }
 
-void
-PointerVariableConstraint::equateWithItype(ProgramInfo &I, bool ForceEquate) {
+void PointerVariableConstraint::equateWithItype(
+    ProgramInfo &I, const std::string &ReasonUnchangeable) {
   Constraints &CS = I.getConstraints();
   assert(SrcVars.size() == Vars.size());
   for (unsigned VarIdx = 0; VarIdx < Vars.size(); VarIdx++) {
@@ -1966,7 +1966,7 @@ PointerVariableConstraint::equateWithItype(ProgramInfo &I, bool ForceEquate) {
       Vars[VarIdx] = SrcVars[VarIdx];
   }
   if (FV) {
-    FV->equateWithItype(I, ForceEquate);
+    FV->equateWithItype(I, ReasonUnchangeable);
   }
 }
 
@@ -2006,11 +2006,11 @@ bool FunctionVariableConstraint::isOriginallyChecked() const {
   return ReturnVar.ExternalConstraint->isOriginallyChecked();
 }
 
-void
-FunctionVariableConstraint::equateWithItype(ProgramInfo &I, bool ForceEquate) {
-  ReturnVar.equateWithItype(I, ForceEquate);
+void FunctionVariableConstraint::equateWithItype(
+    ProgramInfo &I, const std::string &ReasonUnchangeable) {
+  ReturnVar.equateWithItype(I, ReasonUnchangeable);
   for (auto Param : ParamVars)
-    Param.equateWithItype(I, ForceEquate);
+    Param.equateWithItype(I, ReasonUnchangeable);
 }
 
 void FVComponentVariable::mergeDeclaration(FVComponentVariable *From,
@@ -2115,18 +2115,32 @@ FVComponentVariable::FVComponentVariable(const QualType &QT,
   }
 }
 
-void
-FVComponentVariable::equateWithItype(ProgramInfo &I, bool ForceEquate) const {
+void FVComponentVariable::equateWithItype(
+    ProgramInfo &I, const std::string &ReasonUnchangeable) const {
   Constraints &CS = I.getConstraints();
-  bool IsGeneric = ExternalConstraint->getIsGeneric();
+  const std::string ReasonUnchangeable2 =
+      (ReasonUnchangeable.empty() && ExternalConstraint->getIsGeneric())
+          // TODO: Add a test for this root-cause message somewhere once
+          // diagnostic verification is re-enabled
+          // (https://github.com/correctcomputation/checkedc-clang/issues/503).
+          ? "Internal constraint for generic function declaration, "
+            "for which 3C currently does not support re-solving."
+          : ReasonUnchangeable;
   bool HasBounds = ExternalConstraint->srcHasBounds();
   bool HasItype = ExternalConstraint->srcHasItype();
-  if (HasItype && (ForceEquate || IsGeneric || HasBounds)) {
-    ExternalConstraint->equateWithItype(I, ForceEquate);
+  // If the type cannot change at all (ReasonUnchangeable2 is set), then we
+  // constrain both the external and internal types to not change. Otherwise, if
+  // the variable has bounds, then we don't want the checked (external) portion
+  // of the type to change because that could blow away the bounds, but we still
+  // allow the internal type to change so that the type can change from an itype
+  // to fully checked.
+  bool MustConstrainInternalType = !ReasonUnchangeable2.empty();
+  if (HasItype && (MustConstrainInternalType || HasBounds)) {
+    ExternalConstraint->equateWithItype(I, ReasonUnchangeable2);
     if (ExternalConstraint != InternalConstraint)
       linkInternalExternal(I, false);
-    if (IsGeneric)
-      InternalConstraint->constrainToWild(CS, "Internal constraint for generic function.");
+    if (MustConstrainInternalType)
+      InternalConstraint->constrainToWild(CS, ReasonUnchangeable2);
   }
 }
 

clang/lib/3C/ProgramInfo.cpp

@@ -412,21 +412,23 @@ bool ProgramInfo::link() {
     std::string FuncName = U.first;
     FVConstraint *G = U.second;
 
+    // If there was a checked type on a variable in the input program, it
+    // should stay that way. Otherwise, we shouldn't be adding a checked type
+    // to an extern function.
+    std::string Rsn = (G->hasBody() ? ""
+                                    : "Unchecked pointer in parameter or "
+                                      "return of external function " +
+                                          FuncName);
+
     // Handle the cases where itype parameters should not be treated as their
     // unchecked type.
-    G->equateWithItype(*this, !G->hasBody());
+    G->equateWithItype(*this, Rsn);
 
     // If we've seen this symbol, but never seen a body for it, constrain
     // everything about it.
     // Some global symbols we don't need to constrain to wild, like
     // malloc and free. Check those here and skip if we find them.
     if (!G->hasBody()) {
-      // If there was a checked type on a variable in the input program, it
-      // should stay that way. Otherwise, we shouldn't be adding a checked type
-      // to an extern function.
-      std::string Rsn =
-          "Unchecked pointer in parameter or return of external function " +
-          FuncName;
       const FVComponentVariable *Ret = G->getCombineReturn();
       Ret->getInternal()->constrainToWild(CS, Rsn);
       if (!Ret->getExternal()->srcHasItype() &&
@@ -456,13 +458,15 @@ bool ProgramInfo::link() {
       std::string FuncName = V.first;
       FVConstraint *G = V.second;
 
-      G->equateWithItype(*this, !G->hasBody());
+      std::string Rsn = (G->hasBody() ? ""
+                                      : "Unchecked pointer in parameter or "
+                                        "return of static function " +
+                                            FuncName + " in " + FileName);
+
+      G->equateWithItype(*this, Rsn);
 
       if (!G->hasBody()) {
 
-        std::string Rsn =
-            "Unchecked pointer in parameter or return of static function " +
-            FuncName + " in " + FileName;
         if (!G->getExternalReturn()->getIsGeneric())
           G->getExternalReturn()->constrainToWild(CS, Rsn);
         for (unsigned I = 0; I < G->numParams(); I++)
@@ -729,7 +733,7 @@ void ProgramInfo::addVariable(clang::DeclaratorDecl *D,
 
   assert("We shouldn't be adding a null CV to Variables map." && NewCV);
   if (!canWrite(PLoc.getFileName())) {
-    NewCV->equateWithItype(*this, true);
+    NewCV->equateWithItype(*this, "Declaration in non-writable file");
     NewCV->constrainToWild(CS, "Declaration in non-writable file", &PLoc);
   }
   constrainWildIfMacro(NewCV, D->getLocation());

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

@@ -1,29 +1,27 @@
-// TODO: refactor this test
-// https://github.com/correctcomputation/checkedc-clang/issues/503
-// XFAIL: *
-
 // 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: Windows compatibility?
-
 // RUN: rm -rf %t*
 
 // "Lower" case: -base-dir should default to the working directory, so we should
 // not allow canwrite_constraints.h to change, and the internal types of q and
 // the return should remain wild.
 //
-// RUN: cd %S && 3c -addcr -output-dir=%t.checked/base_subdir -warn-all-root-cause -verify %s --
+// The root cause warning verification part of this test is currently disabled
+// because of https://github.com/correctcomputation/checkedc-clang/issues/503;
+// the rest of the test still works. TODO: Re-enable warning verification.
+//
+// RUN: cd %S && 3c -alltypes -addcr -output-dir=%t.checked/base_subdir -warn-all-root-cause %s --
 // RUN: FileCheck -match-full-lines -check-prefixes=CHECK_LOWER --input-file %t.checked/base_subdir/canwrite_constraints.c %s
 // RUN: test ! -f %t.checked/canwrite_constraints.checked.h
 
 // "Higher" case: When -base-dir is set to the parent directory, we can change
 // canwrite_constraints.h, so both q and the return should become checked.
 //
-// RUN: cd %S && 3c -addcr -base-dir=.. -output-dir=%t.checked2 %s --
+// RUN: cd %S && 3c -alltypes -addcr -base-dir=.. -output-dir=%t.checked2 %s --
 // RUN: FileCheck -match-full-lines -check-prefixes=CHECK_HIGHER --input-file %t.checked2/base_subdir/canwrite_constraints.c %s
 // RUN: FileCheck -match-full-lines -check-prefixes=CHECK_HIGHER --input-file %t.checked2/canwrite_constraints.h %S/../canwrite_constraints.h
 
@@ -38,7 +36,7 @@ int *bar(int *q) {
 
 int gar(intptr a) {
   int *b = a;
-  //CHECK_LOWER: int* b = a;
+  //CHECK_LOWER: int *b = a;
   //CHECK_HIGHER: _Ptr<int> b = a;
   return *b;
 }

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

@@ -7,8 +7,6 @@
 // an error diagnostic.
 // (https://github.com/correctcomputation/checkedc-clang/issues/387)
 
-// TODO: Ditto the TODO comments from canwrite_constraints.c re the RUN
-// commands.
 // RUN: cd %S && 3c -addcr -verify %s --
 
 // expected-error@../base_subdir_partial_defn.h:1 {{3C internal error: 3C generated changes to this file even though it is not allowed to write to the file}}

clang/test/3C/canwrite_constraints.h

@@ -47,3 +47,15 @@ void unwritable_cast(void((*g)(int *q)) : itype(_Ptr<void(_Ptr<int>)>)) {
   // expected-warning@+1 {{3C internal error: tried to insert a cast into an unwritable file}}
   (*g)(p);
 }
+
+// Make sure that FVComponentVariable::equateWithItype prevents both of these
+// from being changed.
+//
+// TODO: Add the correct expected root cause warnings once diagnostic
+// verification is re-enabled
+// (https://github.com/correctcomputation/checkedc-clang/issues/503).
+void unwritable_func_with_itype(int *p : itype(_Array_ptr<int>)) {}
+
+void unwritable_func_with_itype_and_bounds(int *p
+                                           : itype(_Array_ptr<int>) count(12)) {
+}