Fix the root-cause tests. (#613)

mattmccutchen-cci · john-h-kastner · web-flow · commit 157e2cdfe85d · 2021-06-21T17:21:16.000-04:00
* Re-enable root-cause-related tests with minor changes. - Update the expected warnings. - Add a test of a global function with no definition to go along with the existing test of a global variable, so we can test that the new equateWithItype code gets the correct PSL even if the AtomSourceMap is turned off. - Add a test of the root-cause warning for the internal constraint of a generic function (a feature added in #593 while the test was disabled). * Only count atoms in writable files when computing root cause stats * Make equateWithItype take a PSL for constraints to wild. Change the "unwritable code" case to actually pass a PSL. The "external variable or function" case doesn't yet. Also, make PointerVariableConstraint::equateWithItype attach ReasonUnchangeable to the wildness constraints it generates. Apparently this case affects unwritable global variables and I missed it in #593. * Improve the reliability of the AtomSourceMap fallback. - We shouldn't exclude unwritable elements because they may well be the root cause of wildness of pointers we care about. This decision shouldn't be confused with excluding unwritable pointers from the count of affected pointers, which we definitely _do_ want. - Prioritize variable definitions over references. * Improve the comment about the `&i` root-cause warning. Co-authored-by: John Kastner <john@correctcomputation.com>
diff --git a/clang/include/clang/3C/ConstraintVariables.h b/clang/include/clang/3C/ConstraintVariables.h
@@ -179,7 +179,8 @@ class ConstraintVariable {
   // 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;
+                               const std::string &ReasonUnchangeable,
+                               PersistentSourceLoc *PSL) = 0;
 
   virtual ConstraintVariable *getCopy(Constraints &CS) = 0;
 
@@ -485,8 +486,8 @@ class PointerVariableConstraint : public ConstraintVariable {
 
   ~PointerVariableConstraint() override{};
 
-  void equateWithItype(ProgramInfo &CS,
-                       const std::string &ReasonUnchangeable) override;
+  void equateWithItype(ProgramInfo &CS, const std::string &ReasonUnchangeable,
+                       PersistentSourceLoc *PSL) override;
 };
 
 typedef PointerVariableConstraint PVConstraint;
@@ -537,8 +538,8 @@ class FVComponentVariable {
   PVConstraint *getInternal() const { return InternalConstraint; }
   PVConstraint *getExternal() const { return ExternalConstraint; }
 
-  void equateWithItype(ProgramInfo &CS,
-                       const std::string &ReasonUnchangeable) const;
+  void equateWithItype(ProgramInfo &CS, const std::string &ReasonUnchangeable,
+                       PersistentSourceLoc *PSL) const;
 
   bool solutionEqualTo(Constraints &CS, const FVComponentVariable *CV,
                        bool ComparePtyp) const;
@@ -660,8 +661,8 @@ class FunctionVariableConstraint : public ConstraintVariable {
   bool isSolutionChecked(const EnvironmentMap &E) const override;
   bool isSolutionFullyChecked(const EnvironmentMap &E) const override;
 
-  void equateWithItype(ProgramInfo &CS,
-                       const std::string &ReasonUnchangeable) override;
+  void equateWithItype(ProgramInfo &CS, const std::string &ReasonUnchangeable,
+                       PersistentSourceLoc *PSL) override;
 
   ~FunctionVariableConstraint() override {}
 };
diff --git a/clang/lib/3C/ConstraintVariables.cpp b/clang/lib/3C/ConstraintVariables.cpp
@@ -1955,18 +1955,22 @@ Atom *PointerVariableConstraint::getAtom(unsigned AtomIdx, Constraints &CS) {
 }
 
 void PointerVariableConstraint::equateWithItype(
-    ProgramInfo &I, const std::string &ReasonUnchangeable) {
+    ProgramInfo &I, const std::string &ReasonUnchangeable,
+    PersistentSourceLoc *PSL) {
   Constraints &CS = I.getConstraints();
   assert(SrcVars.size() == Vars.size());
   for (unsigned VarIdx = 0; VarIdx < Vars.size(); VarIdx++) {
     ConstAtom *CA = SrcVars[VarIdx];
     if (isa<WildAtom>(CA))
-      CS.addConstraint(CS.createGeq(Vars[VarIdx], CA, true));
+      CS.addConstraint(CS.createGeq(
+          Vars[VarIdx], CA,
+          ReasonUnchangeable.empty() ? DEFAULT_REASON : ReasonUnchangeable, PSL,
+          true));
     else
       Vars[VarIdx] = SrcVars[VarIdx];
   }
   if (FV) {
-    FV->equateWithItype(I, ReasonUnchangeable);
+    FV->equateWithItype(I, ReasonUnchangeable, PSL);
   }
 }
 
@@ -2007,10 +2011,11 @@ bool FunctionVariableConstraint::isOriginallyChecked() const {
 }
 
 void FunctionVariableConstraint::equateWithItype(
-    ProgramInfo &I, const std::string &ReasonUnchangeable) {
-  ReturnVar.equateWithItype(I, ReasonUnchangeable);
+    ProgramInfo &I, const std::string &ReasonUnchangeable,
+    PersistentSourceLoc *PSL) {
+  ReturnVar.equateWithItype(I, ReasonUnchangeable, PSL);
   for (auto Param : ParamVars)
-    Param.equateWithItype(I, ReasonUnchangeable);
+    Param.equateWithItype(I, ReasonUnchangeable, PSL);
 }
 
 void FVComponentVariable::mergeDeclaration(FVComponentVariable *From,
@@ -2124,14 +2129,12 @@ FVComponentVariable::FVComponentVariable(const QualType &QT,
   }
 }
 
-void FVComponentVariable::equateWithItype(
-    ProgramInfo &I, const std::string &ReasonUnchangeable) const {
+void FVComponentVariable::equateWithItype(ProgramInfo &I,
+                                          const std::string &ReasonUnchangeable,
+                                          PersistentSourceLoc *PSL) const {
   Constraints &CS = I.getConstraints();
   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;
@@ -2145,11 +2148,11 @@ void FVComponentVariable::equateWithItype(
   // to fully checked.
   bool MustConstrainInternalType = !ReasonUnchangeable2.empty();
   if (HasItype && (MustConstrainInternalType || HasBounds)) {
-    ExternalConstraint->equateWithItype(I, ReasonUnchangeable2);
+    ExternalConstraint->equateWithItype(I, ReasonUnchangeable2, PSL);
     if (ExternalConstraint != InternalConstraint)
       linkInternalExternal(I, false);
     if (MustConstrainInternalType)
-      InternalConstraint->constrainToWild(CS, ReasonUnchangeable2);
+      InternalConstraint->constrainToWild(CS, ReasonUnchangeable2, PSL);
   }
 }
 
diff --git a/clang/lib/3C/ProgramInfo.cpp b/clang/lib/3C/ProgramInfo.cpp
@@ -402,6 +402,7 @@ bool ProgramInfo::link() {
           "External global variable " + VarName + " has no definition";
       const std::set<PVConstraint *> &C = GlobalVariableSymbols[VarName];
       for (const auto &Var : C) {
+        // TODO: Is there an easy way to get a PSL to attach to the constraint?
         Var->constrainToWild(CS, Rsn);
       }
     }
@@ -423,7 +424,9 @@ bool ProgramInfo::link() {
 
     // Handle the cases where itype parameters should not be treated as their
     // unchecked type.
-    G->equateWithItype(*this, Rsn);
+    // TODO: Ditto re getting a PSL (in the case in which Rsn is non-empty and
+    // it is actually used).
+    G->equateWithItype(*this, Rsn, nullptr);
 
     // If we've seen this symbol, but never seen a body for it, constrain
     // everything about it.
@@ -464,7 +467,8 @@ bool ProgramInfo::link() {
                                         "return of static function " +
                                             FuncName + " in " + FileName);
 
-      G->equateWithItype(*this, Rsn);
+      // TODO: Ditto re getting a PSL
+      G->equateWithItype(*this, Rsn, nullptr);
 
       if (!G->hasBody()) {
 
@@ -734,7 +738,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, "Declaration in non-writable file");
+    NewCV->equateWithItype(*this, "Declaration in non-writable file", &PLoc);
     NewCV->constrainToWild(CS, "Declaration in non-writable file", &PLoc);
   }
   constrainWildIfMacro(NewCV, D->getLocation());
@@ -1015,7 +1019,9 @@ bool ProgramInfo::computeInterimConstraintState(
       auto *SearchVA = dyn_cast<VarAtom>(SearchAtom);
       if (SearchVA && AllValidVars.find(SearchVA) != AllValidVars.end()) {
         CState.RCMap[SearchVA->getLoc()].insert(VA->getLoc());
-        TmpCGrp.insert(SearchVA->getLoc());
+
+        if (ValidVarsKey.find(SearchVA->getLoc()) != ValidVarsKey.end())
+          TmpCGrp.insert(SearchVA->getLoc());
         if (DirectWildVarAtoms.find(SearchVA) == DirectWildVarAtoms.end()) {
           OnlyIndirect.insert(SearchVA->getLoc());
         }
@@ -1038,6 +1044,12 @@ bool ProgramInfo::computeInterimConstraintState(
   findIntersection(CState.TotalNonDirectWildAtoms, ValidVarsKey,
                    CState.InSrcNonDirectWildAtoms);
 
+  // The ConstraintVariable for a variable normally appears in Variables for the
+  // definition, but it may also be reused directly in ExprConstraintVars for a
+  // reference to that variable. We want to give priority to the PSL of the
+  // definition, not the reference. We currently achieve this by processing
+  // Variables before ExprConstraintVars and making insertIntoPtrSourceMap not
+  // overwrite a PSL already recorded for a given atom.
   for (const auto &I : Variables)
     insertIntoPtrSourceMap(&(I.first), I.second);
   for (const auto &I : ExprConstraintVars) {
@@ -1070,13 +1082,13 @@ void ProgramInfo::insertIntoPtrSourceMap(const PersistentSourceLoc *PSL,
   std::string FilePath = PSL->getFileName();
   if (canWrite(FilePath))
     CState.ValidSourceFiles.insert(FilePath);
-  else
-    return;
 
   if (auto *PV = dyn_cast<PVConstraint>(CV)) {
     for (auto *A : PV->getCvars())
       if (auto *VA = dyn_cast<VarAtom>(A))
-        CState.AtomSourceMap[VA->getLoc()] = PSL;
+        // Don't overwrite a PSL already recorded for a given atom: see the
+        // comment in computeInterimConstraintState.
+        CState.AtomSourceMap.insert(std::make_pair(VA->getLoc(), PSL));
     // If the PVConstraint is a function pointer, create mappings for parameter
     // and return variables.
     if (auto *FV = PV->getFV()) {
diff --git a/clang/test/3C/base_subdir/canwrite_constraints.c b/clang/test/3C/base_subdir/canwrite_constraints.c
@@ -10,12 +10,7 @@
 // not allow canwrite_constraints.h to change, and the internal types of q and
 // the return should remain wild.
 //
-// TODO: Fix the discrepancies in warnings that were introduced while the
-// diagnostic verifier was disabled
-// (https://github.com/correctcomputation/checkedc-clang/issues/609)
-// and then re-enable diagnostic verification here.
-//
-// RUN: cd %S && 3c -alltypes -addcr -output-dir=%t.checked/base_subdir -warn-all-root-cause %s --
+// RUN: cd %S && 3c -alltypes -addcr -output-dir=%t.checked/base_subdir -warn-all-root-cause %s -- -Xclang -verify
 // 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
 
diff --git a/clang/test/3C/canwrite_constraints.h b/clang/test/3C/canwrite_constraints.h
@@ -17,7 +17,12 @@ int *foo_var = ((void *)0);
 // Make sure we don't allow the types of casts in non-writable files to be
 // changed. This problem was seen with the inline definition of `atol(p)` as
 // `strtol(p, (char **) NULL, 10)` in the system headers.
+
+// Now that itypes can be re-solved, an itype in a non-writable file generates a
+// root cause warning just like a fully unchecked type.
+// expected-warning@+1 {{Declaration in non-writable file}}
 void strtol_like(int *p : itype(_Ptr<int>));
+
 void atol_like() {
   // expected-warning@+1 {{Expression in non-writable file}}
   strtol_like((int *)0);
@@ -34,11 +39,14 @@ inline void no_op() {}
 // expected-warning@+1 {{Declaration in non-writable file}}
 typedef int *intptr;
 // CHECK_HIGHER: typedef _Ptr<int> intptr;
+
 // Test the unwritable cast internal warning
 // (https://github.com/correctcomputation/checkedc-clang/issues/454) using the
 // known bug with itypes and function pointers
 // (https://github.com/correctcomputation/checkedc-clang/issues/423) as an
 // example.
+
+// expected-warning@+1 {{Declaration in non-writable file}}
 void unwritable_cast(void((*g)(int *q)) : itype(_Ptr<void(_Ptr<int>)>)) {
   // expected-warning@+1 {{Declaration in non-writable file}}
   int *p = 0;
@@ -50,20 +58,24 @@ void unwritable_cast(void((*g)(int *q)) : itype(_Ptr<void(_Ptr<int>)>)) {
 
 // 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/609).
+
+// expected-warning@+1 {{Declaration in non-writable file}}
 void unwritable_func_with_itype(int *p : itype(_Array_ptr<int>)) {}
 
+// expected-warning@+1 {{Declaration in non-writable file}}
 void unwritable_func_with_itype_and_bounds(int *p
                                            : itype(_Array_ptr<int>) count(12)) {
 }
 
 // Test for https://github.com/correctcomputation/checkedc-clang/issues/580
+// expected-warning@+1 {{Declaration in non-writable file}}
 _Itype_for_any(T) void my_generic_function(void *p : itype(_Ptr<T>));
 
 void unwritable_type_argument() {
   int i;
+  // This warning relates to the atom representing the temporary pointer of
+  // `&i`. https://github.com/correctcomputation/checkedc-clang/issues/618 would
+  // make 3C smarter to avoid the need to constrain the temporary pointer.
+  // expected-warning@+1 {{Expression in non-writable file}}
   my_generic_function(&i);
 }
diff --git a/clang/test/3C/root_cause.c b/clang/test/3C/root_cause.c
@@ -1,8 +1,3 @@
-// TODO: Fix the failures in this test that were introduced while the diagnostic
-// verifier was disabled
-// (https://github.com/correctcomputation/checkedc-clang/issues/609).
-// XFAIL: *
-
 // RUN: 3c -base-dir=%S -alltypes -warn-root-cause %s -- -Xclang -verify -Wno-everything
 
 // This test is unusual in that it checks for the errors in the code
@@ -41,8 +36,11 @@ void test1() {
   int *d = malloc(1); // expected-warning {{Unsafe call to allocator function}}
 }
 
-extern int *
-    glob; // expected-warning {{External global variable glob has no definition}}
+// expected-warning@+1 {{External global variable glob has no definition}}
+extern int *glob;
+
+// expected-warning@+1 {{Unchecked pointer in parameter or return of external function glob_f}}
+int *glob_f(void);
 
 void (*f)(void *); // expected-warning {{Default void* type}}
 
@@ -51,3 +49,6 @@ typedef struct {
   float f;
 } A, *PA;
 // expected-warning@-1 {{Unable to rewrite a typedef with multiple names}}
+
+// expected-warning@+1 {{Internal constraint for generic function declaration, for which 3C currently does not support re-solving.}}
+_Itype_for_any(T) void remember(void *p : itype(_Ptr<T>)) {}
